Don't exceed MAX_CONCURRENT_STREAMS when sending HTTP/2 requests

[sethmlarson]

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.

[tomchristie]

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]

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 as MAX_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?

[tomchristie]

@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]

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]

Moving this issue to httpcore, I feel this has already been addressed but we need to check.

[yeraydiazdiaz]

Yep, this issue is still present.

[tomchristie]

Okay, so there's two related bit here...

• Honoring MAX_CONCURRENT_STREAMS.
• Reconnecting on ConnectionTerminated / NO_ERROR eg. 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 a max_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.

[tomchristie]

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

[tomchristie]

Blergh, sorry - I'm mixing up max concurrent streams vs. max stream id.

[tomchristie]

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

[tomchristie]

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.