Fix Groot2 publisher thread safety and hook handling#1165
Merged
Conversation
Apply code-review cleanups to the thread-safety fix: - Extract the duplicated callback drain-gate (CallbackState/CallbackGuard and HookCallbackState/HookCallbackGuard were byte-identical) into a single shared BT::details::CallbackGate in utils/callback_gate.h. notify_all() now fires only when a drain is actually waiting. - Replace the 69-line injected hook lambda and its immediately-invoked inner lambda with PImpl::runHook(). The BREAKPOINT_REACHED message is now built and sent without holding hook->mutex, and only the ZMQ send holds publisher_mutex. - Drop the Hook::removed tombstone from the public protocol header: with the single lock order established (hooks_map_mutex before hook->mutex), enableAllHooks() and HOOKS_DUMP traverse the maps under hooks_map_mutex, so no stale snapshot can resurrect an erased hook and 'enabled = false' suffices for stale callback copies. - GET_TRANSITIONS: swap the transitions buffer out under status_mutex and serialize outside the lock the status callback contends on. - Call unsubscribeFromTreeChanges() in the destructors of the five other StatusChangeLogger subclasses; the base-class destructor call runs too late to protect derived state, so each dtor must drain callbacks first. - Deduplicate the hook-JSON literals in the Groot2 tests into Groot2Test::makeHook(); use make_unique for PImpl; rely on CTAD for lock guards in touched code. Verified: 495/495 tests (Release), 499/499 under TSan with the CI suppressions file, zero ThreadSanitizer warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI builds zmq from source with Conan and links it statically, so the module-based rule 'race:libzmq.so' does not match and zmq-internal false positives (encoder_base_t::encode, msg_t::close) fail the Groot2 hook tests. Match the zmq namespace instead, which works regardless of how the library is linked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StatusChangeLogger is the base class of a public extension point: every logger, including user-defined ones, embeds its layout. The thread-safety fix already changes that layout (and, being header-only, the old inlined implementation survives in consumer binaries even after a library upgrade), so take the opportunity to pay the ABI break only once: - all data members move into a PImpl defined in the new src/loggers/abstract_logger.cpp - the member functions are no longer inline, so future behavior fixes in this class can ship as an ABI-compatible library update Adding or changing state in StatusChangeLogger no longer affects the layout derived classes compile against. Note that changing its virtual interface would still break ABI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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
1 participant
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
TDD evidence
The new tests were added and run before the implementation changes.
Red phase:
Green phase:
Validation
pre-commit run --files <changed files>./run_clang_tidy.shCompatibility
This is source-compatible, but it changes the in-memory layout of the public
StatusChangeLoggerandMonitor::Hooktypes, so consumers should rebuild against the updated library. The hook tombstone is internal state and is not serialized in the Groot2 JSON protocol.