Skip to content

Serialize the HTTP/2 send path under a dedicated lock (thread-safety race) - #1118

Open
Dextheking1 wants to merge 4 commits into
encode:masterfrom
Dextheking1:fix/h2-thread-safety-3566
Open

Dextheking1 wants to merge 4 commits into
encode:masterfrom
Dextheking1:fix/h2-thread-safety-3566

Conversation

@Dextheking1

Copy link
Copy Markdown

Closes encode/httpx#3566

Root cause

httpx.Client(http2=True) shares a single connection across threads (HTTP/2 multiplexing). That connection is httpcore.HTTP2Connection (plus the async twin), which wraps one shared h2.H2Connection state machine. The receive path (_read_lock) and socket writes (_write_lock) were already serialized, but the send path was not: concurrent threads could interleave

  • get_next_available_stream_id() → duplicate stream IDs handed out (StreamIDTooLowError: 2411 is lower than 2413),
  • send_headers() → HPACK encode iterating dynamic_entries while another thread mutates it (RuntimeError: deque mutated during iteration — the first traceback in the linked issue),
  • open_outbound_streams iterating h2.streams while another thread adds/removes streams (dictionary changed size during iteration, KeyError).

h2's docs require get_next_available_stream_id() to be immediately followed by the matching send_headers() — impossible under concurrency without a lock. Serializing in httpx would kill multiplexing, so the fix has to live here.

The fix

One new lock, _send_lock (threading.Lock / anyio.AsyncLock), held in handle_request around stream-ID allocation, _events[stream_id] registration, _send_request_headers, and _send_request_body. The NoAvailableStreamIDError → ConnectionNotAvailable handling and the except BaseException cleanup (_response_closed, semaphore release) keep their exact original semantics — the send block simply moved inside the existing cleanup try.

Lock-order audit: _send_lock is acquired only in handle_request, outermost relative to _read_lock/_write_lock/_state_lock; the max-streams semaphore is acquired before it and its holder never waits on a waiter. No new deadlock.

Evidence

Fail-before / pass-after with an in-memory fake HTTP/2 transport (real client- and server-side h2 state machines, server-side guarded by its own lock so only client-side httpcore code is under test):

case pre-fix post-fix
new regression test: 8 threads × 50 requests (half with 64 B bodies) 327/400 fail — LocalProtocolError(StreamIDTooLowError), KeyError 400/400 pass (×4 runs)
GET 8×300 33/2400 ok 2400/2400 (×3)
POST 16×100, half with 64 B bodies 8–1191/1600 ok 1600/1600 (×10)
POST 16×400 6400/6400 (×3)
existing tests: sync http2 + connection + pool (37), async http2 + connection (38) 75 passed 75 passed

New regression test: tests/_sync/test_http2_thread_safety.py.

httpx.Client(http2=True) multiplexes requests from multiple threads over a
single connection. HTTP2Connection (and its async twin) wrapped one shared
h2.H2Connection state machine, but while the receive path and socket writes
were already serialized, the send path was not. Concurrent threads could
interleave stream-ID allocation (duplicate IDs -> StreamIDTooLowError),
HPACK encoding in send_headers ("deque mutated during iteration"), and
stream-dict iteration ("dictionary changed size during iteration").

Add a _send_lock (threading.Lock / anyio.AsyncLock), held in
handle_request around stream-ID allocation, _events registration,
_send_request_headers and _send_request_body. The lock is acquired
outermost relative to the existing _read_lock/_write_lock/_state_lock,
and after the max-streams semaphore, so no new deadlock is possible.

Adds tests/_sync/test_http2_thread_safety.py: one shared HTTP2Connection
driven from 8 threads over an in-memory fake HTTP/2 server. Fails on the
pre-fix code (327/400 requests errored with LocalProtocolError /
StreamIDTooLowError / KeyError), passes with the fix.

Related to encode/httpx#3566
- tests/_sync/test_http2_thread_safety.py: add type annotations so mypy
  strict passes (FakeStream now subclasses httpcore.NetworkStream, all
  methods annotated).
- Align the new comments between httpcore/_async/http2.py (unasync source
  of truth) and httpcore/_sync/http2.py so scripts/unasync.py --check
  passes; use a magic trailing comma on the split _send_request_headers
  call so ruff format and unasync agree.
- tests/_sync/test_http2_thread_safety.py: add type annotations so mypy
  strict passes (FakeStream now subclasses httpcore.NetworkStream, all
  methods annotated).
- Align the new comments between httpcore/_async/http2.py (unasync source
  of truth) and httpcore/_sync/http2.py so scripts/unasync.py --check
  passes; use a magic trailing comma on the split _send_request_headers
  call so ruff format and unasync agree.
- tests/_sync/test_http2_thread_safety.py: add type annotations so mypy
  strict passes (FakeStream now subclasses httpcore.NetworkStream, all
  methods annotated).
- Align the new comments between httpcore/_async/http2.py (unasync source
  of truth) and httpcore/_sync/http2.py so scripts/unasync.py --check
  passes; use a magic trailing comma on the split _send_request_headers
  call so ruff format and unasync agree.

This branch has not been deployed

No deployments
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.

deque mutated / dictionary changed during iteration

1 participant