Conversation
The future shared state behind a GenericClient response is freed by whichever thread drops the last reference to it. When that is a gateway executor thread, ~_State_base and the _Result::_M_destroy() it runs happen inside librclcpp, which carries no TSan instrumentation: the operator delete is intercepted in the allocator, but the acquire on the shared_ptr refcount that orders it after the caller's future::get() is not, so TSan pairs the free with the caller's read of _Result::_M_error. Anchor the entry on the libstdc++ frame that performs that read. It is the only frame of the pair that is reliably symbolized: librclcpp is stripped, so the freeing frame carries no symbol name for a race: pattern to match.
… reason An aggregator answers a fanned-out listing 200 with no `partial` until a discovery pass has marked the peer healthy and published it as a contributor to the entity. Peer health does not imply the second: the two are set at opposite ends of the same pass, with the peer metadata fetch and its whole budget in between. Reading why a peer dropped out therefore waits for the fan-out to be in the answer rather than for health alone. The reason and shape assertions stay as they were, so a wrong reason or a reshaped failed_peers still fails; only the moment of reading moves.
DISCOVERY_TIMEOUT already carries the sanitizer time scale, so multiplying it by the scale again squared it: one wait became 540 s under a scale of 3 instead of 180 s. setUpClass runs three of those waits back to back, so the class asked for more wall clock than this test's 300 s ctest budget grants even after the sanitizer jobs stretch it.
| # DISCOVERY_TIMEOUT already carries the sanitizer time scale. Applying the scale | ||
| # again squares it, and setUpClass runs three of these waits back to back, so the | ||
| # class would ask for more wall clock than this test's ctest budget grants. | ||
| TIMEOUT = DISCOVERY_TIMEOUT |
There was a problem hiding this comment.
The squaring you removed here is still in three siblings: test_aggregation_time_budgets.test.py:114, test_relay_peer_credential.test.py:101 and test_aggregator_fault_stream.test.py:93 all do DISCOVERY_TIMEOUT * get_time_scale(). test_relay_peer_credential has no ctest override, so under TSan its _wait_for_gateways and _wait_for_peer_status deadlines are 540 s inside a 360 s ctest budget, and a stuck wait there gets killed with no test name in the output, the exact failure this comment says it avoids. Fix all four here or say why the other three stay out.
| # of _Result::_M_error. Anchored on the libstdc++ frame that performs that read: | ||
| # it is the only frame of the pair that is reliably symbolized, since librclcpp | ||
| # is stripped and the freeing frame prints as <null> at librclcpp.so+0x..., | ||
| # which no race: pattern can match. |
There was a problem hiding this comment.
This last clause is false: TSan matches race: patterns against function, file and module name, the module is filled in even when symbolization fails (that is where the (librclcpp.so+0x...) suffix comes from), and matching is an unanchored substring, so race:librclcpp.so would catch that <null> frame. The real reason to anchor on _M_get_result is scope: a module pattern suppresses every report with any librclcpp frame on either stack. Reword to that, and note the pattern also covers the gateway's own std::future<NodeQueryResult> in the configuration fan-out, so it is wider than the one GenericClient report.
…apper Two patterns anchor on the read side of the report, FutureAndRequestId::get(): the rclcpp one for Jazzy and newer, and the compat::GenericServiceClient one the gateway uses on Humble. The read is header code compiled into our binaries, so the frame is always symbolized, and the pattern matches only a future rclcpp handed us for a client request. The wrapper's destructor and wait_for stay unmatched. The comment blocks around it describe the synchronisation the way libstdc++ implements it: the setter runs under call_once and release-stores _M_status, wait() acquires the same atomic, and for GenericClient that setter chain is compiled into librclcpp, which is uninstrumented.
DISCOVERY_TIMEOUT already carries the sanitizer time scale. Seven feature tests multiplied it by the scale again, and test_aggregation_time_budgets scaled the already scaled GENEROUS_FORWARD_MS a second time in two request timeouts. Every budget is now scaled once. test_peer_failure_reasons: _fan_out_failure polls on TIMEOUT, retries a request that raised, keeps the 200 assertion on every answer, and names the last transport error when the answer never carries the fan-out.
The test starts three gateways and a fault_manager on three extra domains, waits for both aggregators, then waits a scaled 60 s for the fault_manager on the peer's domain, before the first case runs. That does not fit the 120 s feature default.
2 participants
Pull Request
Summary
The
sanitizer-tsanjob onmainis red on two tests that share nothing but the job.test_cancel_outcomespasses all 20 cases; the process exits non-zero because TSan reports one race. The last reference to aGenericClientfuture's shared state is dropped by the rclcpp executor thread, insidelibrclcpp, which is not instrumented. TSan sees theoperator deletethere but not the acquire on theshared_ptrrefcount that orders it after ourfuture::get(), so it pairs the free with the caller's earlier read of the result. Our use of the API is correct and the reference is held for the wholeget().tsan_suppressions.txtgets two lines for this class of report, both anchored on the read side, the wrapper'sget():rclcpp::detail::FutureAndRequestIdon Jazzy and newer, and the gateway'scompat::GenericServiceClienton Humble. The wrapper's destructor andwait_forare not matched, and nostd::futurethe gateway builds itself is matched. The comment blocks around the pattern describe the synchronisation the way libstdc++ implements it. Reproduced under TSan with a standalone reproducer against the real suppressions file: 5 of 5 runs reported the race with no suppression. Thesanitizer-tsanjob of this pull request checks the pattern against the real gateway.test_peer_failure_reasonsfailed on an assertion. Between the aggregator marking a peer online and publishing its contributor map for an entity, a read answers 200 with no fan-out at all, which is the documented purpose of the contributor map. The test's readiness gate waited for the first signal and read the failure reason from a single response, so it asserted a transient state; the window is about 0.9 s without instrumentation and wider under TSan._fan_out_failurenow polls until the answer carries the fan-out before reading the reason, retries a request that raised, and keeps every original assertion. With the readiness gate reduced to "the HTTP server answers", the old helper fails 5 of 5 runs with the exact CI message and the new one passes 5 of 5.Seven feature tests applied
get_time_scale()on top ofDISCOVERY_TIMEOUT, which already carries the scale, andtest_aggregation_time_budgetsscaledGENEROUS_FORWARD_MStwice in two request timeouts. Every budget now scales once, so the longest single wait under TSan is 180 s.test_relay_peer_credentialgets a 300 s ctest budget like the other multi-gateway tests; it starts three gateways and a fault manager and holds two sequential waits before its first case.Issue
No issue. Two independent failures of one CI job.
Type
Testing
test_cancel_outcomesandtest_peer_failure_reasonson jazzy, plain build: 10 of 10 and 10 of 10, plus the aggregation sweep (ctest -R "peer|aggregat", 11 of 11).test_1still fails the fixed test ('timeout' != 'unreachable').test_relay_peer_credentialpass on jazzy, plain build, with the new budgets in place; the integration package's linters pass.sanitizer-tsanjob of this pull request is the only place the suppression is verified against the real gateway under TSan.Checklist