Skip to content

Make diagnostics mock/live source handoff thread safe - #10

Open
khuzaymahbinharis-jpg wants to merge 1 commit into
mainfrom
fix/diagnostics-source-handoff
Open

Make diagnostics mock/live source handoff thread safe#10
khuzaymahbinharis-jpg wants to merge 1 commit into
mainfrom
fix/diagnostics-source-handoff

Conversation

@khuzaymahbinharis-jpg

Copy link
Copy Markdown
Contributor

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 RosDiagnosticsSource while 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

Area Change
RosDiagnosticsSource Received state moved into a separately owned SharedState block; the callback captures it by shared_ptr instead of capturing this
DiagnosticsSource New virtual stop() hook (no-op by default) so the panel can retire a source without RTTI checks
RosDiagnosticsSource::stop() Drops the subscription handle, then retires the state under its mutex; idempotent and also called from the destructor
DiagnosticsPanel::configureSource() Retires the outgoing source before installing the replacement, so no second subscription ever coexists with the old one
DiagnosticsPanel::refresh() Pins the source shared_ptr for the whole tick, so a swap mid-tick cannot leave widgets reading a half-replaced source
~DiagnosticsPanel() Stops the refresh timer first, then the source

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.md documents the ownership rules and the test procedure.

Repeatable stress check

test/test_ros_diagnostics_source.cpp is the automated equivalent of toggling the checkbox repeatedly while the cycle publisher runs. It spins a 4-thread MultiThreadedExecutor while a separate thread publishes every 200 microseconds, then creates, reads, stops, and destroys live sources in a loop.

Test Covers
ReportsWaitingBeforeAnyMessageArrives Waiting row before any traffic
NormalizesReceivedDiagnostics Status/value/unit mapping
StopFreezesStateAndIgnoresLaterMessages Retired source stops ingesting
StopIsIdempotent Repeated stop() plus later reads
RepeatedLiveMockChurnUnderTrafficIsSafe 300 create/stop/destroy cycles under load
DestructionWithoutExplicitStopIsSafe Destructor alone retires the subscription
ConcurrentReadsDuringTeardownAreSafe Reader thread racing stop()
ChurnLeavesNoLingeringSubscription No duplicate/leaked subscription

The 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)

colcon build --packages-select waybionic_rviz_plugins --symlink-install
colcon test --packages-select waybionic_rviz_plugins
colcon test-result --all --verbose
100% tests passed, 0 tests failed out of 4
Summary: 24 tests, 0 errors, 0 failures, 0 skipped

Breakdown: lint_cmake 1, xmllint 2, test_package_metadata 9, test_ros_diagnostics_source 8.

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.cpp were compiled standalone twice — once against this branch, once against a mutated copy whose callback captures this and whose stop() does not drop the subscription (the pre-fix ownership model).

Build Result
This branch 4 stress tests pass
Mutated to capture this AddressSanitizer: heap-use-after-free in std::__shared_ptr<SharedState>::get(), raised by ConcurrentReadsDuringTeardownAreSafe

Manual GUI check

# Terminal 1
ros2 launch waybionic_rviz_plugins temporary_diagnostics_publisher.launch.py mode:=cycle

# Terminal 2
ros2 launch waybionic_rviz_plugins engineer_view.launch.py use_mock_diagnostics:=false

Toggle Use Mock Diagnostics repeatedly while messages arrive. Expected: no crash, hang, or stale callback update; the source label alternates between Mock and ROS /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 main and is not stacked on this branch.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant