net, http: allow setting socket connection timeout for http request #8101
Conversation
lib/net.js
Outdated
There was a problem hiding this comment.
The else branch is unnecessary. You're returning the same thing in both branches.
There was a problem hiding this comment.
oh yeah, will fix that
|
@nodejs/http |
|
+1 to @cjihrig's comment. Once that is addressed this should be good. |
lib/_http_client.js
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think a shorter name is better.
There was a problem hiding this comment.
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 |
|
It also needs a documentation update |
a4eafb8 to
1acb85f
Compare
|
I've changed everything as discussed. I also noticed that we wouldn't emit a timeout event. To make it consistent with |
lib/_http_client.js
Outdated
There was a problem hiding this comment.
Minor nit, but to be consistent we could just reference self.timeout here instead.
1acb85f to
16c6661
Compare
lib/_http_client.js
Outdated
There was a problem hiding this comment.
Why not simply:
if (req.timeout)
socket.once('timeout', () => req.emit('timeout'));|
Few additional nits. Almost there! |
16c6661 to
00aa5d1
Compare
|
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 |
|
LGTM if CI is green. |
|
Shouldn't the |
This allows passing the socket connection timeout to http#request such that it will be set before the socket is connecting Fixes nodejs#7580
00aa5d1 to
4341e4d
Compare
|
True, I've added it now to the |
|
Removing |
|
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 |
|
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:
|
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: #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) #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>
* 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. |
|
@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. |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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