stream: don't emit finish on error #28979

ronag · 2019-08-05T14:09:00Z

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~~.

ronag · 2019-08-05T14:09:08Z

ping @mcollina

lib/_stream_writable.js

nodejs-github-bot · 2019-08-05T15:17:58Z

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

Trott · 2019-08-08T02:38:21Z

@nodejs/streams

mcollina · 2019-08-09T09:40:25Z

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

ronag · 2019-08-17T05:30:08Z

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

nodejs-github-bot · 2019-08-17T06:17:09Z

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

ronag · 2019-08-17T07:13:42Z

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

nodejs-github-bot · 2019-08-17T07:21:51Z

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

ronag · 2019-08-17T07:34:57Z

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

nodejs-github-bot · 2019-08-17T13:26:28Z

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

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

ronag · 2019-08-24T12:35:23Z

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

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


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?

ronag · 2019-08-30T16:56:51Z

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

mcollina

LGTM

mcollina · 2019-08-30T17:29:55Z

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

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


can you rename the test and add a description?

ronag · 2019-08-30T17:42:27Z

Renamed test. They both already have description?

ronag · 2019-09-04T21:53:55Z

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

Trott · 2019-09-05T02:43:52Z

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

jasnell

The change in behavior should be documented in streams.md

ronag · 2019-09-11T19:30:27Z

@jasnell: Updated docs.

ronag · 2019-09-11T19:32:32Z

@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?

doc/api/stream.md

Co-Authored-By: Rich Trott <[email protected]>

nodejs-github-bot · 2019-09-12T13:05:36Z

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

ronag · 2019-09-18T23:03:12Z

@Trott: Anything missing here?

mcollina · 2019-09-19T11:22:41Z

Here is a fresh CITGM run

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

mcollina

LGTM

After 'error' only 'close' should be emitted. PR-URL: #28979 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

mcollina · 2019-09-20T09:23:04Z

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

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag commented Aug 5, 2019

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag commented Aug 5, 2019

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

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

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

Trott reviewed Sep 12, 2019

View reviewed changes

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

Apply suggestions from code review

7b33a42

Co-Authored-By: Rich Trott <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: don't emit finish on error #28979

stream: don't emit finish on error #28979

ronag commented Aug 5, 2019 •

edited

Loading

ronag commented Aug 5, 2019

nodejs-github-bot commented Aug 5, 2019

Trott commented Aug 8, 2019

mcollina commented Aug 9, 2019

ronag commented Aug 17, 2019 •

edited

Loading

nodejs-github-bot commented Aug 17, 2019

ronag commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

ronag commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

ronag Aug 24, 2019

mcollina Aug 30, 2019

ronag commented Aug 30, 2019 •

edited

Loading

mcollina left a comment

mcollina Aug 30, 2019

ronag commented Aug 30, 2019

ronag commented Sep 4, 2019

Trott commented Sep 5, 2019

jasnell left a comment

ronag commented Sep 11, 2019

ronag commented Sep 11, 2019 •

edited

Loading

nodejs-github-bot commented Sep 12, 2019

ronag commented Sep 18, 2019

mcollina commented Sep 19, 2019

mcollina left a comment

mcollina commented Sep 20, 2019

stream: don't emit finish on error #28979

stream: don't emit finish on error #28979

Conversation

ronag commented Aug 5, 2019 • edited Loading

Checklist

ronag commented Aug 5, 2019

nodejs-github-bot commented Aug 5, 2019

Trott commented Aug 8, 2019

mcollina commented Aug 9, 2019

ronag commented Aug 17, 2019 • edited Loading

nodejs-github-bot commented Aug 17, 2019

ronag commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

ronag commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

ronag Aug 24, 2019

Choose a reason for hiding this comment

mcollina Aug 30, 2019

Choose a reason for hiding this comment

ronag commented Aug 30, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina Aug 30, 2019

Choose a reason for hiding this comment

ronag commented Aug 30, 2019

ronag commented Sep 4, 2019

Trott commented Sep 5, 2019

jasnell left a comment

Choose a reason for hiding this comment

ronag commented Sep 11, 2019

ronag commented Sep 11, 2019 • edited Loading

nodejs-github-bot commented Sep 12, 2019

ronag commented Sep 18, 2019

mcollina commented Sep 19, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 20, 2019

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