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: fix request with option timeout and agent #21204

Closed
wants to merge 7 commits into from

Conversation

killagu
Copy link
Contributor

@killagu killagu commented Jun 8, 2018

When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Update agent doc, add timeout option.

Fixes: #21185

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

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels Jun 8, 2018
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Update agent doc, add timeout option.

Fixes: nodejs#21185
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@vsemozhetbyt
Copy link
Contributor

Trott
Trott previously requested changes Jun 9, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant test failure in CI.

https://ci.nodejs.org/job/node-test-commit-arm/16725/nodes=debian8-docker-armv7/console

07:23:39 not ok 762 parallel/test-http-client-timeout-option-with-agent
07:23:39   ---
07:23:39   duration_ms: 2.837
07:23:39   severity: fail
07:23:39   exitcode: 1
07:23:39   stack: |-
07:23:39     assert.js:270
07:23:39         throw err;
07:23:39         ^
07:23:39     
07:23:39     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
07:23:39     
07:23:39       assert(Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20)
07:23:39     
07:23:39         at ClientRequest.req.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-arm/nodes/debian8-docker-armv7/test/parallel/test-http-client-timeout-option-with-agent.js:40:5)
07:23:39         at ClientRequest.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/debian8-docker-armv7/test/common/index.js:451:15)
07:23:39         at ClientRequest.emit (events.js:182:13)
07:23:39         at Socket.emitRequestTimeout (_http_client.js:660:40)
07:23:39         at Object.onceWrapper (events.js:273:13)
07:23:39         at Socket.emit (events.js:182:13)
07:23:39         at Socket._onTimeout (net.js:445:8)
07:23:39         at ontimeout (timers.js:471:11)
07:23:39         at tryOnTimeout (timers.js:341:5)
07:23:39         at listOnTimeout (timers.js:315:5)
07:23:39   ...

https://ci.nodejs.org/job/node-test-commit-arm/16725/nodes=ubuntu1604-arm64/console

07:15:19 not ok 794 parallel/test-http-client-timeout-option-with-agent
07:15:19   ---
07:15:19   duration_ms: 2.909
07:15:19   severity: fail
07:15:19   exitcode: 1
07:15:19   stack: |-
07:15:19     assert.js:270
07:15:19         throw err;
07:15:19         ^
07:15:19     
07:15:19     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
07:15:19     
07:15:19       assert(Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20)
07:15:19     
07:15:19         at ClientRequest.req.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-http-client-timeout-option-with-agent.js:40:5)
07:15:19         at ClientRequest.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/common/index.js:451:15)
07:15:19         at ClientRequest.emit (events.js:182:13)
07:15:19         at Socket.emitRequestTimeout (_http_client.js:660:40)
07:15:19         at Object.onceWrapper (events.js:273:13)
07:15:19         at Socket.emit (events.js:182:13)
07:15:19         at Socket._onTimeout (net.js:445:8)
07:15:19         at ontimeout (timers.js:471:11)
07:15:19         at tryOnTimeout (timers.js:341:5)
07:15:19         at listOnTimeout (timers.js:315:5)
07:15:19   ...

Looks like the test contains a somewhat arbitrary setTimeout(). Ideally, the test should be rewritten without it. But a minimal workaround may be to move the test out of the parallel directory and into the sequential directory.

@Trott
Copy link
Member

Trott commented Jun 9, 2018

You can probably replicate the test failures locally by running multiple copies of the test simultaneously:

tools/test.py -j 50 --repeat 100 parallel/test-http-client-timeout-option-with-agent

@@ -0,0 +1,48 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be willing to add a comment here explaining what this is testing?

req.on('timeout', common.mustCall(() => {
timeout_events += 1;
const duration = Date.now() - start;
assert((Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the request for a comment above, it would be useful in particular to know if what this is all about. 20 seems like a magic number and this sort of fuzzy duration check is a bit of a red flag in tests too. Curious if there's another way to do it that still manages to check whatever the test is testing for.

@Trott
Copy link
Member

Trott commented Jun 9, 2018

@nodejs/testing @nodejs/http

@jasnell jasnell requested a review from mcollina June 10, 2018 20:46
@killagu
Copy link
Contributor Author

killagu commented Jun 11, 2018

@Trott Thank you for the response. The test comment have been added, and the magic number have been removed. Please review it again.

@Trott Trott dismissed their stale review June 11, 2018 03:21

tests seem reliable now, unblocking

@Trott
Copy link
Member

Trott commented Jun 11, 2018

@XadillaX
Copy link
Contributor

@ryzokuken
Copy link
Contributor

@Trott can this be landed now?

@ryzokuken ryzokuken added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 13, 2018
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 14, 2018
@Trott
Copy link
Member

Trott commented Jun 14, 2018

@Trott can this be landed now?

Theoretically, yes, but in practice, I try to ping for more reviews rather than land if something only has one review. Especially a semver-minor (because we'll be stuck supporting a new option forever). So, in that spirit:

/ping @nodejs/http @nodejs/streams @nodejs/tsc

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@targos
Copy link
Member

targos commented Jun 22, 2018

@mcollina
Copy link
Member

mcollina commented Jul 5, 2018

@addaleax
Copy link
Member

Landed in 949e885, thanks for the PR! 🎉

@addaleax addaleax closed this Jul 13, 2018
addaleax pushed a commit that referenced this pull request Jul 13, 2018
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Fixes: #21185

PR-URL: #21204
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
targos pushed a commit that referenced this pull request Jul 14, 2018
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Fixes: #21185

PR-URL: #21204
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 17, 2018
targos added a commit that referenced this pull request Jul 17, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented. (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* napi:
  * Added experimental support for functions dealing with bigint numbers. (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented. (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata. (#21477)
@targos targos mentioned this pull request Jul 17, 2018
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented. (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* napi:
  * Added experimental support for functions dealing with bigint numbers. (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented. (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata. (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
@fengmk2
Copy link
Contributor

fengmk2 commented Oct 20, 2018

Should this bugfix backport to Node 8?

@mcollina
Copy link
Member

I would prefer not to, it caused some
breakage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request with agent, timeout not work