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 ${typeofvalue}`
+}));
+});


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 ${typeofvalue}`
+}));
+});


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 resolvedHide resolved

ronag commented Aug 5, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

ronag commented Aug 5, 2019
View reviewed changes

lib/_stream_writable.js Outdated Show resolvedHide resolved

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

ronag force-pushed the stream-fix-writable branch from 71e1822 to 5b9c143Compare 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 ece83deCompare 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 c6d35e0Compare August 9, 2019 15:58

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

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

ronag force-pushed the stream-fix-writable branch from 16ac559 to d97138eCompare 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 a8b6e9bCompare 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 resolvedHide resolved

Trott reviewed Sep 12, 2019
View reviewed changes

doc/api/stream.md Outdated Show resolvedHide resolved

Apply suggestions from code review
7b33a42
Co-Authored-By: Rich Trott <[email protected]>

Trott force-pushed the master branch from 1ecc406 to 49cf67eCompare 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

Uh oh!

stream: don't emit finish on error#28979

stream: don't emit finish on error #28979

Uh oh!

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!

ronagAug 24, 2019

Choose a reason for hiding this comment

Uh oh!

mcollinaAug 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!

mcollinaAug 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

Milestone

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