http: set socket timeout when socket connects by lpinca · Pull Request #8895 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

http

Description of change

request.setTimeout() calls socket.setTimeout() as soon as a socket is assigned to the request. This makes the timeout event to be emitted on the request even if the underlying socket never connects.

This commit makes socket.setTimeout() to be called only when the underlying socket is connected.

CI: https://ci.nodejs.org/job/node-test-pull-request/4354/

I would think that this would be semver major since it changes the timing?

Can you capitalize and punctuate this comment please?

Sure. Done.

@evanlucas yeah semver major or patch depending on if it is considered a bug fix or not.

nit: This is probably over-micro-optimizing, but using this instead of sock inside the callback would avoid allocating a closure every time here.

It makes sense. Used sock for readability but happy to fix it.

It doesn't hurt anything, but you don't really need this assertion since the next one would handle this.

Yeah but if for absurd err is undefined it will be a TypeError (Cannot read property 'message' of undefined) instead of an AssertionError.

Yea, but it also doesn't really matter.

@cjihrig I think it's easier to grok but I'm fine with removing it.

Edit: done.

in the off chance that someone updates this later to an arrow function, the use of this internally may be slightly problematic. Perhaps make it more explicit using sock.setTimeout(msec, emitTimeout); instead?

@jasnell read @addaleax's comment above.

😄 … yeah, if you want to leave it at sock.setTimeout for readability, I’m fine with that, too. ;)

Restored sock.

thank you, I appreciate it.

LGTM

re: semver-patch or semver-major, It's not clear to me either. Let's see what @nodejs/ctc think...

It might help to know why the old behaviour was bad, and if it was undesirable in all cases?

Thanks for contributing this! Could you please elaborate on why this is better than the current behavior?

@indutny I don't know if it's better but

It is consistent with the documentation: "Once a socket is assigned to this request and is connected socket.setTimeout() will be called."
It is consistent with this case?

@lpinca thank you for explanation! I see that we have an inconsistency here indeed. I wonder if we should do it in reverse instead and always set timeout without waiting for connect event?

If we'll go the way it is proposed in this PR, http.request().setTimeout() won't be triggered if socket hasn't connected in a reasonable time. Kernel defaults are usually around 1 minute and this may be too much.

@indutny yes it makes sense. I know that some userland modules use an additional timer that is cleared when the response event is emitted to handle cases like the one you are describing.

Should I open a new PR to always set the timeout without waiting for the connect event? If so should this one be closed?

@lpinca perhaps, let's see what others from @nodejs/http or @nodejs/ctc think?

Thanks!

@indutny I thought again about this and I think it doesn't make sense to change request.setTimeout() to make it set the timeout without waiting the connect event.

It seems that in all cases (1, 2) request.setTimeout() explicitly checks if the socket is connected (with the exception of the one this PR deals with) so I guess it's like this by design.

The case you are describing is handled by the timeout option of http.request(). That timeout is forwarded to net.createConnection() without waiting for the socket to be connected.

Closing due to lack of forward progress. We can reopen if necessary

I think it still makes sense. If we want to keep the current behavior we should at least update the documentation accordingly.

I am for reopening this. I think it makes sense and I would consider it as a bug fix because it is clearly documented to behave like this PR.

Fixes a bug that prevented `ClientRequest.prototype.setTimeout()` from working properly when the socket was reused for multiple requests. Fixes: nodejs#16716 Refs: nodejs#8895

Fixes a bug that prevented `ClientRequest.prototype.setTimeout()` from working properly when the socket was reused for multiple requests. Fixes: #16716 Refs: #8895 PR-URL: #16725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Fixes a bug that prevented `ClientRequest.prototype.setTimeout()` from working properly when the socket was reused for multiple requests. Fixes: nodejs#16716 Refs: nodejs#8895 PR-URL: nodejs#16725 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: nodejs#8895

Refs: #8895 PR-URL: #25123 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #25121 Refs: #8895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#25121 Refs: nodejs#8895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #8895 PR-URL: #25123 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #25121 Refs: #8895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 2, 2016

