stream: fix _final and 'prefinish' timing #32780

ronag · 2020-04-11T11:21:46Z

This PR fixes a few different things:

The timing of 'prefinish' depends on whether or not
_final is defined. In one case the event is emitted
synchronously with end() and otherwise asynchronously.

_final is currently unecessarily called asynchronously
which forces implementors to use 'prefinish' as a hack
to emulate synchronous behaviour. Furthermore, this hack
is subtly broken due to the above issue.

The stream should not finish if errored or destroyed
synchronously during the prefinish stage.

Refs: #31401
Refs: #32763 (comment)

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

test/parallel/test-fs-write-stream-end.js

lib/internal/fs/streams.js

ronag · 2020-04-11T14:53:42Z

lib/_stream_writable.js

+      // Backwards compat. Don't use state.sync. Some stream
+      // implemenations expect 'finish' to be emitted
+      // asynchronously relative to _final callback.
+      finishMaybe(stream, state, true);


Note compat comment

test/parallel/test-http2-client-rststream-before-connect.js

nodejs-github-bot · 2020-04-11T20:55:15Z

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

ronag · 2020-04-11T20:59:41Z

lib/internal/http2/core.js

  // Currently this status value is unused
  this.callback();
-  const stream = this.handle[kOwner];
-  if (stream)
-    stream[kMaybeDestroy]();


This assumes writableFinished === true after callback which is not necessarily true.

ronag · 2020-04-11T21:00:10Z

lib/_stream_writable.js

-      finishMaybe(stream, state);
+      // Backwards compat. Don't check state.sync here.
+      // Some stream assume 'finish' will be emitted
+      // asynchronously relative to _final callback.


Might be worth to revisit once _construct PR lands.

This is to support the following commonly used pattern:

