Repository navigation
Don't exceed MAX_CONCURRENT_STREAMS when sending HTTP/2 requests #85
Description
Bug described in encode/httpx#281 where HTTPX doesn't support waiting for other streams to close before creating a new stream on an HTTP/2 connection.
Activity
lovelydinosaur commentedon Aug 27, 2019
Also noting http://http2.github.io/http2-spec/index.html#rfc.section.9.1 "Clients SHOULD NOT open more than one HTTP/2 connection to a given host and port pair" means that we shouldn't just open up a new conenction when we hit this limit.
ndhansen commentedon Mar 2, 2020
I'm familiar to asyncio but new to this project. Before I start working on this, my intuitive solution would be to wrap https://github.com/encode/httpx/blob/master/httpx/_dispatch/http2.py#L58-L63 in a semaphore, ideally one inheriting from the
BaseSemaphore, with the same capacity asMAX_CONCURRENT_STREAMS.That way it wouldn't acquire the lock before it has an available, and would wait until a stream becomes free.
Does that sound about right?
lovelydinosaur commentedon Mar 2, 2020
@ndhansen Exactly correct yup, although I'd suggest on resolving this in https://github.com/encode/httpcore rather than here, since the intent is to move to that package for providing the core network dispatching.
ndhansen commentedon Mar 3, 2020
Thanks @tomchristie! Is there a branch in this project that already uses httpcore, or should I open a PR containing the equivalent code when the PR in httpcore is approved?
yeraydiazdiaz commentedon May 13, 2020
Moving this issue to httpcore, I feel this has already been addressed but we need to check.
yeraydiazdiaz commentedon May 13, 2020
Yep, this issue is still present.
lovelydinosaur commentedon May 13, 2020
Okay, so there's two related bit here...
ConnectionTerminated / NO_ERROReg. ConnectionTerminated error_code:ErrorCodes.NO_ERROR #45 and also comment in possibly wrong handling of error_code? #87 (comment) (Eg. Apparently nginx doesn't issue amax_stream_id, but will close the connection after 1000 streams, so we need to gracefully re-issue requests for that case.)Really we ought to tackle the second of those points, and then treat the first as an additional enhancement on top of that.
lovelydinosaur commentedon May 13, 2020
We should raise
NewConnectionRequired()in either of those cases.Are we already dealing with the MAX_CONCURRENT_STREAMS case, here?...
https://github.com/encode/httpcore/blob/master/httpcore/_async/http2.py#L94-L98
lovelydinosaur commentedon May 13, 2020
Blergh, sorry - I'm mixing up max concurrent streams vs. max stream id.
lovelydinosaur commentedon May 13, 2020
I suppose we want to wrap this block up in a semaphore...
https://github.com/encode/httpcore/blob/master/httpcore/_async/http2.py#L102-L105
lovelydinosaur commentedon May 13, 2020
So, looks to me like https://github.com/encode/httpcore/pull/38/files has it about right, but really we should wrap the semaphore around the post-init part of the request method.