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

[12.x] http, http2: flag for overriding server timeout #27704

Closed

Conversation

Projects
None yet
8 participants
@ofrobots
Copy link
Contributor

commented May 14, 2019

Make it possible to override the default http server timeout. Ideally
there should be no server timeout - as done on the master branch. This
is a non-breaking way to enable platform providers to override the
value.

Ref: #27558
Ref: #27556

This is a semver-minor alternative to #27558 suitable for backporting to 8.x, 10.x and 12.x.

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

This comment has been minimized.

@addaleax addaleax added http http2 https and removed lib / src labels May 14, 2019

@nodejs-github-bot

This comment has been minimized.

Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/http.md Outdated
@MylesBorins
Copy link
Member

left a comment

LGTM

a couple of doc nits

Show resolved Hide resolved doc/api/http.md Outdated
Show resolved Hide resolved doc/api/http2.md Outdated
Show resolved Hide resolved doc/api/http2.md Outdated
@nodejs-github-bot

This comment has been minimized.

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@nodejs/lts this is good to squash and land. Let me know if anything else is needed from my side. Once this lands, I can prepare PRs for 10.x and 8.x as well.

@BridgeAR
Copy link
Member

left a comment

LGTM % comments.

Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/http.md Outdated

@BridgeAR BridgeAR requested a review from Trott May 21, 2019

@ofrobots ofrobots force-pushed the ofrobots:http-server-timeout-env branch from 2a456f6 to 0759a63 May 21, 2019

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@nodejs-github-bot

This comment has been minimized.

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@nodejs/build: the CI lite built something other than this PR. Did I do something wrong, or is it a problem with a CI-lite?

@Trott

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@nodejs/build: the CI lite built something other than this PR. Did I do something wrong, or is it a problem with a CI-lite?

Looks like it built this PR to me, but I may be missing something you're seeing. What makes you say CI-Lite built the wrong thing?

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@Trott looking at the console output I see:

10:56:46  Fetching changes from the remote Git repository
10:56:46   > git rev-parse --is-inside-work-tree # timeout=10
10:56:46   > git config remote.origin.url https://github.com/nodejs/node.git # timeout=10
10:56:46  Fetching upstream changes from https://github.com/nodejs/node.git
10:56:46   > git --version # timeout=10
10:56:46   > git fetch --tags --force --progress https://github.com/nodejs/node.git +refs/heads/*:refs/remotes/origin/* # timeout=10
10:56:51  Checking out Revision 46d8af5e618554fcac89e4b15bf06f81de36ef54 (refs/remotes/origin/master)
10:56:51  Commit message: "deps: upgrade to libuv 1.29.1"
10:56:51  [Pipeline] }

This shows a commit that landed on master whereas this PR is targeting 12.x. This suggests to me that CI lite might be hard coded to pick up the master branch rather than the PR head?

@ofrobots ofrobots force-pushed the ofrobots:http-server-timeout-env branch 2 times, most recently from bbd99ec to 9d8d3ca May 22, 2019

http, http2: flag for overriding server timeout
Make it possible to override the default http server timeout. Ideally
there should be no server timeout - as done on the master branch. This
is a non-breaking way to enable platform providers to override the
value.

Ref: #27558
Ref: #27556

@ofrobots ofrobots force-pushed the ofrobots:http-server-timeout-env branch from 9d8d3ca to 4f093e8 May 22, 2019

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Landed in 588fd0c.

@ofrobots ofrobots closed this May 23, 2019

@ofrobots ofrobots deleted the ofrobots:http-server-timeout-env branch May 23, 2019

ofrobots added a commit that referenced this pull request May 23, 2019

http, http2: flag for overriding server timeout
Make it possible to override the default http server timeout. Ideally
there should be no server timeout - as done on the master branch. This
is a non-breaking way to enable platform providers to override the
value.

Ref: #27558
Ref: #27556

PR-URL: #27704
Refs: #27558
Refs: #27556
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@refack

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Thanks for the heads up.

the CI lite built something other than this PR.

