Skip to content

Fix ConnectionPool deadlock from reentrant _optional_thread_lock - #1108

Open
n1ck-04 wants to merge 1 commit into
encode:masterfrom
n1ck-04:fix/connection-pool-reentrant-lock-deadlock
Open

Fix ConnectionPool deadlock from reentrant _optional_thread_lock#1108
n1ck-04 wants to merge 1 commit into
encode:masterfrom
n1ck-04:fix/connection-pool-reentrant-lock-deadlock

Conversation

@n1ck-04

@n1ck-04 n1ck-04 commented Sep 8, 2026

Copy link
Copy Markdown

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 throwing GeneratorExit at its suspension point. That lands in the except BaseException handler in PoolByteStream.__iter__, which calls self.close() -> re-acquires ConnectionPool._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_lock is already held (see handle_request() and PoolByteStream.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 on httpx/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; AsyncThreadLock is already a no-op, so this can't happen in async mode) now uses threading.RLock instead of threading.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.
  • Making the lock reentrant exposes a second, previously-impossible failure mode: a reentrant call into _assign_requests_to_connections() can remove a connection from self._connections before the outer, suspended call gets back around to removing the same connection, which would raise ValueError: list.remove(x): x not in list. I guarded the three list.remove(connection) call sites in that method against this. Without this, swapping to RLock alone 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

  • Added tests/test_synchronization.py: direct unit tests of ThreadLock's reentrancy contract (same thread can re-acquire; a second thread still blocks correctly).
  • Added 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's is_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 bounded thread.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 that httpcore/_sync/* and tests/_sync/* are generated from httpcore/_async/* and tests/_async/* via scripts/unasync.py, so the actual fix lives in httpcore/_async/connection_pool.py and was regenerated into _sync with that script.
  • Ran scripts/check (ruff format/check, mypy strict, unasync.py --check) and the full test suite locally: 217 passed, 100% coverage maintained.

Checklist

🤖 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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants