Jens-G opened a new pull request, #3849:
URL: https://github.com/apache/thrift/pull/3849
`bad_alloc_does_not_end_the_process` has 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, so `server` goes before `threadManager_`, 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.
`~Fixture()` 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 about to be 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 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 with `THRIFT_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 `ThreadManager`
stop is removed:
```
ERROR: AddressSanitizer: heap-use-after-free ... READ of size 8
#0 TConnection::notifyIOThread() TNonblockingServer.cpp:302
#1 TConnection::Task::run() TNonblockingServer.cpp:376
freed by thread T0 here:
... ~TNonblockingServer() TNonblockingServer.cpp:964
... Fixture::~Fixture()
```
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_process` fails under
ASan because its `RLIMIT_AS` child collides with ASan's own allocator. That
reproduces identically on unmodified master and is unrelated to this change.
---
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
