Reset the HTTP/2 stream when a request is cancelled#1098
Open
wwmanidumaneesha wants to merge 1 commit into
Open
Reset the HTTP/2 stream when a request is cancelled#1098wwmanidumaneesha wants to merge 1 commit into
wwmanidumaneesha wants to merge 1 commit into
Conversation
`_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.
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.
Reset the HTTP/2 stream when a request is cancelled
Closes #1022.
The problem
_response_closed()releases our own stream semaphore, but never tellsh2that the stream is finished:reset_streamis never called anywhere inhttp2.py.When a request is cancelled before its END_STREAM frame reaches the network,
h2still counts the stream as open while we count it as closed. The two drift apart by one on every cancelled request, untilh2rejects new streams with:...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_lockat 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_cancellationcancels a request once its headers have provably reached the stream, then assertsopen_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
masterit fails withassert 1 == 0, 10 runs out of 10.