Fix Windows shutdown deadlock and leak with the Ruy backend - #2076
Fix Windows shutdown deadlock and leak with the Ruy backend#2076timo9378 wants to merge 4 commits into
Conversation
The per-thread ruy::Context in src/cpu/backend.cc is a static thread_local whose destructor joins Ruy's internal thread pool. On Windows that join runs while the worker thread is exiting (under the loader lock) and deadlocks ThreadPool shutdown; ThreadPool::~ThreadPool then blocks forever in worker->join(). Destroy the context explicitly from ReplicaWorker::finalize() via destroy_context() instead. finalize() runs on the worker thread in a normal context (not thread exit), so the join completes and the context is freed rather than leaked. Refs jkawamoto/ctranslate2-rs#64
|
Hi! Any updates on this? I'd love to get this merged when you have a chance to review. Thanks! |
Per review, restrict the ruy::Context lifetime change to _WIN32 so Linux and macOS keep the original thread_local RAII unchanged. backend.cc uses the heap-pointer + finalize() cleanup only under _WIN32 (the #else restores the original thread_local context); backend.h declares clear_ruy_context() under _WIN32; devices.cc guards the call with defined(CT2_WITH_RUY) && defined(_WIN32). Add test_shutdown_does_not_deadlock: it runs a CPU int8 Translator through a large batch in a subprocess with a timeout so the shutdown hang fails cleanly instead of blocking the runner. The batch is large on purpose -- the deadlock only triggers once Ruy has spawned its thread pool, which needs a big enough GEMM.
tools/ruy_shutdown_repro builds a CPU int8 Translator, runs a large batch, and destroys it. Unpatched it deadlocks on shutdown in both static-CRT (/MT) and shared (/MD) builds; with this fix both exit cleanly.
|
Thanks, that makes sense. Scoping it to Windows keeps the risk off Linux and macOS, so I pushed an update. One thing I ran into while doing it. Guarding just the clear_ruy_context() call in devices.cc isn't quite enough on its own. If backend.cc still hands out a heap allocated context on every platform but the cleanup only compiles on Windows, then on Linux and macOS the context gets new'd and never freed, since the raw thread_local pointer doesn't destroy anything, so it would turn today's clean RAII into a leak. So I put the _WIN32 split in backend.cc too. Non Windows keeps the original static thread_local ruy::Context, and only Windows uses the heap pointer with the finalize() cleanup. The header declares clear_ruy_context() under _WIN32 only, and devices.cc guards the call with defined(CT2_WITH_RUY) && defined(_WIN32) like you suggested. Off Windows it is a real no op versus what ships today. For the test I added test_shutdown_does_not_deadlock in test_translator.py. It builds a CPU int8 Translator, runs a translation, and destroys it, all inside subprocess.run with a timeout so a hang fails cleanly instead of blocking the runner. It is marked skipif not win32 since the hang is Windows only. One detail that is easy to miss and worth spelling out. The test only reproduces if the batch is large enough that Ruy actually spawns its thread pool. With a single short sentence Ruy stays single threaded, there are no threads to join at shutdown, and the process exits cleanly even without the fix, so a naive test passes whether or not the bug is present. The test uses a 512 sentence batch so the pool is really up. I checked it both ways against a from source Ruy build on Windows: unpatched it times out and fails, with this PR it passes in well under a second. I also confirmed this is not specific to the static CRT. A shared /MD build, same config as the released wheels, hangs on shutdown too once the batch is big enough, so the wheels are affected as well. I dropped a small standalone C++ repro under tools/ruy_shutdown_repro that shows this outside the test suite: same source built once with /MT and once with /MD, both hang unpatched and both exit cleanly with this PR. Happy to move or drop it if you would rather not carry it in the tree. |
Hi, thanks for CTranslate2. I ran into a Windows-only shutdown hang and tracked it down
to the Ruy backend, so here's a fix.
Creating and destroying a
Translator(orGenerator/Whisper) with the Ruybackend on Windows hangs the process on shutdown. The culprit is the per-thread
ruy::Contextinsrc/cpu/backend.cc:It's constructed lazily on a worker thread the first time that thread runs a GEMM,
and its destructor joins Ruy's internal thread pool. Because it's
thread_local, thatdestructor runs while the worker thread is exiting, under the Windows loader lock, and
the join deadlocks.
ThreadPool::~ThreadPoolinsrc/thread_pool.ccthen blocksforever in
worker->join().Sequence when a model is destroyed:
~ReplicaPool→~ThreadPoolcloses the queue and callsworker->join()run()loop returns and the thread begins to terminatethread_localdestructors run, so~ruy::Contextjoins the Ruy threadsThis only happens on CPU. On CUDA the GEMM never calls
get_ruy_context(), so theworker thread has nothing to join and exits cleanly. That matches the reports that GPU
builds don't hang while CPU builds do.
The fix is to destroy the context from
ReplicaWorker::finalize()instead.finalize()runs on the worker thread inside
run(), before the thread exits, so the join happensin a normal context (no loader lock) and completes. It already calls
destroy_context(device)for this kind of per-thread cleanup (freeing curand state onCUDA today), so the Ruy cleanup goes in the same place:
backend.cc: make the context a resettable heap pointer and addclear_ruy_context()backend.h: declare itdevices.cc: callcpu::clear_ruy_context()fromdestroy_context()forDevice::CPUEverything stays under the existing
CT2_WITH_RUY/CT2_WITH_CUDAguards, so otherbackends and platforms are untouched.
Besides fixing the hang, this frees the context instead of leaking it. RSS after each
of 5 load/translate/unload cycles (Windows, MSVC 14.44, x64, int8 NLLB-200 600M, Ruy):
ruy::ContextThe ~250 MB/cycle in the middle row is Ruy's prepacked cache, which the leak keeps
alive; finalize-clear releases it. Translation output is unchanged and identical across
repeated calls, so rebuilding the context after clearing it is fine.
Found via jkawamoto/ctranslate2-rs#64, where the hang was worked around by skipping the
model's destructor entirely (avoiding the hang but leaking the model). With this change
the destructor can run.
Happy to adjust the approach if you'd prefer the cleanup somewhere else. Thanks!