_final(cb) ( if (!this.ready) { this.once('ready', () => this._final(cb)) // Unfortunately not wrapped in a nextTick } // cleanup... cb(); }

ronag · 2020-04-11T21:00:43Z

lib/_stream_writable.js

    } else {
+      // Backwards compat. Don't check needFinish() here.
+      // Some streams assume 'finish' will be emitted
+      // even if stream has been destroyed.


Might want to add a check for errored here in a follow up PR...

ronag · 2020-04-11T21:16:19Z

@mcollina compat cases resolved incl. comments in code, CI looks good. PTAL

ronag · 2020-04-11T21:40:02Z

@vweevers

This PR fixes a few different things: The timing of 'prefinish' depends on whether or not _final is defined. In on case the event is emitted synchronously with end() and otherwise asynchronously. _final is currently unecessarily called asynchronously which forces implementors to use 'prefinish' as a hack to emulate synchronous behaviour. Furthermore, this hack is subtly broken due to the above issue. The stream should not finish if errored or destroyed synchronously during the prefinish stage. Refs: nodejs#31401 Refs: nodejs#32763 (comment)

ronag · 2020-04-15T19:39:49Z

rebased to fix conflict

nodejs-github-bot · 2020-04-15T19:40:09Z

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

nodejs-github-bot · 2020-04-15T19:52:34Z

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

nodejs-github-bot · 2020-04-16T07:22:24Z

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

Trott · 2020-04-19T12:37:43Z

@lpinca Are you neutral on this change? +1? Somewhere in between?

As far as the second TSC approval goes, it looks good to me, and I can +1 it if it stalls, but I'd feel better if there were more reviews from folks with a better understanding of all the implications than I have. (On the other hand, @mcollina's +1 certainly fits that description!)

lpinca · 2020-04-19T16:58:43Z

I'm neutral.

This PR fixes a few different things: The timing of 'prefinish' depends on whether or not _final is defined. In on case the event is emitted synchronously with end() and otherwise asynchronously. _final is currently unecessarily called asynchronously which forces implementors to use 'prefinish' as a hack to emulate synchronous behaviour. Furthermore, this hack is subtly broken due to the above issue. Refs: #31401 Refs: #32763 (comment) PR-URL: #32780 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>

ronag · 2020-04-22T07:41:39Z

Landed in ea87809

ronag · 2020-04-22T07:41:46Z

@lpinca

Ensure that `'finish'` event is emitted. Refs: nodejs/node#32780

Keep the `WebSocket` connection open while the test is run. Refs: nodejs/node#32780

lpinca · 2020-04-22T08:28:37Z

I've merged websockets/ws#1736. Will cut a new version later today.

lpinca · 2020-04-22T17:14:27Z

I would prefer to delay the ws release a bit to see if I can include a fix for #32887 in it (can't get to it before the week end). Until then CIGTM will report regressions in ws. Let me know if it is a problem.

ronag · 2020-04-22T17:17:38Z

@MylesBorins FYI. Not sure who to ping on this. We have a known/ok CITGM error on ws post this PR. @lpinca is working on updating the ws test but there will be a few days delay (see above).

lpinca · 2020-04-22T17:25:28Z

cc: @nodejs/releasers

This is semver-major so it shouldn't be a problem.

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 stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 11, 2020

ronag requested a review from mcollina April 11, 2020 11:21

This comment has been minimized.

Sign in to view

mcollina requested changes Apr 11, 2020

View reviewed changes

test/parallel/test-fs-write-stream-end.js Outdated Show resolved Hide resolved

ronag commented Apr 11, 2020

View reviewed changes

lib/internal/fs/streams.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-finish branch 3 times, most recently from 52ad447 to 1514209 Compare April 11, 2020 14:31

ronag commented Apr 11, 2020

View reviewed changes

test/parallel/test-http2-client-rststream-before-connect.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-finish branch from d0b3e8e to 8155782 Compare April 11, 2020 19:40

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-finish branch from c38dca7 to 5a6c161 Compare April 11, 2020 20:01

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-finish branch from 2d89c91 to 7a7d831 Compare April 11, 2020 20:27

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-finish branch 3 times, most recently from 73a08ce to 0c7a5e8 Compare April 11, 2020 20:53

ronag commented Apr 11, 2020

View reviewed changes

ronag force-pushed the stream-finish branch from 3076154 to be9f726 Compare April 15, 2020 19:39

ronag added 2 commits April 15, 2020 21:47

fixup: comments

469766e

fixup: missing needFinish

571eb88

lpinca mentioned this pull request Apr 18, 2020

stream: avoid drain for sync streams #32887

Closed

4 tasks

ronag requested a review from jasnell April 18, 2020 19:08

Trott approved these changes Apr 22, 2020

View reviewed changes

ronag closed this Apr 22, 2020

lpinca added a commit to websockets/ws that referenced this pull request Apr 22, 2020

[fix] Call the _final() callback before destroying the stream

651d662

Ensure that `'finish'` event is emitted. Refs: nodejs/node#32780

lpinca added a commit to websockets/ws that referenced this pull request Apr 22, 2020

[test] Do not call duplex.end() prematurely

3730467

Keep the `WebSocket` connection open while the test is run. Refs: nodejs/node#32780

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: fix _final and 'prefinish' timing #32780

stream: fix _final and 'prefinish' timing #32780

ronag commented Apr 11, 2020 •

edited

Loading

This comment has been minimized.

ronag Apr 11, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Apr 11, 2020

ronag Apr 11, 2020

ronag Apr 11, 2020

ronag Apr 11, 2020 •

edited

Loading

ronag Apr 11, 2020

ronag commented Apr 11, 2020

ronag commented Apr 11, 2020

ronag commented Apr 15, 2020

nodejs-github-bot commented Apr 15, 2020

nodejs-github-bot commented Apr 15, 2020

nodejs-github-bot commented Apr 16, 2020 •

edited by ronag

Loading

Trott commented Apr 19, 2020 •

edited

Loading

lpinca commented Apr 19, 2020

ronag commented Apr 22, 2020

ronag commented Apr 22, 2020

lpinca commented Apr 22, 2020

lpinca commented Apr 22, 2020 •

edited

Loading

ronag commented Apr 22, 2020 •

edited

Loading

lpinca commented Apr 22, 2020 •

edited

Loading

stream: fix _final and 'prefinish' timing #32780

stream: fix _final and 'prefinish' timing #32780

Conversation

ronag commented Apr 11, 2020 • edited Loading

Checklist

This comment has been minimized.

ronag Apr 11, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Apr 11, 2020

ronag Apr 11, 2020

Choose a reason for hiding this comment

ronag Apr 11, 2020

Choose a reason for hiding this comment

ronag Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

ronag Apr 11, 2020

Choose a reason for hiding this comment

ronag commented Apr 11, 2020

ronag commented Apr 11, 2020

ronag commented Apr 15, 2020

nodejs-github-bot commented Apr 15, 2020

nodejs-github-bot commented Apr 15, 2020

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

Trott commented Apr 19, 2020 • edited Loading

lpinca commented Apr 19, 2020

ronag commented Apr 22, 2020

ronag commented Apr 22, 2020

lpinca commented Apr 22, 2020

lpinca commented Apr 22, 2020 • edited Loading

ronag commented Apr 22, 2020 • edited Loading

lpinca commented Apr 22, 2020 • edited Loading

ronag commented Apr 11, 2020 •

edited

Loading

ronag Apr 11, 2020 •

edited

Loading

nodejs-github-bot commented Apr 16, 2020 •

edited by ronag

Loading

Trott commented Apr 19, 2020 •

edited

Loading

lpinca commented Apr 22, 2020 •

edited

Loading

ronag commented Apr 22, 2020 •

edited

Loading

lpinca commented Apr 22, 2020 •

edited

Loading