Skip to content

Reset the HTTP/2 stream when a request is cancelled#1098

Open
wwmanidumaneesha wants to merge 1 commit into
encode:masterfrom
wwmanidumaneesha:fix/h2-stream-desync-on-cancel
Open

Reset the HTTP/2 stream when a request is cancelled#1098
wwmanidumaneesha wants to merge 1 commit into
encode:masterfrom
wwmanidumaneesha:fix/h2-stream-desync-on-cancel

Conversation

@wwmanidumaneesha

Copy link
Copy Markdown

Reset the HTTP/2 stream when a request is cancelled

Closes #1022.

The problem

_response_closed() releases our own stream semaphore, but never tells h2 that the stream is finished:

async def _response_closed(self, stream_id: int) -> None:
    await self._max_streams_semaphore.release()
    del self._events[stream_id]

reset_stream is never called anywhere in http2.py.

When a request is cancelled before its END_STREAM frame reaches the network, h2 still counts the stream as open while we count it as closed. The two drift apart by one on every cancelled request, until h2 rejects new streams with:

httpcore.LocalProtocolError: Max outbound streams is 100, 100 open

...even though the pool still believes it has spare capacity. Credit to @fcourtial for the diagnosis in #1022.

The fix

Reset the stream locally when it is still open, so both sides agree on the count.

The RST_STREAM frame this queues is deliberately not flushed here. _response_closed() runs inside a cancellation shield during teardown, and taking _write_lock at that point risks deadlocking against the request currently holding it — which is what made the more direct attempts in #1022 either deadlock or fall short. The frame is written out by the next _write_outgoing_data() call instead.

Tests

test_h2_stream_count_stays_in_sync_after_cancellation cancels a request once its headers have provably reached the stream, then asserts open_outbound_streams == 0.

The cancellation is triggered off an event rather than a timeout, so it always lands during the request write rather than sometimes during the handshake. On master it fails with assert 1 == 0, 10 runs out of 10.

`_response_closed()` releases our stream semaphore, but never told `h2`
that the stream was finished. When a request is cancelled before its
END_STREAM frame reaches the network, `h2` continues to count the stream
as open while we count it as closed.

The two therefore drift apart by one on every cancelled request, until
`h2` rejects new streams with "Max outbound streams is 100, 100 open"
even though the pool still believes it has spare capacity.

The stream is now reset locally so that both sides agree. The RST_STREAM
frame this queues is not flushed here, since this runs inside a
cancellation shield during teardown and taking the write lock at that
point risks deadlocking against the request holding it. The frame is
written out by the next call to `_write_outgoing_data()`.

Closes encode#1022.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deadlock: Stream Synchronization Issue Between httpcore and h2

1 participant