stream: use callback to properly propagate error #29179

ronag · 2019-08-17T11:12:30Z

The stream will be destroyed upstream through the proper error flow.

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

lib/internal/stream_base_commons.js

Trott · 2019-08-19T16:43:53Z

@nodejs/streams

nodejs-github-bot · 2019-08-19T16:44:29Z

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

mcollina · 2019-08-19T17:07:40Z

Can you add a unit test for this?

Trott

This appears to break parallel/test-tls-hello-parser-failure on macOS, SmartOS, and possibly others.

Trott · 2019-08-19T17:13:06Z

Here's the test failing on FreeBSD. It also failed on macOS and SmartOS. Might be failing elsewhere too, as CI is still running....

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-tls-hello-parser-failure.js:64:12)
    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:371:15)
    at Socket.emit (events.js:209:13)
    at TCP.<anonymous> (net.js:588:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}

ronag · 2019-08-19T19:04:51Z

@mcollina: This is a bit over my head in terms of unit tests... I think the observed behaviour will be the same. It's just more "correct" in terms of flow.

ronag · 2019-08-20T23:13:14Z

@Trott: tests pass now, this needed a little extra fiddling to follow the "proper" stream logic.

In particular we need the socket to be automatically destroyed on error. This also means that the tests can't expect the socket to continue to function once an error has been caused. I had to split one of those test into two separate files.

This probably needs a semver-major...

test fixed

nodejs-github-bot · 2019-08-21T03:17:19Z

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

lib/_stream_writable.js

lib/internal/stream_base_commons.js

test/parallel/test-net-connect-buffer.js

test/parallel/test-net-connect-buffer2.js

nodejs-github-bot · 2019-08-21T21:44:38Z

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

Trott · 2020-02-05T19:23:26Z

@Trott: Can I get a second opinion on the CI failures on Windows. They seem legit don't they?

They do seem legit. After we get the security release out and the CI comes out of embargo, we can try a full rebuild to make sure.

nodejs-github-bot · 2020-02-08T22:29:40Z

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

ronag · 2020-02-17T15:58:54Z

This needs help with sorting out Windows failures.

ronag · 2020-02-17T23:47:14Z

This is blocked by #31806 in order to sort out Windows failures

nodejs-github-bot · 2020-02-18T23:48:55Z

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

The stream will be destroyed upstream through the proper error flow.

nodejs-github-bot · 2020-04-03T08:13:10Z

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

ronag · 2020-04-03T08:13:21Z

I think this might finally be landable, @mcollina @jasnell @BridgeAR PTAL

nodejs-github-bot · 2020-04-03T08:42:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30416/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2333/

mcollina

lgtm

The stream will be destroyed upstream through the proper error flow. PR-URL: #29179 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

ronag · 2020-04-03T17:01:21Z

Landed in 8f86986

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 force-pushed the stream-use-callback branch from 0068306 to d9fc519 Compare August 17, 2019 11:17

ronag mentioned this pull request Aug 17, 2019

stream: destroy with callback #29180

Closed

5 tasks

mscdex reviewed Aug 17, 2019

View reviewed changes

lib/internal/stream_base_commons.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-use-callback branch from d9fc519 to 7018652 Compare August 17, 2019 14:55

Trott previously requested changes Aug 19, 2019

View reviewed changes

ronag force-pushed the stream-use-callback branch 7 times, most recently from 20fac88 to bef7095 Compare August 20, 2019 23:38

mcollina added net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. labels Aug 21, 2019

mcollina reviewed Aug 21, 2019

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

lib/_stream_writable.js Outdated Show resolved Hide resolved

lib/internal/stream_base_commons.js Show resolved Hide resolved

test/parallel/test-net-connect-buffer.js Outdated Show resolved Hide resolved

jasnell reviewed Aug 21, 2019

View reviewed changes

test/parallel/test-net-connect-buffer2.js Outdated Show resolved Hide resolved

jasnell reviewed Aug 21, 2019

View reviewed changes

test/parallel/test-net-connect-buffer2.js Show resolved Hide resolved

ronag force-pushed the stream-use-callback branch 3 times, most recently from c8e66f6 to ef9e30f Compare August 21, 2019 22:08

ronag added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2020

ronag force-pushed the stream-use-callback branch from 6c9b39b to a89f557 Compare February 18, 2020 23:40

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-use-callback branch from a89f557 to 945c44b Compare February 18, 2020 23:48

ronag mentioned this pull request Mar 1, 2020

stream: make stream.destroy with callback API public #32021

Closed

4 tasks

stream: use callback to properly propagate error

6b86519

The stream will be destroyed upstream through the proper error flow.

ronag force-pushed the stream-use-callback branch from 945c44b to 6b86519 Compare April 3, 2020 06:42

ronag removed the blocked PRs that are blocked by other issues or PRs. label Apr 3, 2020

mcollina approved these changes Apr 3, 2020

View reviewed changes

ronag requested review from BridgeAR, Trott and jasnell April 3, 2020 10:13

jasnell approved these changes Apr 3, 2020

View reviewed changes

ronag closed this Apr 3, 2020

BethGriggs mentioned this pull request Sep 1, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: use callback to properly propagate error #29179

stream: use callback to properly propagate error #29179

ronag commented Aug 17, 2019 •

edited

Trott commented Aug 19, 2019

nodejs-github-bot commented Aug 19, 2019

mcollina commented Aug 19, 2019

Trott left a comment

Trott commented Aug 19, 2019

ronag commented Aug 19, 2019

ronag commented Aug 20, 2019 •

edited

nodejs-github-bot commented Aug 21, 2019

nodejs-github-bot commented Aug 21, 2019

Trott commented Feb 5, 2020

nodejs-github-bot commented Feb 8, 2020

ronag commented Feb 17, 2020

ronag commented Feb 17, 2020

This comment has been minimized.

nodejs-github-bot commented Feb 18, 2020

nodejs-github-bot commented Apr 3, 2020

ronag commented Apr 3, 2020

nodejs-github-bot commented Apr 3, 2020 •

edited by ronag

mcollina left a comment

ronag commented Apr 3, 2020

Navigation Menu

stream: use callback to properly propagate error #29179

stream: use callback to properly propagate error #29179

Conversation

ronag commented Aug 17, 2019 • edited

Checklist

Trott commented Aug 19, 2019

nodejs-github-bot commented Aug 19, 2019

mcollina commented Aug 19, 2019

Trott left a comment

Choose a reason for hiding this comment

Trott commented Aug 19, 2019

ronag commented Aug 19, 2019

ronag commented Aug 20, 2019 • edited

nodejs-github-bot commented Aug 21, 2019

nodejs-github-bot commented Aug 21, 2019

Trott commented Feb 5, 2020

nodejs-github-bot commented Feb 8, 2020

ronag commented Feb 17, 2020

ronag commented Feb 17, 2020

This comment has been minimized.

nodejs-github-bot commented Feb 18, 2020

nodejs-github-bot commented Apr 3, 2020

ronag commented Apr 3, 2020

nodejs-github-bot commented Apr 3, 2020 • edited by ronag

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Apr 3, 2020

ronag commented Aug 17, 2019 •

edited

ronag commented Aug 20, 2019 •

edited

nodejs-github-bot commented Apr 3, 2020 •

edited by ronag