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

reneweb · 2016-08-14T19:58:27Z

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

cjihrig · 2016-08-15T14:28:38Z

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

oh yeah, will fix that

jasnell · 2016-08-15T15:54:00Z

@nodejs/http

jasnell · 2016-08-15T15:55:57Z

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

mscdex · 2016-08-15T16:23:16Z

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.

mcollina · 2016-08-16T15:04:39Z

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

Trott · 2016-08-16T20:08:57Z

Labeling in progress because it needs a test.

mscdex · 2016-08-16T20:14:05Z

It also needs a documentation update

reneweb · 2016-08-21T19:50:26Z

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.

mscdex · 2016-08-21T20:11:52Z

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

jasnell · 2016-08-24T22:26:36Z

Why not simply:
if(req.timeout)socket.once('timeout',()=>req.emit('timeout'));

jasnell · 2016-08-24T22:29:53Z

Few additional nits. Almost there!

reneweb · 2016-08-28T01:16:53Z

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.

jasnell · 2016-08-29T21:06:25Z

LGTM if CI is green.

santigimeno · 2016-08-30T08:50:33Z

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 Fixesnodejs#7580

reneweb · 2016-08-31T00:02:45Z

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

imyller · 2016-09-24T19:19:07Z

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

imyller · 2016-09-24T19:21:21Z

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

reneweb · 2016-09-24T20:15:50Z

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.

imyller

LGTM

imyller · 2016-09-24T21:13:31Z

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.

imyller · 2016-09-26T10:34:26Z

I'll start landing this:

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

imyller · 2016-09-26T10:34:53Z

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: #8101Fixes: #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#8101Fixes: 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: #8101Fixes: #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>

bnoordhuis · 2017-03-24T11:03:43Z

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

jasnell · 2017-03-24T15:03:01Z

Ping @indutny @mcollina

mcollina · 2017-03-24T18:28:58Z

@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 1acb85fCompare 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 16c6661Compare August 21, 2016 22:33

jasnell reviewed Aug 24, 2016
View reviewed changes

Comment threadlib/_http_client.js Outdated
jasnellAug 24, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply:
if(req.timeout)socket.once('timeout',()=>req.emit('timeout'));

reneweb force-pushed the http-request-socket-timeout branch from 16c6661 to 00aa5d1Compare 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 Fixesnodejs#7580

reneweb force-pushed the http-request-socket-timeout branch from 00aa5d1 to 4341e4dCompare 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

Uh oh!

Uh oh!

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

Projects

Milestone

Development

reneweb commented Aug 14, 2016•
edited
Loading

mcollina commented Aug 16, 2016•
edited
Loading

reneweb commented Aug 28, 2016•
edited
Loading