http, http2: remove default server timeout #27558

ofrobots · 2019-05-04T00:13:03Z

Timing out and closing the socket after two minutes have elapsed is
surprising and problematic for users. This behavior was specific to
Node.js, and doesn't seem to be common in other language runtimes.

Fixes: #27556

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-05-04T00:13:05Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3437

doc/api/http.md

apapirovski · 2019-05-04T18:17:33Z

In theory, I’m in favor of this change for all the reasons outlined in the original issue. That said, I worry about just how breaking this could be for some users... and not sure there’s a good way to quantify that side of it.

mcollina

LGTM

lib/https.js

lib/internal/http2/core.js

Timing out and closing the socket after two minutes have elapsed is surprising and problematic for users. This behavior was specific to Node.js, and doesn't seem to be common in other language runtimes. Fixes: nodejs#27556

ofrobots · 2019-05-06T22:22:31Z

@apapirovski

That said, I worry about just how breaking this could be for some users... and not sure there’s a good way to quantify that side of it.

My expectation is that this is not very breaking for users. I can imagine some impact on buggy applications that leak sockets, but I'm not very concerned about that case. As a semver-major, this will go out in an odd-numbered release which lets us manage impact, and revert, if this is very breaky?

nodejs-github-bot · 2019-05-06T22:24:01Z

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

nodejs-github-bot · 2019-05-07T18:21:18Z

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

ofrobots · 2019-05-07T19:41:52Z

async-hooks/test-graph.http.js failed on one arm machine. This is relevant, and probably due to an overly specific async hooks graph.

ofrobots · 2019-05-07T21:25:49Z

With this PR the arm failure is flaky with a 0.5% chance of failure. Upon investigation it turns out that the test is flaky on master as well, but with a 0.1% chance of failure.

ofrobots · 2019-05-08T00:19:13Z

The flakiness is due to the following conditional timer we create to cache timestamps for http responses:

node/lib/internal/http.js

Line 22 in e006a85

setUnrefTimeout(resetCache, 1000 - d.getMilliseconds());

If we are unlucky (and we are unlucky 0.1% or 0.5% of the time on this ARM machine), this ends up creating a Timeout async resource and the test fails because it did not expect that particular Timeout resource in the graph. I'll open a separate issue/PR to get the test fixed. I don't think this should block this PR. Let me know if anyone disagrees.

Trott · 2019-05-08T03:45:10Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1846/

Flarna · 2019-05-08T05:46:39Z

I think this flaky test is related to adding a check for expected types in verify-graph.js done via #27477.
Before this the verify-graph tests were not failing even if all of the expected events were not present (that time it was HTTPParser which was renamed).

ofrobots · 2019-05-08T19:01:33Z

Here's a comparison CTIGM run for master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1847/. Nothing in the delta looked suspicious, but fastify saw some EADDRINUSE in use errors. These are likely infrastructure issues, but I manually tested fastify test suite against this branch for good measure. Things seem to be green.

ofrobots · 2019-05-08T21:39:48Z

Thanks @Flarna, that makes sense. I have split the flaky test discussion into #27617.

I am planning to land this tomorrow (Thu).

gRPC has its own mechanisms for timing out a request, and requests should not timeout by default. Furthermore, the default timeout was removed from Node.js. Refs: nodejs/node#27558

gRPC has its own mechanisms for timing out a request. Furthermore, the default timeout was removed from Node.js Refs: nodejs/node#27558

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. Backport-PR-URL: #27939 PR-URL: #27704 Refs: #27558 Refs: #27556 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

- Remove default connection timeout - nodejs/node#27558. - Add client error listener.

The default timeout was removed from the HTTP module in node 13.x: nodejs/node#27558. I believe this is the most likely cause of fd leaks when running under current node versions.

nodejs-github-bot added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. https Issues or PRs related to the https subsystem. labels May 4, 2019

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 4, 2019

ofrobots force-pushed the http-timeout branch from 99c7dfd to 5a5f657 Compare May 4, 2019 00:20

Trott reviewed May 4, 2019

View reviewed changes

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

mcollina approved these changes May 5, 2019

View reviewed changes

addaleax approved these changes May 5, 2019

View reviewed changes

Trott reviewed May 5, 2019

View reviewed changes

lib/https.js Show resolved Hide resolved

Trott reviewed May 5, 2019

View reviewed changes

lib/internal/http2/core.js Show resolved Hide resolved

Trott reviewed May 5, 2019

View reviewed changes

lib/internal/http2/core.js Show resolved Hide resolved

Trott reviewed May 5, 2019

View reviewed changes

lib/internal/http2/core.js Show resolved Hide resolved

http, http2: remove default server timeout

52594f4

Timing out and closing the socket after two minutes have elapsed is surprising and problematic for users. This behavior was specific to Node.js, and doesn't seem to be common in other language runtimes. Fixes: nodejs#27556

ofrobots force-pushed the http-timeout branch from 5a5f657 to 52594f4 Compare May 6, 2019 21:46

Trott approved these changes May 7, 2019

View reviewed changes

ofrobots mentioned this pull request May 7, 2019

test: better assertion for async hook tests #27601

Closed

2 tasks

ofrobots force-pushed the http-timeout branch from cd7a81c to 52594f4 Compare May 8, 2019 00:19

ofrobots mentioned this pull request May 8, 2019

test/async-hooks/test-graph.http.js flaky #27617

Closed

ofrobots mentioned this pull request May 28, 2019

[10.x] http, http2: flag for overriding server timeout #27939

Closed

4 tasks

cjihrig mentioned this pull request Jun 4, 2019

Ping Frame / Keep alive cjihrig/grpc-server-js#1

Closed

cjihrig added a commit to cjihrig/grpc-node that referenced this pull request Jun 4, 2019

grpc-js: disable http2 server timeout

93ea51f

gRPC has its own mechanisms for timing out a request. Furthermore, the default timeout was removed from Node.js Refs: nodejs/node#27558

cjihrig mentioned this pull request Jun 4, 2019

grpc-js: disable http2 server timeout grpc/grpc-node#903

Merged

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

JeremyTCD added a commit to JeringTech/Javascript.NodeJS that referenced this pull request Dec 6, 2019

Fix connection drops

ce7ded4

- Remove default connection timeout - nodejs/node#27558. - Add client error listener.

JeremyTCD mentioned this pull request Dec 6, 2019

Fix connection drops JeringTech/Javascript.NodeJS#61

Merged

JeremyTCD added a commit to JeringTech/Javascript.NodeJS that referenced this pull request Dec 6, 2019

Fix connection drops

cdc946f

- Remove default connection timeout - nodejs/node#27558. - Add client error listener.

aduh95 mentioned this pull request Jan 16, 2020

Hardcoded socket timeout #31378

Closed

This was referenced Feb 8, 2020

doc: fix https default server timeout #31692

Closed

Add connectionTimeout option fastify/fastify#2086

Merged

jonchurch mentioned this pull request Feb 14, 2020

Default http timeout changed in Node 13 stripe/stripe-node#806

Closed

This was referenced Feb 24, 2020

Default Node.js HTTP Timeout changes in v13 dnsimple/dnsimple-node#25

Closed

Node.js v13+ default timeout changed goshippo/shippo-node-client#55

Closed

This was referenced Apr 8, 2020

Git: Support editing the commit message in a text editor microsoft/vscode#30562

Closed

A full editor can be used as git commit message editor microsoft/vscode#95266

Merged

AllanZhengYP mentioned this pull request May 27, 2020

Set default H2 session timeout aws/aws-sdk-js-v3#1206

Closed

RandomByte mentioned this pull request Dec 15, 2020

ui5-server: Add timeout property to settings SAP/ui5-tooling#471

Closed

UnKnoWn-Consortium mentioned this pull request Jan 23, 2021

IPFS Desktop stuck on "IPFS is Starting" ipfs/ipfs-desktop#1723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http, http2: remove default server timeout #27558

http, http2: remove default server timeout #27558

ofrobots commented May 4, 2019 •

edited

Loading

nodejs-github-bot commented May 4, 2019

apapirovski commented May 4, 2019

mcollina left a comment

ofrobots commented May 6, 2019

nodejs-github-bot commented May 6, 2019

nodejs-github-bot commented May 7, 2019

ofrobots commented May 7, 2019

ofrobots commented May 7, 2019

ofrobots commented May 8, 2019

Trott commented May 8, 2019

Flarna commented May 8, 2019

ofrobots commented May 8, 2019

ofrobots commented May 8, 2019

http, http2: remove default server timeout #27558

http, http2: remove default server timeout #27558

Conversation

ofrobots commented May 4, 2019 • edited Loading

Checklist

nodejs-github-bot commented May 4, 2019

apapirovski commented May 4, 2019

mcollina left a comment

Choose a reason for hiding this comment

ofrobots commented May 6, 2019

nodejs-github-bot commented May 6, 2019

nodejs-github-bot commented May 7, 2019

ofrobots commented May 7, 2019

ofrobots commented May 7, 2019

ofrobots commented May 8, 2019

Trott commented May 8, 2019

Flarna commented May 8, 2019

ofrobots commented May 8, 2019

ofrobots commented May 8, 2019

ofrobots commented May 4, 2019 •

edited

Loading