stream: don't emit finish on error by ronag · Pull Request #28979 · nodejs/node · GitHub

I noticed some strange behaviour while working on #28978.

finish should not be emitted if a write has failed.

EDIT: This is blocking work on a few other PR's.

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

NOTE TO SELF: look into errorOrDestroy emitting async (emit-nextTick), needFinish should take into account errorEmitted.

ping @mcollina

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

@nodejs/streams

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

@Trott this is blocking another PR I'm preparing.

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

@Trott: sorry, can I get another CI on this one?

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

@Trott not sure what's going on here, e.g. TRAVIS CI linting is failing on files that hasn't changed?

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

After 'error' only 'close' should be emitted.

Note here. We need to split the test into two since once the socket is errored it no longer gets destroyed as part of emitting 'finish'.

can you rename the test and add a description?

@mcollina: Could I get a review request towards node/stream? This is blocking me on other stuff I'm working on.

LGTM

can you rename the test and add a description?

Renamed test. They both already have description?

@Trott: This needs another review. Ping node/stream?

@nodejs/tsc This is semver-major so needs a second TSC approval.

The change in behavior should be documented in streams.md

@jasnell: Updated docs.

@mcollina: A little guidance here. I added a should in the docs for 2 reasons:

I'm not yet 100% this is the case in core (need to look through the status of my other open and closed PR's)
Currently there is nothing that would stop a user from manually emitting these events and breaking the invariant. I've seen modules in the ecosystem that does this for various reasons.

Questions:

Does it make sense to add a should in the docs?
Does it make sense to enforce the invariant through overriding emit, i.e. ignore (or throw on) 'end' and 'finish' if state.errorEmitted?

Co-Authored-By: Rich Trott <rtrott@gmail.com>

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

@Trott: Anything missing here?

Here is a fresh CITGM run

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

LGTM

After 'error' only 'close' should be emitted. PR-URL: #28979 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in ba3be57

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

See nodejs/node#28979. Node streams have stopped emitting 'finish' after 'error' ocurred in Node v13, we have to adapt.

See nodejs/node#28979

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 5, 2019

ronag commented Aug 5, 2019

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

ronag commented Aug 5, 2019

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

ronag commented Aug 5, 2019

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

ronag force-pushed the stream-fix-writable branch from e466fc1 to 71e1822 Compare August 5, 2019 14:23

ronag force-pushed the stream-fix-writable branch from 71e1822 to 5b9c143 Compare August 5, 2019 18:37

ronag mentioned this pull request Aug 5, 2019

stream: writable buffering #28978

Closed

4 tasks

ronag force-pushed the stream-fix-writable branch 2 times, most recently from a15dbe9 to ece83de Compare August 5, 2019 21:09

This was referenced Aug 6, 2019

stream: cleanup #29000

Closed

stream: event contract #29005

Closed

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 9, 2019

ronag force-pushed the stream-fix-writable branch from ece83de to c6d35e0 Compare August 9, 2019 15:58

ronag force-pushed the stream-fix-writable branch from c6d35e0 to 16ac559 Compare August 17, 2019 05:31

ronag force-pushed the stream-fix-writable branch from d009f59 to 16ac559 Compare August 17, 2019 06:57

ronag force-pushed the stream-fix-writable branch from 16ac559 to d97138e Compare August 17, 2019 08:21

stream: don't emit finish on error

1d9c349

After 'error' only 'close' should be emitted.

ronag force-pushed the stream-fix-writable branch 2 times, most recently from 053198c to a8b6e9b Compare August 24, 2019 10:45

fixup

2ab2399

ronag commented Aug 24, 2019

View reviewed changes

ronag mentioned this pull request Aug 24, 2019

stream: use callback to properly propagate error #29179

Closed

4 tasks

mcollina approved these changes Aug 30, 2019

View reviewed changes

fixup

c4a2047

jasnell approved these changes Sep 11, 2019

View reviewed changes

fixup: docs

62d16ec

Trott reviewed Sep 12, 2019

View reviewed changes

Comment thread doc/api/stream.md Outdated

Trott reviewed Sep 12, 2019

View reviewed changes

Comment thread doc/api/stream.md Outdated

Apply suggestions from code review

7b33a42

Co-Authored-By: Rich Trott <rtrott@gmail.com>

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

mcollina approved these changes Sep 20, 2019

View reviewed changes

mcollina pushed a commit that referenced this pull request Sep 20, 2019

stream: don't emit finish on error

ba3be57

After 'error' only 'close' should be emitted. PR-URL: #28979 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mcollina closed this Sep 20, 2019

BethGriggs mentioned this pull request Sep 25, 2019

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

Merged

meyfa added a commit to meyfa/writable-wrapper that referenced this pull request Nov 17, 2019

Do not emit 'finish' on write failure; update deps

c5db17a

See nodejs/node#28979. Node streams have stopped emitting 'finish' after 'error' ocurred in Node v13, we have to adapt.

meyfa added a commit to meyfa/giantdb that referenced this pull request Nov 18, 2019

Fix unit test failure in Node 13 and later

a703e63

See nodejs/node#28979

+                  message: 'The "chunk" argument must be one of type string or Buffer. ' +
+                            `Received type ${typeof value}`
+                }));
+              });

+                  message: 'The "chunk" argument must be one of type string or Buffer. ' +
+                            `Received type ${typeof value}`
+                }));
+              });

stream: don't emit finish on error by ronag · Pull Request #28979 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: don't emit finish on error - #28979

stream: don't emit finish on error#28979ronag wants to merge 7 commits intonodejs:masternodejs/node:masterfrom nxtedition:stream-fix-writablenxtedition/node:stream-fix-writableCopy head branch name to clipboard

Conversation

ronag commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Aug 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 5, 2019

Uh oh!

Trott commented Aug 8, 2019

Uh oh!

mcollina commented Aug 9, 2019

Uh oh!

ronag commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 17, 2019

Uh oh!

ronag commented Aug 17, 2019

Uh oh!

nodejs-github-bot commented Aug 17, 2019

Uh oh!

ronag commented Aug 17, 2019

Uh oh!

nodejs-github-bot commented Aug 17, 2019

Uh oh!

ronag Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 30, 2019

Uh oh!

ronag commented Sep 4, 2019

Uh oh!

Trott commented Sep 5, 2019

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Sep 11, 2019

Uh oh!

ronag commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

ronag commented Sep 18, 2019

Uh oh!

mcollina commented Sep 19, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

stream: don't emit finish on error#28979
ronag wants to merge 7 commits into
nodejs:masterfrom
nxtedition:stream-fix-writable

ronag commented Aug 5, 2019 •
edited
Loading

ronag commented Aug 17, 2019 •
edited
Loading

ronag commented Aug 30, 2019 •
edited
Loading

ronag commented Sep 11, 2019 •
edited
Loading