THRIFT-6244: Stop TNonblockingServerTest racing its own teardown - #3849
Merged
Conversation
Client: cpp bad_alloc_does_not_end_the_process failed intermittently on AppVeyor, 11 of 229 jobs since it was added, either crashing or hanging until the 300 s ctest timeout. Two independent races, one behind each symptom, so both are fixed here. The crash: the fixture stopped the server and joined the serve thread but never stopped the ThreadManager. Members are destroyed in reverse declaration order, so the server went before the thread manager, and ~TNonblockingServer deletes every TConnection and the IO threads along with their notification pipe. A pool task still unwinding from the injected failure then reached notifyIOThread() -- and close() -- on the TConnection it holds by raw pointer. The fixture destructor now stops the ThreadManager after joining the serve thread and before the members go; ThreadManager::stop() returns only once each worker has finished the task in its hands, so nothing is left running over the objects being destroyed. The hang: FailsFirstCallProcessor failed whichever call reached process() first, tracked in a plain bool that both pool workers read and wrote. When the call from canCommunicate() won that race it consumed the failure and got no reply, and its client had no receive timeout, so the test body blocked until ctest killed it. The flag is now claimed with an atomic exchange, the processor signals a Monitor as it claims it, and the test waits for that signal before opening the second connection -- so the failure is always taken by the call meant to receive it. The exchange matters on its own: read-then-write let two calls each see the flag unset and both throw, which is what the Windows logs showed as a doubled bad_alloc line. canCommunicate() also sets a 10 s receive timeout. Every call it makes is a localhost round-trip of a few bytes, so this only bounds the failure: a regression reports in seconds with the transport exception rather than as an opaque 300 s timeout. Verified by forcing each schedule rather than waiting for the race: - delaying the first arrival at process() so the second connection claims the failure reproduces the hang exactly; without the wait the case fails in 10 s with THRIFT_EAGAIN, with it the case passes. - holding the failing task in flight past the end of the test body puts ASan on the use-after-free with the ThreadManager stop removed -- notifyIOThread() reading a TConnection freed by ~TNonblockingServer() through Fixture::~Fixture() -- and reports nothing with it present. Then 5 clean runs of the full suite, and 200 runs of this case under ASan with eight in parallel. Not touched: allocation_failure_on_the_io_thread_does_not_end_the_process fails under ASan because its RLIMIT_AS child collides with ASan's own allocator. That reproduces identically on master. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
bad_alloc_does_not_end_the_processhas failed intermittently on AppVeyor since it was added — 11 of 229 jobs, and 11 of 39 builds went red because of it — either crashing (ctest reports SEGFAULT) or hanging until the 300 s ctest timeout. There are two independent races, one behind each symptom, so both are fixed here. Test-only; no library change.The crash
The fixture stopped the server and joined the serve thread, but never stopped the
ThreadManager. Members are destroyed in reverse declaration order, soservergoes beforethreadManager_, and~TNonblockingServerdeletes everyTConnectionand the IO threads along with their notification pipe. A pool task still unwinding from the injected failure then reachednotifyIOThread()— andclose()— on theTConnectionit holds by raw pointer.~Fixture()now stops theThreadManagerafter joining the serve thread and before the members go.ThreadManager::stop()returns only once each worker has finished the task in its hands, so nothing is left running over the objects about to be destroyed.The hang
FailsFirstCallProcessorfailed whichever call reachedprocess()first, tracked in a plainboolthat both pool workers read and wrote. When the call fromcanCommunicate()won that race it consumed the failure and got no reply — and its client had no receive timeout, so the test body blocked until ctest killed it.The flag is now claimed with an atomic
exchange, the processor signals aMonitoras it claims it, and the test waits for that signal before opening the second connection, so the failure is always taken by the call meant to receive it. Theexchangematters on its own: read-then-write let two calls each see the flag unset and both throw, which is what the Windows logs showed as a doubledbad_allocline.canCommunicate()also gets a 10 s receive timeout. Every call it makes is a localhost round-trip of a few bytes, so this only bounds the failure mode: a regression now reports in seconds with the transport exception instead of as an opaque 300 s timeout.Verification
Rather than waiting on a race that is roughly 5 % on AppVeyor and 2 in 1200 here, each schedule was forced:
The hang — delaying the first arrival at
process()so the second connection claims the failure reproduces it exactly. Without the wait the case fails in 10 s withTHRIFT_EAGAIN (timed out); with it, the case passes.The crash — holding the failing task in flight past the end of the test body puts ASan straight on the use-after-free once the
ThreadManagerstop is removed:With the stop in place ASan reports nothing.
Then 5 clean runs of the full suite, and 200 runs of this case under ASan with eight in parallel.
Not touched
allocation_failure_on_the_io_thread_does_not_end_the_processfails under ASan because itsRLIMIT_ASchild collides with ASan's own allocator. That reproduces identically on unmodified master and is unrelated to this change.🤖 Generated with Claude Code