Skip to content

Conversation

@lpinca
Copy link
Member

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@lpinca sadly an error occured when I tried to trigger a build :(

@nodejs-github-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 14, 2019
@lpinca
Copy link
MemberAuthor

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.
@lpinca
Copy link
MemberAuthor

lpinca commented Jan 21, 2019

cc: @nodejs/http
New CI: https://ci.nodejs.org/job/node-test-pull-request/20222/ (:heavy_check_mark:)

@addaleax
Copy link
Member

@lpinca Do you want to merge this?

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019
@lpinca
Copy link
MemberAuthor

@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.

@lpinca
Copy link
MemberAuthor

Landed in 4b6e4c1.

@lpincalpinca closed this Jan 31, 2019
@lpincalpinca deleted the fix/timeout-event branch January 31, 2019 10:52
lpinca added a commit that referenced this pull request Jan 31, 2019
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]>
@targos
Copy link
Member

The test added in this PR fails on my computer:

AssertionError [ERR_ASSERTION]: Expected "actual" to be reference-equal to "expected": + actual - expected ... Lines skipped Comparison{+ code: 'EHOSTUNREACH', + message: 'connect EHOSTUNREACH 192.0.2.1:80', - code: 'ECONNRESET', - message: 'socket hang up', type: [Function: Error]{... } } at new AssertionError (internal/assert.js:396:11) 

@lpinca
Copy link
MemberAuthor

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.

lpinca added a commit to lpinca/node that referenced this pull request Jan 31, 2019
Fix flakyness caused by usage of a non-routable IP address. Refs: nodejs#25488 (comment)
addaleax pushed a commit that referenced this pull request Feb 1, 2019
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]>
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 2, 2019
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]>
addaleax pushed a commit that referenced this pull request Feb 3, 2019
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]>
@targostargos mentioned this pull request Feb 14, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.httpIssues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@lpinca@nodejs-github-bot@addaleax@targos@jasnell