Repository navigation
Conversation
| except NoAvailableStreamIDError: | ||
| self.state = h2.connection.H2Connection(config=self.CONFIG) | ||
| self.init_stated = False | ||
| self.send(request, timeout) |
There was a problem hiding this comment.
It's recursively calling send. Maybe limit it/change behaviour in case NoAvailableStreamIDError is reoccurring?
There was a problem hiding this comment.
It's recursively calling send. Maybe limit it/change behaviour in case NoAvailableStreamIDError is reoccurring?
NoAvailableStreamIDError shouldn't be raised again since we're opening a new connection. I suppose the exception should only be raised when the identifier for stream ids expire, which is 2^31 on a particular connection I guess. If it does get raised, that would be a bug in the hyper lib?
Also, shouldn't you await ?
yes, I missed that.
There was a problem hiding this comment.
I'm not very experienced with HTTP2, but I can assume that the exception can also come from the serverside? If so, a 'bad' server could lead to such scenario.
Also I'd add tests to cover the new scenario.
There was a problem hiding this comment.
I'm not very experienced with HTTP2, but I can assume that the exception can also come from the serverside? If so, a 'bad' server could lead to such scenario.
Also I'd add tests to cover the new scenario.
Yes, so I believe according to the HTTP2 spec, even numbers are reserved for the server to identify it's streams with and odd numbers are for the client to label it's streams with, so it is quite possible that the stream id's expire on the server side, but I'm not sure if NoAvailableStreamIDError would be raised in that case. Also, I was hoping maybe we could discuss on how to test this in the review, because I guess we'd need to spin up a MockHTTP2Server and configure it to accept only one stream id, so it gets exhausted. But I couldn't find any other tests doing this thing so I was hoping I could get some advice on how to test it here.
There was a problem hiding this comment.
The test you suggested is blackbox, why not just patch H2 Connection to raise the exception?
I don't see the a lot of value in doing blackbox, especially when the exception is coming from another library (h2).
| await self.init_complete.wait() | ||
| stream_id = self.state.get_next_available_stream_id() | ||
| except NoAvailableStreamIDError: | ||
| self.state = h2.connection.H2Connection(config=self.CONFIG) |
There was a problem hiding this comment.
We can't do this, since it'll break existing streams.
Getting this right is a little bit complex, and I think we should probably pause on it, pending some more comprehensive work around connection state management along the lines in #624
|
@gdhameeja Really appreciate your time on having taken a crack at this. Given #647 (comment) I'm going to close this PR off. |
Increase the default allowable http header size to 100kB similar to curl. Raised in discussion encode/httpx#2520 Co-authored-by: Huy Au <hau@invast.com.au>
Fixes #453
Creating new connections when stream ids expire by catching
NoAvailableStreamIDErrorinsendmethod ofHTTP2Connection.