-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
[v12.x] Disable headers timeout #33307
[v12.x] Disable headers timeout #33307
Conversation
Hey thank you for your contribution, would you mind adding a test for this? |
Good point, I forgot. Will add it now! |
For some reason "now" became "3 days". :) |
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
I'm not 100% sure about this, but I think this needs to be opened against v12.x-staging rather than v12.x. (Someone else, please confirm!) @nodejs/releasers |
That's correct |
/cc @nodejs/http |
@targos should this still be included in the next v12 release? @nodejs/http @nodejs/http2 @nodejs/streams PTAL |
I hope it would be included. |
PR-URL: #33307 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Landed in ebd9090. Thanks @ShogunPanda |
PR-URL: #33307 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Setting
headersTimeout
to 0 does not disable the checks as expected but rather makes any request fail immediately.This PR fixes the problem. This should also be backported to 10.x.
Node 13 and 14/master are fine since headersTimeout live now in HTTPParser which correctly checks for the value being greater than 0.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes