stream: readable throw unhandled error #29806

ronag · 2019-10-01T22:01:25Z

If autoDestroy then we should still throw on unhandled errors.

Currently, since autoDestroy will only emit error once, will cause the error not to end up as unhandled.

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-10-01T22:01:54Z

lib/_stream_readable.js

@@ -778,7 +778,7 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
    unpipe();
    dest.removeListener('error', onerror);
    if (EE.listenerCount(dest, 'error') === 0)
-      errorOrDestroy(dest, er);


this will only emit error if not autoDestroy

If autoDestroy then we should still throw on unhandled errors.

test/parallel/test-stream-pipe-error-unhandled.js

Co-Authored-By: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

Trott · 2019-10-03T22:32:34Z

@nodejs/streams

jasnell · 2019-10-03T23:32:31Z

Because of the change in error handling here, defensively marking semver-major

jasnell · 2019-10-03T23:36:01Z

test/parallel/test-stream-pipe-error-unhandled.js

+});
+
+r.pipe(w);
+w.destroy(new Error('asd'));


hmm... this ends up changing the semantics of destroy() rather significantly in that prior to this change, subsequent destroy() calls would be non-ops. This is not necessarily a bad thing in that I tend to prefer the notion that most calls on stream objects should fail with an error after destroy() is called. What I'm not certain about, however, is whether this should be direct throw or an emitted throw.

I'm not sure I follow. All this PR does is to re-emit the already emitted error.

I think in other places we just re-emit (like this) instead of throwing, e.g. https://github.com/nodejs/node/blob/master/lib/internal/streams/legacy.js#L56

lib/_stream_readable.js

jasnell · 2019-10-03T23:38:18Z

This will need documentation updates

ronag · 2019-10-04T06:19:26Z

This will need documentation updates

Which part are you referring to?

mcollina · 2019-10-04T08:27:23Z

I do not understand this change. Can you make some examples of why this would be better?

ronag · 2019-10-04T08:49:49Z

I do not understand this change. Can you make some examples of why this would be better?

Currently if a stream is piped and emit an error through destroy and autoDestroy is enabled, it will not cause an unhandledException error even though the user has not registered any error handler.

// writable with autoDestroy
readable.pipe(writable);
writable.destroy(new Error()); // Not unhandledException?

Check the updated test for further example.

EDIT: Updated

lib/_stream_readable.js

mcollina

LGTM, I think this is ok.. maybe @mafintosh?

BridgeAR

The code and the reason are LGTM. If the change itself might be good for streams is difficult to tell for me, since streams are so brittle.

mafintosh · 2019-10-11T16:06:47Z

As I read this it will throw an exception if a stream has not previously been destroyed and if no error handler is attached which makes sense to me.

LGTM

nodejs-github-bot · 2019-10-13T03:54:34Z

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

Trott · 2019-10-13T03:55:47Z

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

Trott · 2019-10-13T23:35:29Z

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

CITGM looks OK. Landing.

Trott · 2019-10-13T23:38:51Z

Landed in f8f6a21

If autoDestroy then we should still throw on unhandled errors. PR-URL: #29806 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Co-Authored-By: Trivikram Kamat <trivikr.dev@gmail.com>

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 Oct 1, 2019

ronag commented Oct 1, 2019

View reviewed changes

stream: readable throw unhandled error

25c7fdd

If autoDestroy then we should still throw on unhandled errors.

ronag force-pushed the readable-throw-error branch from 9fd7cee to 25c7fdd Compare October 1, 2019 22:04

trivikr reviewed Oct 2, 2019

View reviewed changes

test/parallel/test-stream-pipe-error-unhandled.js Outdated Show resolved Hide resolved

Update test/parallel/test-stream-pipe-error-unhandled.js

2cc3fc2

Co-Authored-By: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 3, 2019

jasnell reviewed Oct 3, 2019

View reviewed changes

lib/_stream_readable.js Show resolved Hide resolved

ronag commented Oct 4, 2019

View reviewed changes

lib/_stream_readable.js Show resolved Hide resolved

mcollina approved these changes Oct 4, 2019

View reviewed changes

BridgeAR approved these changes Oct 9, 2019

View reviewed changes

jasnell approved these changes Oct 11, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

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

Trott closed this Oct 13, 2019

BethGriggs mentioned this pull request Oct 15, 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: readable throw unhandled error #29806

stream: readable throw unhandled error #29806

ronag commented Oct 1, 2019

ronag Oct 1, 2019

Trott commented Oct 3, 2019

jasnell commented Oct 3, 2019

jasnell Oct 3, 2019

ronag Oct 4, 2019

ronag Oct 4, 2019

jasnell commented Oct 3, 2019

ronag commented Oct 4, 2019

mcollina commented Oct 4, 2019

ronag commented Oct 4, 2019 •

edited

Loading

mcollina left a comment

BridgeAR left a comment

mafintosh commented Oct 11, 2019

This comment has been minimized.

nodejs-github-bot commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

stream: readable throw unhandled error #29806

stream: readable throw unhandled error #29806

Conversation

ronag commented Oct 1, 2019

Checklist

ronag Oct 1, 2019

Choose a reason for hiding this comment

Trott commented Oct 3, 2019

jasnell commented Oct 3, 2019

jasnell Oct 3, 2019

Choose a reason for hiding this comment

ronag Oct 4, 2019

Choose a reason for hiding this comment

ronag Oct 4, 2019

Choose a reason for hiding this comment

jasnell commented Oct 3, 2019

ronag commented Oct 4, 2019

mcollina commented Oct 4, 2019

ronag commented Oct 4, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

mafintosh commented Oct 11, 2019

This comment has been minimized.

nodejs-github-bot commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

Trott commented Oct 13, 2019

ronag commented Oct 4, 2019 •

edited

Loading