Fix HTTP/2 race condition waiting on stream events.#439
Merged
lovelydinosaur merged 1 commit intoNov 18, 2021
Conversation
Merged
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.
While looking at https://github.com/encode/httpx/issues/1414 I've come across a race condition in the HTTP/2 handling. It's not something I'm able to figure out any sensible test case for, but can describe how to reproduce locally...
First we'll setup
Hypercornas a locally HTTP/2-capable server...app.py
Let's run that lil kiddo...
Okey dokes, now let's bump into it with a coupla concurrent HTTP/2 requests. We've not setup any certificate on the hypercorn server, so we'll just enforce HTTP/2 support, so that we'll still get HTTP/2 without any
httpsconnection.If we run that without this proposed change we get the following...
Here's why:
AandBboth start.Ahitswhile not self._events.get(stream_id)- there's no events yet so we move on to_receive_events().Bhitswhile not self._events.get(stream_id)- there's no events yet so we move on to_receive_events().Areads the response events for both streams, and returns ok.Bhangs waiting for network data, despite the fact that there is now actually an event for it to return.The key to resolving this is to move the read lock, so that we check "have we got an event that we can return" inside the lock.
I'm not wild about this PR, because it's just difficult to be convinced that it's resilient and correct enough, and it's also not obvious how to test case it. but. Working code's gotta be better than code that I can demo as hanging. So.