Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: set socket timeout when socket connects #8895

Closed
wants to merge 1 commit into from

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented Oct 2, 2016

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.

Copy link
Member

@evanlucas evanlucas left a comment

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

test/parallel/test-http-client-timeout-on-connect.js Outdated
const http = require('http');

const server = http.createServer((req, res) => {
// this space intentionally left blank

This comment has been minimized.

@evanlucas

evanlucas Oct 3, 2016
Member

Can you capitalize and punctuate this comment please?

This comment has been minimized.

@lpinca

lpinca Oct 3, 2016
Author Member

Sure. Done.

@lpinca
Copy link
Member Author

@lpinca lpinca commented Oct 3, 2016

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

@lpinca lpinca force-pushed the lpinca:fix/request-set-timeout branch Oct 3, 2016
@@ -639,7 +639,9 @@ ClientRequest.prototype.setTimeout = function(msecs, callback) {
}

this.once('socket', function(sock) {
sock.setTimeout(msecs, emitTimeout);
sock.once('connect', function() {
sock.setTimeout(msecs, emitTimeout);

This comment has been minimized.

@addaleax

addaleax Oct 3, 2016
Member

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

This comment has been minimized.

@lpinca

lpinca Oct 3, 2016
Author Member

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

@lpinca lpinca force-pushed the lpinca:fix/request-set-timeout branch Oct 3, 2016
test/parallel/test-http-client-timeout-on-connect.js Outdated
}));
req.on('timeout', common.mustCall(() => req.abort()));
req.on('error', common.mustCall((err) => {
assert.ok(err);

This comment has been minimized.

@cjihrig

cjihrig Oct 3, 2016
Contributor

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

This comment has been minimized.

@lpinca

lpinca Oct 3, 2016
Author Member

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

This comment has been minimized.

@cjihrig

cjihrig Oct 3, 2016
Contributor

Yea, but it also doesn't really matter.

This comment has been minimized.

@lpinca

lpinca Oct 3, 2016
Author Member

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

Edit: done.

@lpinca lpinca force-pushed the lpinca:fix/request-set-timeout branch Oct 3, 2016
lib/_http_client.js Outdated
@@ -639,7 +639,9 @@ ClientRequest.prototype.setTimeout = function(msecs, callback) {
}

this.once('socket', function(sock) {
sock.setTimeout(msecs, emitTimeout);
sock.once('connect', function() {
this.setTimeout(msecs, emitTimeout);

This comment has been minimized.

@jasnell

jasnell Oct 6, 2016
Member

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?

This comment has been minimized.

@lpinca

lpinca Oct 6, 2016
Author Member

@jasnell read @addaleax's comment above.

This comment has been minimized.

@addaleax

addaleax Oct 6, 2016
Member

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

This comment has been minimized.

@lpinca

lpinca Oct 6, 2016
Author Member

Restored sock.

This comment has been minimized.

@jasnell

jasnell Oct 6, 2016
Member

thank you, I appreciate it.

@lpinca lpinca force-pushed the lpinca:fix/request-set-timeout branch Oct 6, 2016
@jasnell
jasnell approved these changes Oct 6, 2016
Copy link
Member

@jasnell jasnell left a comment

LGTM

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 7, 2016

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

@addaleax
Copy link
Member

@addaleax addaleax commented Oct 7, 2016

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

Copy link
Member

@indutny indutny left a comment

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

@lpinca
Copy link
Member Author

@lpinca lpinca commented Oct 8, 2016

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

  1. It is consistent with the documentation: "Once a socket is assigned to this request and is connected socket.setTimeout() will be called."
  2. It is consistent with this case?
@indutny
Copy link
Member

@indutny indutny commented Oct 8, 2016

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

@lpinca
Copy link
Member Author

@lpinca lpinca commented Oct 8, 2016

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

@indutny
Copy link
Member

@indutny indutny commented Oct 8, 2016

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

Thanks!

@lpinca
Copy link
Member Author

@lpinca lpinca commented Oct 18, 2016

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

@rvagg rvagg force-pushed the nodejs:master branch 2 times, most recently to 83c7a88 Oct 18, 2016
lpinca referenced this pull request in sindresorhus/timed-out Jan 16, 2017
@jasnell jasnell added the stalled label Mar 1, 2017
@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017
@jasnell
Copy link
Member

@jasnell jasnell commented Aug 24, 2017

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

@jasnell jasnell closed this Aug 24, 2017
@lpinca
Copy link
Member Author

@lpinca lpinca commented Aug 25, 2017

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Aug 25, 2017

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.

@jasnell jasnell reopened this Aug 25, 2017
lpinca added a commit to lpinca/node that referenced this pull request Nov 6, 2017
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
lpinca added a commit that referenced this pull request Nov 6, 2017
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>
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 6, 2017
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>
jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018
jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018
jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018
jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018
MylesBorins added a commit that referenced this pull request Dec 21, 2018
Refs: #8895

PR-URL: #25123
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 26, 2018
Refs: #8895

PR-URL: #25123
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
vsemozhetbyt added a commit that referenced this pull request Jan 9, 2019
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>
addaleax added a commit that referenced this pull request Jan 14, 2019
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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>
rvagg added a commit that referenced this pull request Feb 28, 2019
Refs: #8895

PR-URL: #25123
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this pull request Apr 28, 2019
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>
BethGriggs added a commit that referenced this pull request May 10, 2019
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>
MylesBorins added a commit that referenced this pull request May 16, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.