-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: make timeout event work with agent timeout #25488
Conversation
@lpinca sadly an error occured when I tried to trigger a build :( |
f4f3182
to
07a6300
Compare
The `'timeout'` event is currently not emitted on the `ClientRequest` instance when the socket timeout expires if only the `timeout` option of the agent is set. This happens because, under these circumstances, `listenSocketTimeout()` is not called. This commit fixes the issue by calling it also when only the agent `timeout` option is set.
07a6300
to
772c28b
Compare
cc: @nodejs/http |
@lpinca Do you want to merge this? |
@addaleax let's wait a little more for another approval, it's not a critical issue. We can merge this in a couple of days if nothing changed. Thank you. |
Landed in 4b6e4c1. |
The `'timeout'` event is currently not emitted on the `ClientRequest` instance when the socket timeout expires if only the `timeout` option of the agent is set. This happens because, under these circumstances, `listenSocketTimeout()` is not called. This commit fixes the issue by calling it also when only the agent `timeout` option is set. PR-URL: #25488 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
The test added in this PR fails on my computer:
|
Ok, I will change it to prevent it from using a non routable IP address so it will not be flaky. I'll get to it as soon as I can. |
Fix flakyness caused by usage of a non-routable IP address. Refs: nodejs#25488 (comment)
The `'timeout'` event is currently not emitted on the `ClientRequest` instance when the socket timeout expires if only the `timeout` option of the agent is set. This happens because, under these circumstances, `listenSocketTimeout()` is not called. This commit fixes the issue by calling it also when only the agent `timeout` option is set. PR-URL: #25488 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fix flakyness caused by usage of a non-routable IP address. Refs: nodejs#25488 (comment) PR-URL: nodejs#25854 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
Fix flakyness caused by usage of a non-routable IP address. Refs: #25488 (comment) PR-URL: #25854 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
The
'timeout'
event is currently not emitted on theClientRequest
instance when the socket timeout expires if only the
timeout
optionof the agent is set. This happens because, under these circumstances,
listenSocketTimeout()
is not called.This commit fixes the issue by calling it also when only the agent
timeout
option is set.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes