fix(NObserver): Lock-Order-Inversion Deadlock #5168 #5169
Conversation
AsyncObserver::start() used NObserver::mutex() which was removed to fix lock-order-inversion deadlock. Replace with atomic compare_exchange_strong on _started flag. Also fix _done flag semantics: - Initialize _done=true (thread not running) - dequeue() sets _done=false first thing (signals thread started) - start() waits for _done to become false - Fixes timeout in start() due to incorrect wait condition
There was a problem hiding this comment.
Pull request overview
This pull request fixes a lock-order-inversion deadlock issue (#5168) in the observer notification system by removing mutexes from Observer, NObserver, and AsyncObserver classes and relying on atomic operations instead.
Changes:
- Removed mutex-based synchronization from Observer and NObserver classes, using atomic loads for
_pObjectto prevent lock-order-inversion deadlocks when handlers call back into NotificationCenter - Updated AsyncObserver to use compare-and-exchange for start() synchronization and corrected the
_doneflag initialization to properly reflect thread state - Removed the
mutex()accessor method from NObserver as it's no longer needed
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Foundation/include/Poco/Observer.h | Removed mutex lock from notify() and disable(), using atomic load for _pObject instead |
| Foundation/include/Poco/NObserver.h | Removed mutex and locks from handle(), handleSync(), match(), and disable() methods; removed mutex() accessor; added explanatory comment about deadlock avoidance |
| Foundation/include/Poco/AsyncObserver.h | Changed _done initialization to true (correct initial state), replaced mutex lock in start() with CAS operation, adjusted synchronization logic to use _done flag instead of _started |
The changes are well-thought-out and address the deadlock issue by removing the mutex that was causing lock-order inversion. The trade-off is that observers should not be modified (via assignment operators) concurrently with notification delivery, which is a reasonable assumption for typical observer pattern usage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is true, but does not apply to this change because it was always like that - destructor did not acquire mutex. |
| setMatcher(observer._matcher); | ||
| _started = false; | ||
| _done =false; | ||
| _done = true; |
There was a problem hiding this comment.
Should variable _done be named _running or _active?
No description provided.