This shows a commit that landed on master whereas this PR is targeting 12.x. This suggests to me that CI lite might be hard coded to pick up the master branch rather than the PR head?

That ref is just used to detect which MAJOR version of node is used (which indeed was wrong), but the tests were run against this PR's ref:
https://ci.nodejs.org/job/node-test-commit-linuxone/13487/
https://ci.nodejs.org/job/node-linter/3531/console

Ref for version detection, fixed:
https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3625/console
picked 4f093e8

15:19:00   > git fetch --tags --force --progress https://github.com/nodejs/node.git refs/pull/27704/head:refs/remotes/origin/_jenkins_local_branch # timeout=10
15:19:03  Checking out Revision 4f093e879b110771b586b701ddd2e9438a9e0eb9 (refs/remotes/origin/_jenkins_local_branch)
15:19:04  Commit message: "http, http2: flag for overriding server timeout"
15:19:04  [Pipeline] }
15:19:04  [Pipeline] // stage
15:19:04  [Pipeline] stage
15:19:04  [Pipeline] { (Run tests)
15:19:04  [Pipeline] script
15:19:04  [Pipeline] {
15:19:04  [Pipeline] sh
15:19:03   > git rev-parse refs/remotes/origin/_jenkins_local_branch^{commit} # timeout=10
15:19:03   > git rev-parse refs/remotes/origin/refs/heads/_jenkins_local_branch^{commit} # timeout=10
15:19:03   > git config core.sparsecheckout # timeout=10
15:19:03   > git checkout -f 4f093e879b110771b586b701ddd2e9438a9e0eb9 # timeout=10
15:19:05  + python tools/getnodeversion.py
15:19:05  [Pipeline] echo
15:19:05  Detected node version: 12.3.2
15:19:05  [Pipeline] echo
15:19:05  NODE_MAJOR_VERSION=12

ofrobots added a commit to ofrobots/node that referenced this pull request May 28, 2019

http, http2: flag for overriding server timeout
Make it possible to override the default http server timeout. Ideally
there should be no server timeout - as done on the master branch. This
is a non-breaking way to enable platform providers to override the
value.

PR-URL: nodejs#27704
Refs: nodejs#27558
Refs: nodejs#27556
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos added a commit that referenced this pull request Jun 3, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) #27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    #27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) #27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) #27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) #27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    #27933.

PR-URL: TODO

@targos targos referenced this pull request Jun 3, 2019

Merged

v12.4.0 release proposal #28040

targos added a commit that referenced this pull request Jun 3, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) #27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    #27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) #27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) #27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) #27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    #27933.

PR-URL: #28040

pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Jun 4, 2019

2019-06-04, Version 12.4.0 (Current)
Notable changes:

* doc:
  * The JSON variant of the API documentation is no longer experimental
    (Rich Trott) nodejs#27842.
* esm:
  * JSON module support is always enabled under
    `--experimental-modules`. The `--experimental-json-modules` flag
    has been removed (Myles Borins)
    nodejs#27752.
* http,http2:
  * A new flag has been added for overriding the default HTTP server
    socket timeout (which is two minutes). Pass
    `--http-server-default-timeout=milliseconds`
    or `--http-server-default-timeout=0` to respectively change or
    disable the timeout. Starting with Node.js 13.0.0, the timeout will
    be disabled by default
    (Ali Ijaz Sheikh) nodejs#27704.
* inspector:
  * Added an experimental `--heap-prof` flag to start the V8 heap
    profiler on startup and write the heap profile to disk before exit
    (Joyee Cheung) nodejs#27596.
* stream:
  * The `readable.unshift()` method now correctly converts strings to
    buffers. Additionally, a new optional argument is accepted to
    specify the string's encoding, such as `'utf8'` or `'ascii'`
    (Marcos Casagrande) nodejs#27194.
* v8:
  * The object returned by `v8.getHeapStatistics()` has two new
    properties: `number_of_native_contexts` and
    `number_of_detached_contexts` (Yuriy Vasiyarov)
    nodejs#27933.

PR-URL: nodejs#28040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.