net, http: allow setting socket connection timeout for http request by reneweb · Pull Request #8101 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http, net

Description of change

This allows passing the socket connection timeout to http#request
such that it will be set before the socket is connecting

Fixes #7580

The else branch is unnecessary. You're returning the same thing in both branches.

oh yeah, will fix that

@nodejs/http

+1 to @cjihrig's comment. Once that is addressed this should be good.

Why change the name?

It used to be optionsPath since the only option was the path. With this change we introduce non-path related options in there, which is why I felt renaming would make sense and I've chosen createConnectionOptions since we pass them to the createConnection function.

To me optionsPath is more general, like 'options for (unix) socket path' as opposed to 'options for TCP socket'. With that in mind, I'd prefer to see the name unchanged. If other @nodejs/collaborators disagree, at the very least I'd rather see a more succinct name like connectOptions or createOptions or similar.

I think a shorter name is better.

I see what you mean @mscdex . I think it's ok to go back to optionsPath then.

Some nits (same as @mscdex), but LGTM for me.

Labeling in progress because it needs a test.

It also needs a documentation update

I've changed everything as discussed. I also noticed that we wouldn't emit a timeout event. To make it consistent with setTimeout - which does emit a timeout event - and to make it easier to test, I added that functionality. I've also added a test and updated the docs.

Minor nit, but to be consistent we could just reference self.timeout here instead.

Fixed it

Why not simply:

if (req.timeout)
  socket.once('timeout', () => req.emit('timeout'));

Few additional nits. Almost there!

I've fixed all the mentioned issues except for the req error handling one, which I commentent on. I've also applied the fixes to test-http-client-timeout-event, which I based my test off of, so it that is consistent.

LGTM if CI is green.

Shouldn't the timeout option be documented in the net module too?

This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting Fixes nodejs#7580

True, I've added it now to the net documentation

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

Removing in-progress label, because test has been implemented.

From the implementation point of view it should be fine. The only thing is that I got no feedback on, yet, are the doc changes, specifically for the net docs. Unless there are any objection there, it should be good to go.

LGTM

I'm counting three LGTMs and no clear objections for landing this. CI tests have passed.

I've assigned this to myself for landing prep, but I'd like to hold until monday to give everyone time to comment.

I'll start landing this:

Three LGTMs
No objections
All requested changes have been made
CI tests passed (only known CI failures)

landed in c9b59e8

Thank you for your effort and contribution, @reneweb

This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting PR-URL: #8101 Fixes: #7580 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting PR-URL: nodejs#8101 Fixes: nodejs#7580 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting PR-URL: #8101 Fixes: #7580 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) nodejs/node#8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) nodejs/node#8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) nodejs/node#8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) nodejs/node#8100 * npm: Upgraded to 3.10.8 (Kat Marchan) nodejs/node#8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) nodejs/node#8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) nodejs/node#8661 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Can the people who authored/reviewed this comment on #12005? It's unclear if the code actually does what the documentation suggests it does.

Ping @indutny @mcollina

@bnoordhuis to the best of my knowledge, it does what is supposed to be doing. The doc is definitely not clear in what is happening.

nodejs-github-bot added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels Aug 14, 2016

cjihrig reviewed Aug 15, 2016
View reviewed changes

mscdex reviewed Aug 15, 2016
View reviewed changes

Trott added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 16, 2016

reneweb force-pushed the http-request-socket-timeout branch from a4eafb8 to 1acb85f Compare August 21, 2016 19:46

mscdex reviewed Aug 21, 2016
View reviewed changes

reneweb force-pushed the http-request-socket-timeout branch from 1acb85f to 16c6661 Compare August 21, 2016 22:33

jasnell reviewed Aug 24, 2016
View reviewed changes

reneweb force-pushed the http-request-socket-timeout branch from 16c6661 to 00aa5d1 Compare August 28, 2016 01:10

net, http: allow setting socket connection timeout for http request

4341e4d

This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting Fixes nodejs#7580

reneweb force-pushed the http-request-socket-timeout branch from 00aa5d1 to 4341e4d Compare August 31, 2016 00:01

imyller removed the wip Issues and PRs that are still a work in progress. label Sep 24, 2016

imyller self-assigned this Sep 24, 2016

imyller approved these changes Sep 24, 2016

View reviewed changes

imyller closed this Sep 26, 2016

imyller removed their assignment Sep 26, 2016

sotojuan mentioned this pull request Oct 13, 2016

http.request now receives a timeout option sindresorhus/got#226

Closed

joyeecheung mentioned this pull request Mar 9, 2017

net: Socket.prototype.connect should accept args like net.connect #11761 #11762

Closed

3 tasks

lpinca mentioned this pull request Mar 21, 2017

Nock affecting how request times out nock/nock#754

Closed

aleung mentioned this pull request Mar 23, 2017

http.request timeout doesn't take effect #12005

Closed

YECHUNAN mentioned this pull request Oct 7, 2018

https.request timeout options doesn't work #23282

Closed

tulios mentioned this pull request Mar 25, 2020

Request Timeout to include DNS and Socket connection tulios/mappersmith#179

Merged

net, http: allow setting socket connection timeout for http request by reneweb · Pull Request #8101 · nodejs/node · GitHub

Uh oh!

Uh oh!

net, http: allow setting socket connection timeout for http request - #8101

net, http: allow setting socket connection timeout for http request#8101reneweb wants to merge 1 commit intonodejs:masternodejs/node:masterfrom reneweb:http-request-socket-timeoutreneweb/node:http-request-socket-timeoutCopy head branch name to clipboard

Conversation

reneweb commented Aug 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 15, 2016

Uh oh!

jasnell commented Aug 15, 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!

mcollina commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 16, 2016

Uh oh!

mscdex commented Aug 16, 2016

Uh oh!

reneweb commented Aug 21, 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!

jasnell commented Aug 24, 2016

Uh oh!

reneweb commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 29, 2016

Uh oh!

santigimeno commented Aug 30, 2016

Uh oh!

reneweb commented Aug 31, 2016

Uh oh!

imyller commented Sep 24, 2016

Uh oh!

imyller commented Sep 24, 2016

Uh oh!

reneweb commented Sep 24, 2016

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

imyller commented Sep 24, 2016

Uh oh!

imyller commented Sep 26, 2016

Uh oh!

imyller commented Sep 26, 2016

Uh oh!

bnoordhuis commented Mar 24, 2017

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

mcollina commented Mar 24, 2017

Uh oh!

Reviewers

Assignees

Labels

net, http: allow setting socket connection timeout for http request#8101
reneweb wants to merge 1 commit into
nodejs:masterfrom
reneweb:http-request-socket-timeout

reneweb commented Aug 14, 2016 •
edited
Loading

mcollina commented Aug 16, 2016 •
edited
Loading

reneweb commented Aug 28, 2016 •
edited
Loading