Improvements to thread safety #2736
Conversation
jstone-lucasfilm
requested review from
JGamache-autodesk and
niklasharrysson
jstone-lucasfilm
force-pushed
the
dev_thread_safety
branch
from
068184b to
9f6e2fe
Compare
jstone-lucasfilm
changed the title
jstone-lucasfilm
force-pushed
the
dev_thread_safety
branch
2 times, most recently
from
2844810 to
71e3c67
Compare
- Fix a race condition in the Document cache, where the `valid` flag could be simultaneously written and read from different threads. - Add an initial Thread Safety section to the Developer Guidelines.
jstone-lucasfilm
force-pushed
the
dev_thread_safety
branch
from
71e3c67 to
88cfd14
Compare
source/MaterialXCore/Document.cpp
Outdated
| void setDocument(weak_ptr<Document> document) | ||
| { | ||
| _doc = document; | ||
| invalidate(); |
There was a problem hiding this comment.
What happens if another thread tries to fetch cached data in the miniscule moment after _doc is set, but before invalidate() has had the time to set _valid to false?
I would recommend using a C++17 std::shared_mutex instead for robustness. It allows multiple-reader/single-writer semantics.
There was a problem hiding this comment.
That's a really good suggestion, @JGamache-autodesk, and I've updated the PR to use this approach.
Following up on review notes from Jerry Gamache, this change switches over to a std::shared_mutex pattern, which should be simpler and more robust.
| const string& nodeGraphName = elem->getAttribute(PortElement::NODE_GRAPH_ATTRIBUTE); | ||
| const string& nodeString = elem->getAttribute(NodeDef::NODE_ATTRIBUTE); | ||
| const string& nodeDefString = elem->getAttribute(InterfaceElement::NODE_DEF_ATTRIBUTE); | ||
| lock.unlock(); |
There was a problem hiding this comment.
Thanks! That is the best currently possible with C++ (up to 23). Rechecking the _valid condition after re-acquiring the lock makes sure we handle the case where another thread pre-emptively rebuilds in the small window after the shared lock is released, especially since this thread now has to wait for all shared locks to be released before proceeding.
LGTM!
jstone-lucasfilm
merged commit 5961041
into
AcademySoftwareFoundation:main
2 participants
validflag could be simultaneously written and read from different threads.std::shared_mutexpattern in the Document cache for greater robustness and clearer guarantees.