Fix: reap test_remote_endpoint's server threads on the unwind path - #1578
Conversation
RemoteSocketTransport tests start a helper server thread, then construct a
RemoteL3SocketTransport whose constructor throws on a connect or HELLO
timeout — routine on a loaded box. The unwind destroyed the local
std::thread while it was still joinable, so std::terminate aborted the whole
binary mid-suite:
hw-native-sys#5 std::terminate ()
hw-native-sys#6 std::thread::~thread() ()
hw-native-sys#7 RemoteSocketTransport_ClosedPeerWriteDoesNotRaiseSigpipe_Test::TestBody()
Reproduced at 2/96 with 32 concurrent copies on a loaded 320-core box; the
run that aborts takes every other case in the binary with it, and the
message ("terminate called without an active exception", printed because no
handler has caught the in-flight exception yet) reads like a hang rather
than a timeout.
start_stalling_server also captured the test's stack std::atomic<bool> stop
flag by reference, so the same unwind left a running thread polling a dead
object.
ScopedServerThread owns both the thread and the stop flag, and joins in its
destructor, which closes both holes: the flag now outlives the thread, and
no unwind can reach a joinable std::thread.
Joining alone would swap the abort for a hang, because a thread parked in
accept() never returns when the client's connect is exactly what failed.
accept_until_stop polls the listener in 20 ms slices and rechecks the stop
flag, so stop_and_join always makes progress. It reads the flag only after
a poll slice, so a connection already pending still wins:
ClosedPeerWriteDoesNotRaiseSigpipe calls stop_and_join right after a
successful connect and depends on that connection being accepted and RST.
Add ServerThreadIsJoinedWhenTestBodyUnwinds, which throws out of a scope
holding a live server thread that never sees a client. It covers both
halves — reverted to the old bare-std::thread idiom it aborts with the
production message, and a join that could not reap a parked accept() would
trip its elapsed bound instead.
ut-cpp 65/65 green; 192 concurrent runs of the binary now abort 0 times.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
RemoteSocketTransporttests start a helper server thread, then construct aRemoteL3SocketTransportwhose constructor throws on a connect or HELLO timeout — routine on a loaded box. The unwind destroyed the localstd::threadwhile it was still joinable, sostd::terminateaborted the whole binary mid-suite, taking every other case in it down too.Core dump from a reproduced abort:
Reproduced at 2/96 with 32 concurrent copies of the binary on a loaded box. The printed message —
terminate called without an active exception— is misleading: the exception is in flight, but no handler has caught it yet, so libstdc++'s verbose terminate handler reports no active exception. It reads like a hang rather than a timeout, which is part of why this looked like unexplained flakiness.A second, latent bug in the same code:
start_stalling_servercaptured the test's stackstd::atomic<bool> stopby reference, so the same unwind left a running thread polling a destroyed object.Fix
ScopedServerThreadowns both the thread and the stop flag and joins in its destructor. The flag moving inside is what fixes the dangling reference; joining from the destructor is what stops an unwind ever reaching a joinablestd::thread. It is declared before the transport in each test, so the transport is destroyed first.accept_until_stopreplaces the bare blockingaccept()in all three server helpers. Joining alone would have swapped the abort for a hang: a thread parked inaccept()never returns when the client's connect is exactly what failed. It polls the listener in 20 ms slices and rechecks the stop flag, sostop_and_joinalways makes progress, with a 10 s cap.Ordering inside that loop is load-bearing: the stop flag is read only after a poll slice, so a connection already pending still wins.
ClosedPeerWriteDoesNotRaiseSigpipecallsstop_and_joinimmediately after a successful connect and depends on that connection being accepted and RST — reading the flag first would have introduced a fresh flake.Regression barrier
ServerThreadIsJoinedWhenTestBodyUnwindsthrows out of a scope holding a live server thread that never sees a client, covering both halves:std::threadidiom it aborts with the production message (verified).accept()would trip its elapsed bound rather than hang silently.No timing bound was widened. The 1.0 s attach timeout in the aborting test is incidental to what it asserts, but raising it would only paper over the crash; the two tests where a bound is the subject (
HelloReadBoundedByAttachTimeout,RuntimeWriteToStalledReaderTimesOut) are untouched.Testing
ctest -LE requires_hardware— 66/66 passtest_remote_endpointpost-fix: 0 aborts, 0 failures (was 2/96)