-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
possibly wrong handling of error_code? #87
Comments
Indeed. I wonder if a more comprehensive tack onto this would be...
Does that sound sufficient here? |
GOAWAYKindof, with a caveat: if the connection is being closed:
For this stream: I doubt raising right away is correct, because Thus, I think it's simpler to change the handling to:
What do you think? RST_STREAMI think the data model needs to change a bit, for example:
|
GOAWAY error codeThe spec says:
https://tools.ietf.org/html/rfc7540#section-6.8 I'm not an expert, but to me, this sounds like client should have 2 code paths:
↑↑↑ That's just my reading, I could be wrong... In fact after reading the spec again, I'm not sure that non-zero goaway means terminate... Ref for non-zero error code: https://tools.ietf.org/html/rfc7540#section-5.4.1 Perhaps |
RST_STREAM error codehttps://tools.ietf.org/html/rfc7540#section-8.1.4 contains this special case:
So, handling should be different depending on |
@dimaqq This looks like a weak spot on our end, yes… Have you seen any issues caused by this yet? |
Maybe... I'm not sure. It's possible that the cause was that RST_STREAM was being treated like GOAWAY and that being taken as hard connection shutdown. 🤷♂ |
The default Ref: http://nginx.org/en/docs/http/ngx_http_v2_module.html#http2_max_requests If someone runs How
re: 1.: I think it really should inform client in advance how many streams are allowed, oh well. So, uhm, it would be nice to support |
https://github.com/encode/httpx/blob/50d337e807839c21e796fd8b01c67d8a672a9721/httpx/_dispatch/http2.py#L147-L155
IIRC, RFC defines 2 messages that may carry
error_code
:In addition,
h2
might generateResetStream
event internally:I find it weird that there's only this one place where
error_code
is evaluated in the library 🤔The text was updated successfully, but these errors were encountered: