On Windows, thread-local destruction holds the loader lock. Waiting for the event loop can deadlock if the loop is joining a worker that needs that lock to exit.
Queue client cleanup on the async cleanup thread, keeping its maps and mutex alive for disconnect callbacks. Also join workers off the event loop to keep it responsive during teardown.
Add portable tests for client exit, deferred cleanup races, and worker teardown.
Used in:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | ryanofsky |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Nce find. I wasn't aware of the windows loader lock. tl;dr: it is global lock held while threads start and exit and DLLs load, and also while thread_local destructors run.
Concept ACK on fixing the deadlock. Joining the worker on the async cleanup thread does break the cycle you describe, but I think there's another path to the same deadlock that it doesn't cover, and a client-side fix might handle both (see the end of this comment).
To check that I understand the bug: it's a cycle between three threads in the same process. An exiting client thread holds the windows loader lock while its thread_local ThreadContext destructor runs. The destructor calls EventLoop::sync(). The event loop is blocked in ~ProxyServer<Thread> joining a worker. And the worker can't finish exiting, because thread exit needs the windows loader lock. On Linux, exiting threads don't wait on a shared lock, so the join just completes. Is that right?
My main concern is that this fixes only one of the places where the event loop waits for another thread that needs the loader lock. makeThread and makePool also block the event loop, in thread_context.get_future().get(), until the new worker starts running, and a new thread can't start while another thread holds the loader lock (DLL_THREAD_ATTACH). So if one client thread is exiting and calling sync() while the event loop is handling another client thread's first IPC call, I think the same deadlock happens, with or without this PR.
Because of that, I think it would be better to fix this on the client side: the thread holding the loader lock is the one that shouldn't wait. This should also be easier to avoid breaking in the future. Our code only runs under the loader lock in a few places, mainly thread_local destructors like ~ThreadContext. But it can wait for something that needs the loader lock in many places: any time it starts or joins a thread, or waits for another thread that does. Avoiding waits in the first few places prevents the deadlock everywhere, including in code that hasn't been written yet. The alternative is checking every place the event loop could wait on a thread.
To implement this, ~ThreadContext could hand its request_threads and callback_threads entries to addAsyncCleanup() and return without waiting. The async thread would destroy them, and it can block in sync() safely because it doesn't hold the loader lock. Two things would need to change: addAsyncCleanup() currently has to be called from the event loop thread, and the disconnect callbacks registered in SetThread() refer to the map and mutex inside the ThreadContext, so these would need to live on the heap and be kept alive by the cleanup callback. This client side approach would also be what the page you linked recommends: not waiting on other threads while holding the loader lock.
Some other questions (if you know or have this information):
thread_local destructors from a TLS callback under the loader lock, MinGW builds might be affected too.sync() call after releasing its Thread::Client, because ~ProxyServer<Thread> only runs once the event loop processes the Release message. That seems to depend on timing. But it could also be a different worker: during a disconnect the event loop joins all of the connection's workers, so any IPC client thread exiting at the same time could deadlock. The second case seems more likely in practice, and it's what the portable test models.sync() again after the release, so it might only hang sometimes.
Add TestSetup::initAsyncCalls() to exchange thread maps and install a
no-op callFnAsync() handler. Use it in the existing worker-lifetime,
disconnect, and simultaneous-call tests without changing their hooks,
thread synchronization, or assertions.
This prepares reusable setup for the client-exit and worker-exit
regression tests in subsequent commits.
Introduce AsyncCleanupList to limit the changes to what the next commit
needs, without migrating the other CleanupList users.
Group the per-connection client maps in ThreadClients with a mutex
independent of the thread waiter. Update map users and the existing
thread-mapping test to use this state, still stored in ThreadContext.
Add ThreadClients::clear() to pin each event loop, extract its clients
under the map mutex, and destroy them on that loop after unlocking.
Use it during worker teardown so explicit release does not hold the
map mutex while waiting for another loop or destroying client objects.
Thread-exit cleanup remains synchronous in this preparatory change.
Allocate ThreadClients separately from ThreadContext without changing
its cleanup algorithm or synchronization. Disconnect callbacks can keep
referring to the maps and mutex when a later change defers destruction
beyond the lifetime of ThreadContext.
Update client-state accesses to use the pointer. Thread-exit cleanup
remains synchronous in this preparatory change.
Pin the event loop while queuing cleanup. Calls from another thread
notify an existing cleanup worker, or wake the event loop to start one.
Post only the first wakeup for a pending queue, so exiting threads
cannot fill the pipe while the loop is waiting for a worker to start.
Keep async-thread creation on the event loop, since callers can hold the
Windows loader lock. Test posting move-only callbacks from another
thread, both before and after the cleanup worker has started.
Windows ThreadContext destructors can run under the loader lock. Waiting
for an event loop from these destructors can deadlock if the loop starts
or joins a worker, which needs the loader lock to start or finish
exiting.
Hand the heap-owned ThreadClients to async cleanup without waiting for
an event loop or creating a thread. The maps and mutex remain valid for
disconnect callbacks until cleanup releases the clients on their
respective event loops.
Add portable coverage for client exit before connection closure, exit
with a blocked event loop, disconnect after thread exit while deferred
cleanup is held, and calls over multiple event loops.
Signal the worker to stop and queue its join on the existing async
cleanup thread. Retain the waiter until the worker has exited, since it
still uses the waiter's mutex and condition variable while returning
from Waiter::wait(). The event loop remains available for disconnects
and other requests during teardown, and still waits for queued cleanup
before exiting.
Add a worker-exit hook and a portable test holding a stopped worker
alive while disconnect completes. This covers event-loop responsiveness
separately from the preceding client-exit cleanup fix.
To check that I understand the bug
I think that's correct.
- Is this specific to MSVC
My agent confirmed with both MSVC 19.44 and MinGW GCC 15.2, native builds:
- Do you have a stack trace from sv2-tp#93
My agent produced and describes this cycle:
- Caller 6604 owns
LdrpLoaderLockand waits inEventLoop::sync()during TLS cleanup.- Event loop 17380 processes a Release and joins worker 15556.
- Worker 15556, named
…15556 (from …6604), waits insideLdrShutdownThread.
The extra
sync()comes from destroying the callback-thread client after the request-thread client was released. This observed hang involves the caller’s own worker.
- Did the first test hang reliably on MSVC without the fix?
Yes.
With MingW it's less reliable:
three hung, six crashed with
STATUS_HEAP_CORRUPTION, and one passed. The captured hang shows TLS cleanup waiting on the event loop, the loop joining a worker, and the worker stuck in Windows thread exit.
https://gist.github.com/Sjors/9b8695e98322f050d7fc21fd95bc4957
I think it would be better to fix this on the client side
I ended up boing both...
The tests here were run with a native compiler. Additionally, I tested (a fresh rebase) with a guix build of https://github.com/stratum-mining/sv2-tp/pull/93.