-
Notifications
You must be signed in to change notification settings - Fork 192
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
http: PRI upgrade should return PAUSED_H2_UPGRADE #95
Conversation
`PAUSED_UPGRADE` is a recoverable error and should not be returned when parsing HTTP/2 `PRI /...` preambule, which is the last message that can be parsed on the stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understood these changes correctly, ERROR.PAUSED_H2_UPGRADE
is returned instead of ERROR.PAUSED_UPGRADE
in http2
, which is not a recoverable error. ERROR.PAUSED_UPGRADE
behavior is unchanged in http/1.1
. Thanks for the heads up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
`PAUSED_UPGRADE` is a recoverable error and should not be returned when parsing HTTP/2 `PRI /...` preambule, which is the last message that can be parsed on the stream. PR-URL: #95 Reviewed-By: Daniele Belardi <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 374434d, thank you. |
`PAUSED_UPGRADE` is a recoverable error and should not be returned when parsing HTTP/2 `PRI /...` preambule, which is the last message that can be parsed on the stream. PR-URL: #95 Reviewed-By: Daniele Belardi <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
Released v4.1.0 and v5.1.0 |
Sorry I missed this, LGTM as well |
PAUSED_UPGRADE
is a recoverable error and should not be returned whenparsing HTTP/2
PRI /...
preambule, which is the last message that canbe parsed on the stream.
@pallas could you give this a quick review when you'll get a chance? I've tried updating llhttp in Node.js and quickly ran into this issue. Basically, our documentation says that
llhttp_resume_after_upgrade
should be called afterPAUSED_UPGRADE
, and it resets the.error
to0
, but in reality the parser is in non-recoverable state! This is a breaking change, but I'm going to treat it as a bugfix and do a minor release for all branches once landed.cc @nodejs/http @nodejs/http2 as well