Fix ConnectionPool deadlock from reentrant _optional_thread_lock - #1108
Open
n1ck-04 wants to merge 1 commit into
Open
Fix ConnectionPool deadlock from reentrant _optional_thread_lock#1108n1ck-04 wants to merge 1 commit into
n1ck-04 wants to merge 1 commit into
Conversation
PoolByteStream.__iter__ is a generator. If it is abandoned mid-iteration (the last reference to a response/stream is dropped without reading it to completion or calling .close()), CPython finalizes the generator by throwing GeneratorExit at its suspension point, which lands in PoolByteStream.__iter__'s `except BaseException` handler and calls self.close() -> re-acquires ConnectionPool._optional_thread_lock. In production this finalization is triggered by CPython's garbage collector, which can run at essentially any allocation point, including from inside _assign_requests_to_connections() -- which only ever runs while _optional_thread_lock is already held. If that happens on the same thread, the plain (non-reentrant) lock deadlocks permanently. Reported independently by multiple users of multithreaded servers (frequently via the OpenAI SDK, which sits on httpx/httpcore) in encode#990. This makes ThreadLock's underlying lock a threading.RLock so the same thread can safely re-enter it, and hardens the three list.remove(connection) call sites in _assign_requests_to_connections() that would otherwise raise ValueError if a reentrant call had already removed the same connection -- a state-corruption risk that a bare Lock -> RLock swap would introduce silently. This is sync-only: AsyncThreadLock is already a no-op, so no equivalent scenario exists in async mode. Adds a deterministic regression test (no reliance on GC timing) that reproduces the original deadlock by simulating the exact reentrant interleaving, plus direct unit tests of ThreadLock's reentrancy contract. Fixes encode#990 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnkMYMmQtTsupovqXuxDaw
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.
Summary
Fixes the reentrant-lock deadlock described in #990 (and the underlying cause behind #1003, #1026, and the OpenAI-SDK-adjacent reports linked from there).
Root cause:
PoolByteStream.__iter__is a generator. If it's abandoned mid-iteration (the last reference to a response/stream is dropped without reading it to completion or calling.close()), CPython finalizes the generator by throwingGeneratorExitat its suspension point. That lands in theexcept BaseExceptionhandler inPoolByteStream.__iter__, which callsself.close()-> re-acquiresConnectionPool._optional_thread_lock.In the production reports linked from #990, this finalization is triggered by CPython's garbage collector, which can run at essentially any allocation point — including from inside
_assign_requests_to_connections(), which only ever runs while_optional_thread_lockis already held (seehandle_request()andPoolByteStream.close()itself). If the GC-triggered finalization happens to fire on the same thread that's already inside that method, the thread deadlocks trying to re-acquire its own lock, permanently — matching the exact signature independently reported by five different users on #990, most of them via multithreaded servers calling the OpenAI SDK (which sits onhttpx/httpcore).I reproduced this deterministically against current
master(no reliance on GC timing — see the added test) before making any change, to confirm the mechanism rather than just the symptom.Fix:
ThreadLock(sync-only;AsyncThreadLockis already a no-op, so this can't happen in async mode) now usesthreading.RLockinstead ofthreading.Lock, so the same thread can safely re-enter it. The docstring explains exactly why, since Change ThreadLock to ThreadRLock to resolve rare deadlock #1003 stalled on a maintainer asking for that reasoning._assign_requests_to_connections()can remove a connection fromself._connectionsbefore the outer, suspended call gets back around to removing the same connection, which would raiseValueError: list.remove(x): x not in list. I guarded the threelist.remove(connection)call sites in that method against this. Without this, swapping toRLockalone would trade a deadlock for an intermittent crash.I intentionally did not touch anything beyond
_assign_requests_to_connections()and the lock itself — no refactor of the surrounding pool logic.Testing performed
tests/test_synchronization.py: direct unit tests ofThreadLock's reentrancy contract (same thread can re-acquire; a second thread still blocks correctly).tests/_sync/test_connection_pool.py::test_connection_pool_reentrant_close_does_not_deadlock: an end-to-end regression test that reproduces the original deadlock deterministically (monkeypatches one connection'sis_idle()— a method_assign_requests_to_connections()calls while holding the lock — to drop the last reference to an abandoned stream at exactly that point, simulating what the GC does non-deterministically in production). Guarded with a boundedthread.join(timeout=5)so a future regression fails the test instead of hanging CI. This test is sync-only since the scenario is impossible in async mode; note thathttpcore/_sync/*andtests/_sync/*are generated fromhttpcore/_async/*andtests/_async/*viascripts/unasync.py, so the actual fix lives inhttpcore/_async/connection_pool.pyand was regenerated into_syncwith that script.scripts/check(ruff format/check, mypy strict,unasync.py --check) and the full test suite locally: 217 passed, 100% coverage maintained.Checklist
ThreadLockdocstring and this description instead.🤖 I used Claude Code to help investigate and implement this fix — the reproduction script, root-cause tracing through the actual call stack in #1003, and the guarded-removal fix were all verified against a live checkout and the project's own test/lint/unasync gates before opening this PR, not taken on faith from the AI's output.