stream: don't emit after 'error' #28708

ronag · 2019-07-15T21:49:37Z

We should not be emitting e.g. prefinish etc... after error.

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

ronag · 2019-07-15T21:49:48Z

@mcollina ping

mcollina · 2019-07-16T07:37:43Z

This is likely correct, but I would like to understand what use case are you trying to solve with this change.

mcollina · 2019-07-16T07:38:00Z

Tagged as semver-major as it's a behavior change.

ronag · 2019-07-16T07:57:43Z

@mcollina: I have listeners that I don't expect to be fired after error, e.g. if I do cleanup in error doing cleanup again in finish can cause errors.

mcollina

LGTM

nodejs-github-bot · 2019-07-16T08:16:33Z

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

mcollina · 2019-07-16T08:16:58Z

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

Trott

LGTM if CITGM doesn't turn up any surprises

mcollina · 2019-07-16T16:39:38Z

blocked on standard-things/esm#821, there are modules (split2, browserify, tape, etc), that make heavy use of streams.

lpinca

SGTM

test/parallel/test-stream2-writable.js

Trott · 2019-07-16T16:46:38Z

blocked on standard-things/esm#821, there are modules (split2, browserify, tape, etc), that make heavy use of streams.

I guess pretty much all CITGM-needing semver-major changes are blocked on that at this point?

mcollina · 2019-07-16T16:52:02Z

I guess pretty much all CITGM-needing semver-major changes are blocked on that at this point?

That's my point of view. We should probably revert what caused that instead.

test/parallel/test-stream2-writable.js

ronag · 2019-08-09T16:12:34Z

@Trott: This is no longer blocked

nodejs-github-bot · 2019-08-09T18:14:57Z

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

nodejs-github-bot · 2019-08-09T18:52:50Z

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

Trott · 2019-08-09T20:45:05Z

CITGM: https://ci.nodejs.org/view/All/job/citgm-smoker/1942/

ronag · 2019-08-17T07:38:01Z

@Trott: rebased and ready?

Trott · 2019-08-17T13:40:11Z

CITGM (queued): https://ci.nodejs.org/view/All/job/citgm-smoker/1957/

nodejs-github-bot · 2019-08-17T13:40:39Z

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

Do not emit 'prefinish' or 'finish' event after an error. PR-URL: #28708 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Trott · 2019-08-18T23:28:20Z

Landed in 188896e

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

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

ronag mentioned this pull request Jul 15, 2019

emit after 'error' #28710

Closed

mcollina requested a review from mafintosh July 16, 2019 07:37

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

mcollina requested review from addaleax and lpinca July 16, 2019 07:38

mcollina requested a review from a team July 16, 2019 08:14

mcollina approved these changes Jul 16, 2019

View reviewed changes

Trott approved these changes Jul 16, 2019

View reviewed changes

mcollina added the blocked PRs that are blocked by other issues or PRs. label Jul 16, 2019

lpinca approved these changes Jul 16, 2019

View reviewed changes

test/parallel/test-stream2-writable.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-fix-emit-after-error branch from d79213d to bfa60ef Compare July 16, 2019 16:47

jasnell reviewed Jul 17, 2019

View reviewed changes

test/parallel/test-stream2-writable.js Outdated Show resolved Hide resolved

jasnell approved these changes Jul 17, 2019

View reviewed changes

ronag force-pushed the stream-fix-emit-after-error branch from bfa60ef to 4ff22ad Compare July 17, 2019 16:10

trivikr approved these changes Jul 17, 2019

View reviewed changes

This was referenced Aug 6, 2019

stream: cleanup #29000

Closed

stream: event contract #29005

Closed

ronag force-pushed the stream-fix-emit-after-error branch from 4ff22ad to 77f83e1 Compare August 9, 2019 16:06

Trott removed the blocked PRs that are blocked by other issues or PRs. label Aug 9, 2019

stream: don't emit after 'error'

c5cffa8

ronag force-pushed the stream-fix-emit-after-error branch from 77f83e1 to c5cffa8 Compare August 17, 2019 07:37

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 17, 2019

Trott closed this Aug 18, 2019

BethGriggs mentioned this pull request Sep 9, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: don't emit after 'error' #28708

stream: don't emit after 'error' #28708

ronag commented Jul 15, 2019

ronag commented Jul 15, 2019

mcollina commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Jul 16, 2019 •

edited

Loading

mcollina left a comment

nodejs-github-bot commented Jul 16, 2019

mcollina commented Jul 16, 2019

Trott left a comment

mcollina commented Jul 16, 2019

lpinca left a comment

Trott commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Aug 9, 2019

nodejs-github-bot commented Aug 9, 2019

nodejs-github-bot commented Aug 9, 2019

Trott commented Aug 9, 2019

ronag commented Aug 17, 2019

Trott commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

Trott commented Aug 18, 2019

stream: don't emit after 'error' #28708

stream: don't emit after 'error' #28708

Conversation

ronag commented Jul 15, 2019

Checklist

ronag commented Jul 15, 2019

mcollina commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Jul 16, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 16, 2019

mcollina commented Jul 16, 2019

Trott left a comment

Choose a reason for hiding this comment

mcollina commented Jul 16, 2019

lpinca left a comment

Choose a reason for hiding this comment

Trott commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Aug 9, 2019

nodejs-github-bot commented Aug 9, 2019

nodejs-github-bot commented Aug 9, 2019

Trott commented Aug 9, 2019

ronag commented Aug 17, 2019

Trott commented Aug 17, 2019

nodejs-github-bot commented Aug 17, 2019

Trott commented Aug 18, 2019

ronag commented Jul 16, 2019 •

edited

Loading