Fix data race in LazyObject::calculate() under thread-safe observer pattern - #2774
Closed
amanyadav2022 wants to merge 1 commit into
Closed
amanyadav2022 wants to merge 1 commit into
amanyadav2022 wants to merge 1 commit into
Conversation
…attern QL_ENABLE_THREAD_SAFE_OBSERVER_PATTERN makes Observable/Observer registration thread-safe (via a recursive_mutex on their internal sets), but LazyObject::calculate() itself reads and writes calculated_/frozen_/failed_/updating_ with no synchronization at all, so calling calculate() concurrently on the same LazyObject is a data race, confirmed under ThreadSanitizer (10/10 runs). Guard calculate() and the other methods touching this state with a recursive_mutex, following the same locking pattern already used by Observer. The mutex is wrapped so LazyObject keeps its existing implicit copy/move semantics, and the whole change is compiled out when QL_ENABLE_THREAD_SAFE_OBSERVER_PATTERN is off. Fixes #2633. Add testConcurrentCalculate to test-suite/lazyobject.cpp, verified to fail deterministically under TSan before this fix and pass cleanly after.
|
Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it. |
|
|
Owner
|
Thanks for the contribution, but as I wrote in the original issue, I don't think this is something that can or should be fixed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2633.
QL_ENABLE_THREAD_SAFE_OBSERVER_PATTERNmakesObservable/Observerregistration thread-safe (via arecursive_mutexguarding their internal sets inobservable.hpp/.cpp), butLazyObject::calculate()itself reads and writescalculated_/frozen_/failed_/updating_with no synchronization of its own. So callingcalculate()concurrently on the sameLazyObjectis a data race, even with the macro enabled -- the macro name is a bit misleading, since it does not makeLazyObjectitself safe to use from multiple threads.Confirmed with ThreadSanitizer: a minimal repro (8 threads calling
calculate()concurrently on oneLazyObject, 500 iterations) reproduces the race 10/10 runs, always at thecalculated_check-and-set incalculate().Fix
Guard
calculate()and the other methods touching this state (update(),recalculate(),freeze(),unfreeze(),forwardFirstNotificationOnly(),alwaysForwardNotifications(),isCalculated(),setCalculated()) with arecursive_mutex, following the same locking pattern already used byObserverin this codebase. The mutex is wrapped in a small helper soLazyObjectkeeps its existing implicit copy/move semantics (a rawstd::recursive_mutexmember would otherwise silently delete them). The whole change is compiled out whenQL_ENABLE_THREAD_SAFE_OBSERVER_PATTERNis off, so there is no cost or behavior change for the default configuration.Testing
testConcurrentCalculateintest-suite/lazyobject.cpp, guarded by the same macro (matching the existing convention intest-suite/observable.cpp). Verified with real CI-style TSan settings (TSAN_OPTIONS=halt_on_error=1 exitcode=66): fails deterministically (exit 66) before this fix, 5/5 runs; passes cleanly (exit 0) after, 5/5 runs.QL_ENABLE_THREAD_SAFE_OBSERVER_PATTERN=ON(Debug, GCC 13.3.0): all tests pass, no regressions.🤖 Generated with Claude Code