Make Manifold and CrossSection safe for concurrent const access#1636
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1636 +/- ##
==========================================
- Coverage 95.34% 95.34% -0.01%
==========================================
Files 34 34
Lines 7696 7708 +12
==========================================
+ Hits 7338 7349 +11
- Misses 358 359 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm thinking about whether to use this or mutex, since I doubt if we can move to c++20 in a short period of time, since we probably want to wait until the major users move to c++20. I am not worried about the overhead introduced by mutex (probably a |
|
Good point about the C++20 timeline. I'm happy to switch to a One question on the semantics: should copies share the mutex or get independent ones? Currently the copy constructor copies Concretely, the approach would be:
|
a6e7d50 to
0870683
Compare
|
Updated the PR to use Approach matches what I outlined in the earlier comment — per-instance mutex (non-recursive is sufficient), fresh mutex on copy/assign, source locked during copy. |
| transform_ = other.transform_; | ||
| } | ||
|
|
||
| CrossSection& CrossSection::operator=(const CrossSection& other) { |
There was a problem hiding this comment.
I think for this to be thread safe, we need another mutex for the CrossSection object itself?
There was a problem hiding this comment.
Good catch — updated operator= for both classes to lock this as well, using std::scoped_lock for deadlock avoidance. Left the copy constructors locking only the source since this is under construction and can't be accessed concurrently.
|
|
||
| Manifold& Manifold::operator=(const Manifold& other) { | ||
| if (this != &other) { | ||
| std::lock_guard<std::mutex> lock(*other.pNodeMutex_); |
There was a problem hiding this comment.
Same issue as the CrossSection copy constructor.
There was a problem hiding this comment.
Same fix applied here — std::scoped_lock on both mutexes to avoid deadlock from concurrent a = b / b = a.
Add a per-instance shared_ptr<mutex> to both Manifold and CrossSection, protecting the mutable pNode_/paths_ fields from concurrent access. All reads of pNode_ in const methods go through either LoadPNode() (builder methods) or GetCsgLeafNode() (query methods), both of which lock the mutex. CrossSection::GetPaths() and Transform() similarly lock pathsMutex_. The mutex is wrapped in shared_ptr so it works with defaulted move constructors. Copy constructor and operator= lock the source's mutex to read its fields. operator= uses std::scoped_lock on both self and source mutexes for deadlock avoidance. Fixes elalish#1635 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d317c84 to
39f10e5
Compare
Summary
Add a per-instance
shared_ptr<mutex>to bothManifoldandCrossSection, protecting the mutablepNode_/paths_fields from data races when multiple threads call const methods on the same object.All reads of
pNode_in const methods go through eitherLoadPNode()(builder methods likeTranslate,Boolean) orGetCsgLeafNode()(query methods likeNumVert,GetMeshGL), both of which lock the mutex.CrossSection::GetPaths()andTransform()similarly lockpathsMutex_.The mutex is wrapped in
shared_ptrso it works with defaulted move constructors. Copy constructor andoperator=lock the source's mutex to read its fields, andoperator=creates a fresh mutex for the destination.Design
Cost is one
shared_ptr(16 bytes) per object plus an uncontended mutex lock (~20ns) per method call — negligible compared to actual mesh operations. We consideredstd::call_once(protects queries but not builders) and C++17std::atomic_load/std::atomic_storeonshared_ptr, but the per-object mutex is clearer — allpNode_access goes through two accessors (LoadPNodeandGetCsgLeafNode), making it hard to accidentally add an unprotected read.When the project moves to C++20, this can be simplified to
std::atomic<std::shared_ptr<CsgNode>>with no mutex needed (see #1153).Fixes #1635
Test plan
🤖 Generated with Claude Code