Make diagnostics mock/live source handoff thread safe - #10
Open
khuzaymahbinharis-jpg wants to merge 1 commit into
Open
Make diagnostics mock/live source handoff thread safe#10khuzaymahbinharis-jpg wants to merge 1 commit into
khuzaymahbinharis-jpg wants to merge 1 commit into
Conversation
Switching the DiagnosticsPanel back to mock destroyed the live RosDiagnosticsSource from the Qt thread while a ROS executor thread could still be inside its subscription callback, because the callback captured a raw `this`. Received state now lives in a separately owned block that the callback captures by shared_ptr, so a callback can never dereference a destroyed source. DiagnosticsSource gains a stop() hook that RosDiagnosticsSource uses to drop the subscription and retire its state under the mutex, and the panel retires the outgoing source before installing its replacement. The refresh timer pins its source for the duration of a tick so it cannot observe a half-replaced source. Adds a gtest stress suite that churns live sources under multi-threaded traffic. Mutating the source back to capturing `this` makes the suite report a heap-use-after-free under AddressSanitizer, confirming it detects the original defect. Co-authored-by: Cursor <cursoragent@cursor.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
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 the last merge blocker carried over from the July 11 sprint sheet: switching the diagnostics panel from live back to mock could destroy
RosDiagnosticsSourcewhile a ROS callback was still using it.The subscription callback captured a raw
this, so tearing the source down from the Qt thread could free the object out from under an executor thread that was mid-callback.What changed
RosDiagnosticsSourceSharedStateblock; the callback captures it byshared_ptrinstead of capturingthisDiagnosticsSourcestop()hook (no-op by default) so the panel can retire a source without RTTI checksRosDiagnosticsSource::stop()DiagnosticsPanel::configureSource()DiagnosticsPanel::refresh()shared_ptrfor the whole tick, so a swap mid-tick cannot leave widgets reading a half-replaced source~DiagnosticsPanel()The UI thread is never blocked for longer than a single vector assignment, and no source is leaked. ROS callback work stays off the Qt widget path — the callback only writes into the state block, and widgets are still only touched by the refresh timer.
docs/DIAGNOSTICS_SOURCE_LIFECYCLE.mddocuments the ownership rules and the test procedure.Repeatable stress check
test/test_ros_diagnostics_source.cppis the automated equivalent of toggling the checkbox repeatedly while the cycle publisher runs. It spins a 4-threadMultiThreadedExecutorwhile a separate thread publishes every 200 microseconds, then creates, reads, stops, and destroys live sources in a loop.ReportsWaitingBeforeAnyMessageArrivesNormalizesReceivedDiagnosticsStopFreezesStateAndIgnoresLaterMessagesStopIsIdempotentstop()plus later readsRepeatedLiveMockChurnUnderTrafficIsSafeDestructionWithoutExplicitStopIsSafeConcurrentReadsDuringTeardownAreSafestop()ChurnLeavesNoLingeringSubscriptionThe gtest target builds from sources directly, so it links neither Qt nor RViz and runs headless in CI.
Test results (Ubuntu 24.04 / ROS 2 Jazzy / WSL2)
Breakdown:
lint_cmake1,xmllint2,test_package_metadata9,test_ros_diagnostics_source8.Proof the stress check detects the original defect
A stress test that passes against both the broken and the fixed code proves nothing, so the suite was validated by mutation under AddressSanitizer. The test plus
ros_diagnostics_source.cppwere compiled standalone twice — once against this branch, once against a mutated copy whose callback capturesthisand whosestop()does not drop the subscription (the pre-fix ownership model).thisAddressSanitizer: heap-use-after-freeinstd::__shared_ptr<SharedState>::get(), raised byConcurrentReadsDuringTeardownAreSafeManual GUI check
Toggle Use Mock Diagnostics repeatedly while messages arrive. Expected: no crash, hang, or stale callback update; the source label alternates between
MockandROS /diagnostics; mock buttons enable only in mock mode; live rows resume updating each time live mode is reselected.Scope
Diagnostics thread safety only. The IMU package is a separate PR from current
mainand is not stacked on this branch.