-
Notifications
You must be signed in to change notification settings - Fork 30k
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
http2/http1 compatibility API error with connection header #23748
Comments
This is a tricky one. The HTTP/2 is definitely not backwards compatible with HTTP/1. The compat API is a best attempt to get it as close as possible. Will have to think a bit on how to handle this. /cc @nodejs/http2 |
We could just log a warning once on the compatibility side and ignore the header (not pass it on). That would probably make the most sense here? |
The compatibility API could be viewed as an "intermediary":
(Emphasis added) |
Emitting a warning could be ok. I wonder if a config option would be better tho? Changing the behavior now would be semver-major, adding an option would be semver-minor |
I think we should change the behavior. This looks more like a bugfix to me than semver-major. |
I think it is better to ignore the connection in the header + log a warning. |
#23908 is finished and waiting |
Ignoring the connection header and disable the `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. Added a warning log on the compatibility. Fixes: nodejs#23748
When using the compatibility API the connection header is from now on ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. This logs a warning in such case to notify the user about the ignored header. PR-URL: #23908 Fixes: #23748 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
When using the compatibility API the connection header is from now on ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. This logs a warning in such case to notify the user about the ignored header. PR-URL: #23908 Fixes: #23748 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
When using the compatibility API the connection header is from now on ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS` error. This logs a warning in such case to notify the user about the ignored header. PR-URL: #23908 Fixes: #23748 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Original Issue: hapijs/hapi#3830
What you expect:
No error should be thrown. The
connection
header should be ignored by the http2 module.What actually happens:
An error is thrown.
Description:
ERR_HTTP2_INVALID_CONNECTION_HEADERS
error seems to break the http2 compatibility API. By adding aconnection
header to an http2 response, theERR_HTTP2_INVALID_CONNECTION_HEADERS
error is thrown. Seeing as http2 is supposed to be backwards compatible with http1, shouldn't http2 just ignore theconnection
header?MDN states:
The text was updated successfully, but these errors were encountered: