http: remove default 'timeout' listener on upgrade #26030

In my opinion it doesn't make sense to emit the 'timeout' event or even trying to do it on the http.IncomingMessage, http.ServerResponse or http.ClientRequest instances after the protocol has been upgraded.

An argument could be made, however, for the 'timeout' event on the http.Server instance.

addaleax · 2019-03-01T10:07:05Z

@lpinca @Fishrock123 Any further thoughts? Should this land?

lpinca · 2019-03-01T10:10:28Z

I'm still in favor or I would have already closed it.
cc: @nodejs/http

mcollina

LGTM

lpinca · 2019-03-07T16:39:08Z

If there are no objections, I will land this tomorrow.

lpinca · 2019-03-08T19:01:42Z

Nvm, I can't land this as it needs another TSC approval.

lpinca · 2019-03-08T19:02:07Z

cc: @nodejs/tsc

Trott · 2019-03-08T23:34:57Z

I'm not entirely convinced this is a good idea...

@Fishrock123 Please (a) indicate if this is a blocking objection or not, and (b) explain your concern (because this is a conversation, not a vote).

Fishrock123 · 2019-03-21T23:33:26Z

Kinda an objection? I mean this just seems wrong to me.

You still have a connection. The remote could go inactive. Now you've leaked a connection. By default.

lpinca · 2019-03-22T06:33:55Z

@Fishrock123 it is developer responsibility to decide what to do with that connection, after the 'upgrade' or 'connect' event is emitted. A developer might want to use a custom timeout handling logic but if there isn't a handler for the 'timeout' event on the http.IncomingMessage, http.ServerResponse, or the http.Server (which doesn't make sense as you are now dealing with a free socket) the socket is destroyed.

No timeout is set on the socket by default when using net.Server so the connection leak argument is weak in my opinion.

lpinca · 2019-03-27T09:09:24Z

@Fishrock123 if your objection still holds true, can you make it explicit with "Changes requested"?
Ping @nodejs/tsc. I don't want this to stall. I'm fine with closing if there is no consensus.

lpinca · 2019-04-10T06:56:35Z

@Fishrock123?

lpinca · 2019-06-01T12:31:40Z

Now that James is a TSC member again this could theoretically land. Ping @nodejs/tsc again.

nodejs-github-bot · 2019-06-01T12:33:15Z

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

Trott · 2019-06-01T13:22:21Z

LGTM but needs a CITGM run?

Trott

LGTM but needs a CITGM run? @Fishrock123 Last chance to make it clear that your objection is blocking. Use "Request Changes" if you want to stop this from landing and have the TSC take it up. (Or leave an explicit comment saying your concerns are blocking concerns.)

nodejs-github-bot · 2019-06-02T06:46:01Z

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

lpinca · 2019-06-02T06:47:09Z

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

lpinca · 2019-06-08T11:36:58Z

Landed in d5577f0.

Remove the default listener of the `'timeout'` event from the socket before emitting the `'connect'` or `'upgrade'` event. PR-URL: #26030 Fixes: #23857 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[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

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 10, 2019

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2019

jasnell approved these changes Feb 11, 2019

View reviewed changes

http: remove default 'timeout' listener on upgrade

d406eb2

Remove the default listener of the `'timeout'` event from the socket before emitting the `'connect'` or `'upgrade'` event. Fixes: nodejs#23857

lpinca force-pushed the gh-23857 branch from 51fc6bb to d406eb2 Compare February 14, 2019 06:40

BridgeAR requested review from mcollina and mafintosh March 5, 2019 17:27

mcollina approved these changes Mar 5, 2019

View reviewed changes

Trott approved these changes Jun 1, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

lpinca closed this Jun 8, 2019

lpinca deleted the gh-23857 branch June 8, 2019 11:37

BethGriggs mentioned this pull request Sep 9, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: remove default 'timeout' listener on upgrade #26030

http: remove default 'timeout' listener on upgrade #26030

lpinca commented Feb 10, 2019

nodejs-github-bot commented Feb 10, 2019

lpinca commented Feb 10, 2019 •

edited

Loading

danbev commented Feb 13, 2019

addaleax commented Feb 14, 2019

lpinca commented Feb 14, 2019

lpinca commented Feb 14, 2019

Fishrock123 commented Feb 14, 2019

lpinca commented Feb 14, 2019 •

edited

Loading

addaleax commented Mar 1, 2019

lpinca commented Mar 1, 2019

mcollina left a comment

lpinca commented Mar 7, 2019

lpinca commented Mar 8, 2019

lpinca commented Mar 8, 2019

Trott commented Mar 8, 2019

Fishrock123 commented Mar 21, 2019

lpinca commented Mar 22, 2019 •

edited

Loading

lpinca commented Mar 27, 2019 •

edited

Loading

lpinca commented Apr 10, 2019

lpinca commented Jun 1, 2019

nodejs-github-bot commented Jun 1, 2019

Trott commented Jun 1, 2019

Trott left a comment

This comment has been minimized.

nodejs-github-bot commented Jun 2, 2019

lpinca commented Jun 2, 2019 •

edited

Loading

lpinca commented Jun 8, 2019

http: remove default 'timeout' listener on upgrade #26030

http: remove default 'timeout' listener on upgrade #26030

Conversation

lpinca commented Feb 10, 2019

Checklist

nodejs-github-bot commented Feb 10, 2019

lpinca commented Feb 10, 2019 • edited Loading

danbev commented Feb 13, 2019

addaleax commented Feb 14, 2019

lpinca commented Feb 14, 2019

lpinca commented Feb 14, 2019

Fishrock123 commented Feb 14, 2019

lpinca commented Feb 14, 2019 • edited Loading

addaleax commented Mar 1, 2019

lpinca commented Mar 1, 2019

mcollina left a comment

Choose a reason for hiding this comment

lpinca commented Mar 7, 2019

lpinca commented Mar 8, 2019

lpinca commented Mar 8, 2019

Trott commented Mar 8, 2019

Fishrock123 commented Mar 21, 2019

lpinca commented Mar 22, 2019 • edited Loading

lpinca commented Mar 27, 2019 • edited Loading

lpinca commented Apr 10, 2019

lpinca commented Jun 1, 2019

nodejs-github-bot commented Jun 1, 2019

Trott commented Jun 1, 2019

Trott left a comment

Choose a reason for hiding this comment

This comment has been minimized.

nodejs-github-bot commented Jun 2, 2019

lpinca commented Jun 2, 2019 • edited Loading

lpinca commented Jun 8, 2019

lpinca commented Feb 10, 2019 •

edited

Loading

lpinca commented Feb 14, 2019 •

edited

Loading

lpinca commented Mar 22, 2019 •

edited

Loading

lpinca commented Mar 27, 2019 •

edited

Loading

lpinca commented Jun 2, 2019 •

edited

Loading