Skip to content
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

Ignore status message for HTTP/2 #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

genki
Copy link

@genki genki commented Jul 1, 2023

To avoid setting status message that is not supported by HTTP/2, checked if the request's HTTP major version is less than 2.
Without this fix some of apps using this package shows warning as follows

(node:49588) UnsupportedWarning: Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)

@genki
Copy link
Author

genki commented Jul 1, 2023

I found that in node 9.x an unexpected warning (as follows) had caught and failed the test.

(node:2676) ExperimentalWarning: The http2 module is an experimental API.

So I fixed the test to recognize the type of warning.

@genki
Copy link
Author

genki commented Jul 5, 2023

@dougwilson Would you mind if I ask you to proceed the workflow?
I have completed to add the test and confirmed it gets succeeded in several environments.

@dougwilson
Copy link
Contributor

Thanks! Sorry it was a holiday weekend. I'm not sure this module actually works with http2 yet, though, but landing this would imply it does. I'll need to check that out before we can land, to make sure http2 even works right. I wouldn't want people thinking it does with this and ending up with some kind of dos vector. Need to make sure the features all work and this module behaves correctly when it encounters broken pipes, half written reaponses, etc

@dougwilson dougwilson self-assigned this Jul 5, 2023
@genki
Copy link
Author

genki commented Jul 9, 2023

I am looking forward to have done it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants