http: `'aborted'` IncomingMessage should `'error'` #33172

ronag · 2020-04-30T18:47:42Z

IncomingMessage emits 'aborted' instead of 'error' which causes confusion when it is used as a regular stream, i.e. if a function working on streams is passed an IncomingMessage it might not work properly unless this is taken into account.

Furthermore, there is some confusion in the ecosystem whether to and when to listen to 'aborted' vs 'error'. Sometimes leading to mistakes where the wrong one is subscribed (i.e. res vs req).

This PR tries to fix this by emitting 'error' after 'aborted', but only if there is an 'error' listener (i.e. no uncaught exception) in order to avoid breaking the ecosystem.

Refs: nodejs/web-server-frameworks#41
Refs: #28677
Fixes: #28172

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

ronag · 2020-04-30T18:49:28Z

@nodejs/web-server-frameworks @nodejs/http

ronag · 2020-04-30T19:15:00Z

I would like to also doc deprecate the 'aborted' event but maybe that's a separate discussion/PR?

mcollina

lgtm

doc/api/http.md

Trott

LGTM if CITGM results are unsurprising

nodejs-github-bot · 2020-05-01T07:46:15Z

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

nodejs-github-bot · 2020-05-01T08:24:09Z

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

lpinca · 2020-05-01T08:33:51Z

lib/_http_client.js

    if (!res.complete) {
      res.aborted = true;
      res.emit('aborted');
+      if (res.listenerCount('error') > 0) {
+        res.emit('error', connResetException('socket hang up'));


Doesn't this change the meaning of the "socket hang up" error? If I'm not wrong it was only emitted if no 'response' event was emitted.

changed to 'aborted'

lpinca · 2020-05-01T08:35:24Z

lib/_http_server.js

    req.aborted = true;
    req.emit('aborted');
+    if (req.listenerCount('error') > 0) {
+      req.emit('error', connResetException('socket hang up'));


Are we ok with using "socket hang up" on the server? It never existed before...

I'm not all that concerned with the message, more the code. I think we used 'aborted' as a message in the other PR. Would you prefer that?

Yes, how about creating a new and dedicated ERR_HTTP_ABORTED error or something like that?

I don't have a desire to make http streams 100% compliant with other streams so I'm neutral here. As long as the 'error' or the 'aborted' listeners are not mandatory I'm fine with it.

Yes, how about creating a new and dedicated ERR_HTTP_ABORTED error or something like that?

I would prefer the error type to be consistent with OutgoingMessage. I'll change the message.

changed to 'aborted'

While it's true that the ECONNRESET code is used also for the "socket hang up" error (probably for compatibility reasons), I think it is misleading as there is no guarantee that the socket was closed with that error.

So I guess it's a tradeoff between "correctness" and "consistency". For once I'm not on the side of "correctness". However, if you have a strong opinion on it I would not be opposed to changing it to what you propose.

@mcollina: Do you have any thoughts on this matter?

I would also say that it is also not consistent as it's a new and different error but I don't care that much.

lpinca · 2020-05-01T08:37:40Z

I would prefer to use a different error.

Server requests aka. IncomingMessage emits 'aborted' instead of 'error' which causes confusion when the object is used as a regular stream, i.e. if functions working on streams are passed a server request object they might not work properly unless they take this into account. Refs: nodejs/web-server-frameworks#41

Client responses aka. IncomingMessage emits 'aborted' instead of 'error' which causes confusion when the object is used as a regular stream, i.e. if functions working on streams are passed a client response object they might not work properly unless they take this into account. Refs: nodejs/web-server-frameworks#41 Fixes: nodejs#28172

ronag · 2020-05-02T07:39:38Z

@lpinca Just wanted to make sure whether your concern is a blocker for you or whether this can land in its current form?

lpinca · 2020-05-02T07:54:13Z

It's not a blocker.

ronag · 2020-05-02T08:10:44Z

I believe @nodejs/web-server-frameworks was positive to this change based on the last meeting.

Just doing another ping for @nodejs/http in case they have any further comments.

Trott · 2020-05-02T13:01:57Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2371/ (queued)
CITGM against master for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2370/

ronag · 2020-05-03T16:12:00Z

CITGM looks good

BridgeAR

Code is looking good to me, just left some nits. I just can't comment much on the actual change.

test/parallel/test-http-abort-client.js

test/parallel/test-http-aborted.js

test/parallel/test-http-client-aborted-event.js

test/parallel/test-http-client-spurious-aborted.js

test/parallel/test-http-outgoing-message-capture-rejection.js

BridgeAR · 2020-05-03T16:30:51Z

doc/api/http.md

-`response` object does not emit an `'error'` event but instead emits the
-`'aborted'` event.
+For backward compatibility, `res` will only emit `'error'` if there is an
+`'error'` listener registered.


Should the aborted part not be kept? If I understood correct, that's still the case if there's no error listener attached.

I'm not sure, the previous paragraph was there because we didn't emit 'error'. Since this is only informative I would rather not mention something that we would like to deprecate in the future.

Co-authored-by: Ruben Bridgewater <[email protected]>

nodejs-github-bot · 2020-05-03T16:34:31Z

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

nodejs-github-bot · 2020-05-04T04:34:48Z

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

nodejs-github-bot · 2020-05-06T20:38:40Z

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

addaleax · 2020-05-09T06:04:28Z

CI is red, unfortunately

nodejs-github-bot · 2020-05-09T08:59:56Z

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

ronag · 2020-05-09T10:26:10Z

@addaleax PTAL

ronag · 2020-05-10T09:31:37Z

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

Server requests aka. IncomingMessage emits 'aborted' instead of 'error' which causes confusion when the object is used as a regular stream, i.e. if functions working on streams are passed a server request object they might not work properly unless they take this into account. Refs: nodejs/web-server-frameworks#41 PR-URL: #33172 Fixes: #28172 Refs: #28677 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

ronag · 2020-05-10T20:12:30Z

Landed in 8a6fab0

Refs: nodejs#33120 Refs: nodejs#33172

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

ronag added the http Issues or PRs related to the http subsystem. label Apr 30, 2020

ronag changed the title ~~http: aborted error~~ http: 'aborted' IncomingMessage should 'error' Apr 30, 2020

ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 30, 2020

ronag force-pushed the http-aborted-error branch from d2050b9 to 8102163 Compare April 30, 2020 19:48

mcollina approved these changes Apr 30, 2020

View reviewed changes

ronag requested a review from a team April 30, 2020 21:54

Trott reviewed May 1, 2020

View reviewed changes

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

Trott approved these changes May 1, 2020

View reviewed changes

ronag requested a review from lpinca May 1, 2020 08:27

lpinca reviewed May 1, 2020

View reviewed changes

ronag added 2 commits May 1, 2020 10:55

ronag force-pushed the http-aborted-error branch 2 times, most recently from 758414f to 57c3542 Compare May 1, 2020 08:57

fixup! http: emit 'error' on aborted client response

0c54b5f

ronag force-pushed the http-aborted-error branch from 57c3542 to 0c54b5f Compare May 1, 2020 11:23

ronag requested a review from jasnell May 2, 2020 08:09

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2020

BridgeAR reviewed May 3, 2020

View reviewed changes

Apply suggestions from code review

f792724

Co-authored-by: Ruben Bridgewater <[email protected]>

fixup

49baa70

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2020

fixup

fb43aa2

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2020

addaleax approved these changes May 9, 2020

View reviewed changes

ronag closed this May 10, 2020

ronag mentioned this pull request May 10, 2020

http: deprecate aborted property and event #33345

Closed

4 tasks

ronag added a commit to nxtedition/node that referenced this pull request May 10, 2020

http: deprecate aborted property and event

304935f

Refs: nodejs#33120 Refs: nodejs#33172

ronag mentioned this pull request May 28, 2020

http: IncomingMessage does not emit error on premature close nodejs/web-server-frameworks#41

Closed

BethGriggs mentioned this pull request Sep 1, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

bl-ue mentioned this pull request Jun 8, 2021

http2 stream.end error #36860

Open

alolis mentioned this pull request Aug 11, 2021

Memory leak when a stream in the pipeline gets destroyed unexpectedly tomas/needle#368

Closed

flotwig mentioned this pull request Nov 9, 2021

feat(deps): update dependency electron to v15 🌟 cypress-io/cypress#18317

Merged

nflaig mentioned this pull request Jul 24, 2023

chore: remove redundant eventstream error listener ChainSafe/lodestar#5798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: `'aborted'` IncomingMessage should `'error'` #33172

http: `'aborted'` IncomingMessage should `'error'` #33172

ronag commented Apr 30, 2020 •

edited

Loading

ronag commented Apr 30, 2020

ronag commented Apr 30, 2020 •

edited

Loading

mcollina left a comment

Trott left a comment

nodejs-github-bot commented May 1, 2020

nodejs-github-bot commented May 1, 2020

lpinca May 1, 2020 •

edited

Loading

ronag May 1, 2020

lpinca May 1, 2020

ronag May 1, 2020 •

edited

Loading

lpinca May 1, 2020 •

edited

Loading

ronag May 1, 2020

ronag May 1, 2020

lpinca May 1, 2020 •

edited

Loading

ronag May 1, 2020

ronag May 1, 2020

lpinca May 1, 2020

lpinca commented May 1, 2020

ronag commented May 2, 2020

lpinca commented May 2, 2020

ronag commented May 2, 2020

Trott commented May 2, 2020

ronag commented May 3, 2020

BridgeAR left a comment

BridgeAR May 3, 2020

ronag May 3, 2020

nodejs-github-bot commented May 3, 2020

nodejs-github-bot commented May 4, 2020

nodejs-github-bot commented May 6, 2020

addaleax commented May 9, 2020

nodejs-github-bot commented May 9, 2020

ronag commented May 9, 2020

ronag commented May 10, 2020

ronag commented May 10, 2020

http: 'aborted' IncomingMessage should 'error' #33172

http: 'aborted' IncomingMessage should 'error' #33172

Conversation

ronag commented Apr 30, 2020 • edited Loading

Checklist

ronag commented Apr 30, 2020

ronag commented Apr 30, 2020 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 1, 2020

nodejs-github-bot commented May 1, 2020

lpinca May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag May 1, 2020 • edited Loading

Choose a reason for hiding this comment

lpinca May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented May 1, 2020

ronag commented May 2, 2020

lpinca commented May 2, 2020

ronag commented May 2, 2020

Trott commented May 2, 2020

ronag commented May 3, 2020

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 3, 2020

nodejs-github-bot commented May 4, 2020

nodejs-github-bot commented May 6, 2020

addaleax commented May 9, 2020

nodejs-github-bot commented May 9, 2020

ronag commented May 9, 2020

ronag commented May 10, 2020

ronag commented May 10, 2020

http: `'aborted'` IncomingMessage should `'error'` #33172

http: `'aborted'` IncomingMessage should `'error'` #33172

ronag commented Apr 30, 2020 •

edited

Loading

ronag commented Apr 30, 2020 •

edited

Loading

lpinca May 1, 2020 •

edited

Loading

ronag May 1, 2020 •

edited

Loading

lpinca May 1, 2020 •

edited

Loading

lpinca May 1, 2020 •

edited

Loading