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:
kj::Executor::executeSync by xyzconstant)disconnect and waitDrained methods by ryanofsky)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.
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.
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.
I wrote:
With MingW it's less reliable:
three hung, six crashed with
STATUS_HEAP_CORRUPTION, and one passed.
Note that this PR does not fix the heap corruption issue. I only happens with native builds, and I think our priority should be the cross-compiled builds. However, the issue creates confusion when testing bugs, so I might open a separate PR for it.
99 | + EventLoop& loop = *threads->begin()->second->m_context.loop; 100 | + loop.addAsyncCleanup([clients = std::move(clients)]() mutable { 101 | + clients->clear(); 102 | + clients.reset(); 103 | + }); 104 | +}
In commit "proxy: defer thread-local client cleanup" (90c1ae7b602c407f56bbd2ff61cc5fc12a7be9c6)
I think this would be simpler, and avoid some ordering dependencies between event loops, if it queued cleanup for each map entry on that entry's own event loop instead of queuing everything on the first entry's loop.
These destructors run on threads libmultiprocess doesn't control, which can make calls over any number of connections and event loops. With the current approach, all of the thread's clients are released by one loop's async thread, which calls sync() into the other loops. So that loop's other async cleanup (m_impl destruction, worker joins) waits on the other loops, and the other loops can't shut down until that loop's async thread gets to the callback holding their clients. I don't think this causes a problem today, but it's an assumption that isn't needed.
Queuing per entry would also remove the empty() checks, the choice of loop, and moving clients while its mutex is locked. Something like (untested):
ThreadContext::~ThreadContext()
{
// The Windows loader lock may be held here. Queue cleanup for each client
// on its own event loop without waiting for the loop or for a thread to
// start. Disconnect callbacks keep referring to the shared maps and mutex.
std::shared_ptr<ThreadClients> shared{std::move(clients)};
const Lock lock(shared->mutex);
for (ConnThreads* threads : {&shared->request_threads, &shared->callback_threads}) {
for (const auto& [connection, client] : *threads) {
EventLoop& loop = *client->m_context.loop;
loop.addAsyncCleanup([&loop, shared, threads, connection = connection] {
loop.sync([&] { shared->release(*threads, connection); });
});
}
}
}
where ThreadClients::release() extracts the entry for connection under the mutex, if a disconnect hasn't already removed it, and destroys it after unlocking. That's what the SetThread disconnect callback does after 9783fb2eb23d316842eb47bea9a394986304e276 from #361, so on top of that commit the two could share the helper. clients would need to be a std::shared_ptr since several callbacks share it.
This would also replace the synchronous ~ThreadContext that commit adds. Its race between the exiting thread and the disconnect callback can't happen once every entry is released on its loop thread, since disconnect callbacks run on that thread too. Its regression test, "Thread exiting while its connection is destroyed", would need updating, since its hook blocks the exiting thread inside ~ProxyClient<Thread>, which would no longer run on that thread.
81 | + } else { 82 | + ++it; 83 | + } 84 | + } 85 | + return removed; 86 | +}
In commit "proxy: centralize explicit thread-client release" (0b12fe7cafe8c23b100ccacd143e1ef29549f3fc)
Took me a moment to see that this handles clients on multiple event loops. pinLoop() returns one loop, and the while repeats until extractClients() has removed every loop's entries. It would be simpler to release one entry per sync() call, keyed by connection, instead of batching by loop. Something like (untested):
void ThreadClients::clear()
{
for (ConnThreads* threads : {&request_threads, &callback_threads}) {
for (;;) {
Connection* connection;
std::optional<EventLoopRef> loop;
{
const Lock lock(mutex);
if (threads->empty()) break;
connection = threads->begin()->first;
// Pin the loop before a disconnect can erase its client.
loop.emplace(*threads->begin()->second->m_context.loop);
}
(*loop)->sync([&] { release(*threads, connection); });
}
}
}
void ThreadClients::release(ConnThreads& threads, Connection* connection)
{
ConnThreads::node_type removed;
const Lock lock(mutex);
// A disconnect may have removed the entry already. Destroy it after unlocking.
removed = threads.extract(connection);
}
The same release() could be used by deferred cleanup in ~ThreadContext (see the comment there), so each client would be released on its own loop. After rebasing on #361 it would also be the body of the SetThread disconnect callback, which 9783fb2eb23d316842eb47bea9a394986304e276 changes to look up entries by connection instead of by iterator.
323 | + // fill the pipe while the loop is waiting for a worker to start. 324 | + Unlock(lock, [&] { 325 | + char buffer = 0; 326 | + m_post_writer->write(&buffer, 1); 327 | + }); 328 | + }
In commit "proxy: allow async cleanup from other threads" (9ad7c94b89d1ff4f03e5bb2e7e6d8449aec776c5)
Possible simplification, either here or in a follow-up: most of the complexity in this commit comes from the async thread being started lazily, while only the event loop thread is allowed to start it. That's why there are three cases here, the first-item wakeup flag, the EventLoopRef keeping the pipe writer alive, and the startAsyncThread() call on every loop() wakeup (which also wakes the async thread on every sync() call when it is already running).
If loop() started the async thread when it starts, this function could just add to the list and notify from any thread, with no wakeup path through the pipe. The cost is an idle thread per event loop, which seems fine since there is normally one loop per process. Shutdown wouldn't need to change, since the async thread already exits when m_async_fns is reset. It would also mean no thread is ever created while a client thread might be holding the loader lock.
This also seems relevant to #376, which removes the pipe this commit uses to wake the loop, so whichever PR is merged second would otherwise have to redo the wakeup. I think doing it here would make this PR smaller, but I'm also fine with it being a follow-up.
500 | + 501 | +KJ_TEST("Client thread exits before its connection closes") 502 | +{ 503 | + // Keep the connection open while a caller exits and releases its thread 504 | + // handles, then verify another caller can use it. The next test covers 505 | + // blocked event loops deterministically without platform-specific timing.
re: #368 (comment)
In commit "proxy: defer thread-local client cleanup" (90c1ae7b602c407f56bbd2ff61cc5fc12a7be9c6)
You mentioned 6 of 10 MinGW runs of this test crashed with STATUS_HEAP_CORRUPTION before the fix. Do those go away with this PR, or do they still happen? If they still happen, it seems like a separate bug that would be good to track in an issue, since it would also affect MinGW builds of Bitcoin Core once multiprocess works on Windows.
re: #368 (review)
Sjors actually answered before I asked this, this PR does not fix the heap corruption: #368 (comment)
622 | + if (const char* const* tag{std::any_cast<const char*>(&arg)}; 623 | + tag && std::string_view{*tag} == "worker thread exit") { 624 | + worker_stopping.set_value(); 625 | + release_worker_future.wait(); 626 | + } 627 | + };
In commit "proxy: join workers off the event loop" (b89d6bd27a4fd94c79f192a7b532f55f089082f7)
Two small things in this test:
worker_stopping.set_value() would throw on the worker thread, and terminate, if the hook ever ran twice, for example if the test is changed to create a second worker. A bool or std::once_flag guard would make the hook safer to reuse.loop_ref needed? I think the client Connection already keeps the loop alive until setup is destroyed. If there's a reason for it, a short comment would help.544 | - m_thread.join(); 545 | + // Retain the waiter until join completes because the worker still uses 546 | + // its mutex and condition variable while returning from Waiter::wait(). 547 | + m_loop->addAsyncCleanup([thread = std::move(m_thread), waiter = std::move(waiter)]() mutable { 548 | + thread.join(); 549 | + });
In commit "proxy: join workers off the event loop" (b89d6bd27a4fd94c79f192a7b532f55f089082f7)
Talked about this offline, and it was unclear if MSVC test or sv2-tp still hang without this commit. But it will also be a little hard to test at least in short term, so this is not too important.
As I understand it, after "proxy: defer thread-local client cleanup" the thread holding the loader lock doesn't wait for the event loop anymore, so joining the worker here shouldn't be able to deadlock. The comment above mentions a deadlock "if a destructor needs the loop", but the worker's own ~ThreadContext returns early because clients->clear() already emptied its maps, so I think that would only apply to application thread_local destructors on worker threads making IPC calls.
If that's right, this commit looks like a separate improvement (keeping the event loop responsive while a worker exits) that only depends on "proxy: allow move-only async cleanup callbacks". So it might be good to consider moving it and its test to a follow-up PR to keep this PR focused on the deadlock fix, and let the new ordering interactions, with worker joins now sharing the async cleanup queue with m_impl destructors and client cleanup, be reviewed on their own. Or if there still is a hang without this commit, it'd be good understand what path it was, since that would mean the client-side fix misses something.
In commit "proxy: allow async cleanup from other threads" (9ad7c94b89d1ff4f03e5bb2e7e6d8449aec776c5)
The comment at the top of addAsyncCleanup() still says "This function is called in ProxyServerBase destructors" and explains the list order in those terms. With this PR it's also called from ~ThreadContext on other threads and from ~ProxyServer<Thread> for worker joins, so those now run in FIFO order with the m_impl cleanups. I don't think the interleaving matters, since neither new kind of callback destroys server objects, but it'd be good for the comment to mention the other callers.
Code review b89d6bd27a4fd94c79f192a7b532f55f089082f7. Left some LLM assisted review comments below. My main feedback is I think this would be good to base on top of 9783fb2eb23d316842eb47bea9a394986304e276 from #361 and probably merge #361 before this. Also I think it would be good if cleanup code could be simplified to just loop over individual map entries instead of trying to batch them.
re: #368 (comment)
Thanks for checking all of this, and for the stack traces. Keeping both fixes makes sense to me. With the client-side change, the thread holding the loader lock no longer waits for anything, which also covers the makeThread start-up wait I was worried about. I left a question below about whether the last commit is still needed for the deadlock.
I reviewed the new commits and they look right to me. I left a few small comments below, plus a question about the MinGW heap corruption crashes.
One request about ordering: could you rebase this on #361, or cherry-pick its commit "proxy: Fix thread map teardown race causing use-after-free on disconnect" (9783fb2eb23d316842eb47bea9a394986304e276)? That commit reworks the same code as this PR. It adds a ~ThreadContext and changes the SetThread disconnect callback to find its map entry by connection, take it out of the map under the mutex, and destroy it after unlocking. That's the same operation this PR needs for deferred cleanup, so I think this PR gets simpler on top of it (see the suggestions on ~ThreadContext and ThreadClients::clear() below). It also fixes the teardown race you hit in sv2-tp. Since Windows isn't supported yet (#231), I'd like to get #361 merged first and base this on it.
would be good to base on top of https://github.com/bitcoin-core/libmultiprocess/commit/9783fb2eb23d316842eb47bea9a394986304e276 from #361 and probably merge #361 before this
I'll mark this as draft until then. Will address the other feedback after 361 lands, unless it's useful for reviewing that PR.