evanlucas reviewed Oct 3, 2016

View reviewed changes

lpinca force-pushed the fix/request-set-timeout branch from 832de1e to c8a10c7 Compare October 3, 2016 11:38

addaleax reviewed Oct 3, 2016

View reviewed changes

lpinca force-pushed the fix/request-set-timeout branch from c8a10c7 to 22a1bbe Compare October 3, 2016 12:15

cjihrig reviewed Oct 3, 2016

View reviewed changes

lpinca force-pushed the fix/request-set-timeout branch from 22a1bbe to 2a263a6 Compare October 3, 2016 13:30

jasnell requested changes Oct 6, 2016

View reviewed changes

lpinca force-pushed the fix/request-set-timeout branch from 2a263a6 to ce555e2 Compare October 6, 2016 18:20

jasnell approved these changes Oct 6, 2016

View reviewed changes

indutny previously requested changes Oct 8, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

lpinca referenced this pull request in sindresorhus/timed-out Jan 16, 2017

simplify code (move out socket timeout part)

42f6998

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

jasnell closed this Aug 24, 2017

jasnell reopened this Aug 25, 2017

gibfahn mentioned this pull request Dec 19, 2017

Investigate flaky parallel/test-listen-fd-ebadf #17762

Closed

This was referenced Dec 19, 2018

Update docs for http request set timeout #25121

Closed

Update v8.x docs for http request set timeout #25122

Closed

jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018

update the http.request.setTimeout docs to be accurate

b1d62ee

Refs: nodejs#8895

jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018

doc: update http.request.setTimeout docs to include history

84cd47b

Refs: nodejs#8895

jbunton-atlassian mentioned this pull request Dec 19, 2018

[v8.x] update the http.request.setTimeout docs to be accurate #25123

Closed

2 tasks

jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018

doc: update the http.request.setTimeout docs to be accurate

fc173b3

Refs: nodejs#8895

jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018

doc: update http.request.setTimeout docs to include history

5708c6a

Refs: nodejs#8895

MylesBorins pushed a commit that referenced this pull request Dec 21, 2018

doc: update the http.request.setTimeout docs to be accurate

3812a99

Refs: #8895 PR-URL: #25123 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Dec 26, 2018

doc: update the http.request.setTimeout docs to be accurate

3af7528

Refs: #8895 PR-URL: #25123 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request Jan 14, 2019

doc: add history to http.request.setTimeout()

8380bd4

PR-URL: #25121 Refs: #8895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

rvagg pushed a commit that referenced this pull request Feb 28, 2019

doc: update the http.request.setTimeout docs to be accurate

b17819d

Refs: #8895 PR-URL: #25123 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

coderabbitai Bot mentioned this pull request Aug 13, 2026

fix(onboard): make the routed credential own the router env and capture router output NVIDIA/NemoClaw#8972

Merged

17 tasks

http: set socket timeout when socket connects by lpinca · Pull Request #8895 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: set socket timeout when socket connects - #8895

http: set socket timeout when socket connects#8895lpinca wants to merge 1 commit intonodejs:masternodejs/node:masterfrom lpinca:fix/request-set-timeoutlpinca/node:fix/request-set-timeoutCopy head branch name to clipboard

Conversation

lpinca commented Oct 2, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

lpinca commented Oct 2, 2016

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 7, 2016

Uh oh!

addaleax commented Oct 7, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 8, 2016

Uh oh!

indutny commented Oct 8, 2016

Uh oh!

lpinca commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Oct 8, 2016

Uh oh!

lpinca commented Oct 18, 2016

Uh oh!

jasnell commented Aug 24, 2017

Uh oh!

lpinca commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Aug 25, 2017

Uh oh!

Reviewers

Assignees

http: set socket timeout when socket connects#8895
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:fix/request-set-timeout

lpinca Oct 3, 2016 •
edited
Loading

lpinca Oct 3, 2016 •
edited
Loading

lpinca Oct 6, 2016 •
edited
Loading

lpinca commented Oct 8, 2016 •
edited
Loading

lpinca commented Aug 25, 2017 •
edited
Loading