stream: throw invalid argument errors #31831

ronag · 2020-02-17T17:02:42Z

Logic errors that do not depend on stream state should throw instead of invoke callback and emit 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 · 2020-02-17T17:03:29Z

@mcollina: Has a comment which I don't understand:

In the current world of async/await, all errors are automatically catched and go to unhandledRejection :/. Essentially, we need to make sure that the stream is properly destroyed if it happens.

lib/_stream_writable.js

test/parallel/test-stream-writable-null.js

lib/_stream_writable.js

Logic errors that do not depend on stream state should throw instead of invoke callback and emit error.

ronag · 2020-02-17T20:26:16Z

@lpinca: Updated per our discussion

test/parallel/test-stream-writable-invalid-chunk.js

ronag · 2020-02-17T22:57:39Z

test/parallel/test-zlib-invalid-input.js

+  assert.throws(() => {
+    zlib.gunzip(input);
+  }, {
+    name: 'TypeError'
  });


attention here

ronag · 2020-02-17T22:58:45Z

test/parallel/test-zlib-object-write.js

-gunzip.on('error', common.expectsError({
-  name: 'TypeError'
-}));
+});


attention here

nodejs-github-bot · 2020-02-17T22:59:50Z

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

test/parallel/test-zlib-invalid-input.js

test/parallel/test-zlib-object-write.js

mcollina

lgtm

Co-Authored-By: Luigi Pinca <luigipinca@gmail.com>

ronag · 2020-02-18T20:28:15Z

needs another TSC approval, @addaleax @jasnell

ronag · 2020-02-18T20:29:11Z

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

mcollina

lgtm

ronag · 2020-02-19T07:10:38Z

eslint-jest fails in CITGM:

 Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main resolved in /home/iojs/tmp/citgm_tmp/c642c97c-de5a-4bd0-8352-67943d3c1070/eslint-plugin-jest/node_modules/@babel/helper-compilation-targets/package.json
     at applyExports (internal/modules/cjs/loader.js:528:9)
     at resolveExports (internal/modules/cjs/loader.js:545:12)
     at Function.Module._findPath (internal/modules/cjs/loader.js:666:22)
     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1020:27)
     at Function.Module._load (internal/modules/cjs/loader.js:909:27)
     at Module.require (internal/modules/cjs/loader.js:1093:19)
     at require (internal/modules/cjs/helpers.js:72:18)
     at Object.<anonymous> (/home/iojs/tmp/citgm_tmp/c642c97c-de5a-4bd0-8352-67943d3c1070/eslint-plugin-jest/node_modules/@babel/preset-env/lib/debug.js:8:33)
     at Module._compile (internal/modules/cjs/loader.js:1204:30)
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1224:10) {
   code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
 }

ronag · 2020-02-23T22:46:02Z

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

ronag · 2020-02-24T07:25:32Z

CITGM failures does not seem related to this PR.

ronag · 2020-02-24T07:28:04Z

CITGM (master) for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2281/

ronag · 2020-02-24T07:28:51Z

I think this can land given that the CITGM failures seems unrelated. @Trott I'd appreciate a second opinion before landing.

Trott · 2020-02-24T11:22:43Z

I think this can land given that the CITGM failures seems unrelated. @Trott I'd appreciate a second opinion before landing.

Yes, I'd agree. Thanks for opening nodejs/citgm#785.

Trott · 2020-02-24T11:23:20Z

I think this can land given that the CITGM failures seems unrelated. @Trott I'd appreciate a second opinion before landing.

Yes, I'd agree. Thanks for opening nodejs/citgm#785.

Oh, wait, I agree that CITGM is not an obstacle. But it does still need a second TSC approval because it's semver-major. @nodejs/tsc

Trott · 2020-02-24T11:29:00Z

Oh, wait, I agree that CITGM is not an obstacle. But it does still need a second TSC approval because it's semver-major.

Looks good to me and I've added my approval, so this can land. Might not be a bad idea to leave it open for 24 hours or so just to give people one more chance to take a look, but that's not strictly required.

Logic errors that do not depend on stream state should throw instead of invoke callback and emit error. PR-URL: #31831 Refs: #31818 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

ronag · 2020-02-25T11:08:21Z

Landed in 1f20912

ronag added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 17, 2020

ronag requested review from mcollina and lpinca February 17, 2020 17:02

ronag force-pushed the stream-throw-logic-error branch from 92ac737 to 36172d3 Compare February 17, 2020 17:02

ronag mentioned this pull request Feb 17, 2020

http: align with stream.Writable #31818

Closed

4 tasks

ronag force-pushed the stream-throw-logic-error branch 2 times, most recently from 75f4dd3 to f334f94 Compare February 17, 2020 17:07

ronag commented Feb 17, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag commented Feb 17, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag requested a review from mscdex February 17, 2020 17:13

ronag commented Feb 17, 2020

View reviewed changes

test/parallel/test-stream-writable-null.js Outdated Show resolved Hide resolved

lpinca reviewed Feb 17, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

lpinca reviewed Feb 17, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

lpinca reviewed Feb 17, 2020

View reviewed changes

lib/_stream_writable.js Show resolved Hide resolved

stream: throw invalid argument errors

722975a

Logic errors that do not depend on stream state should throw instead of invoke callback and emit error.

ronag force-pushed the stream-throw-logic-error branch from 9f2b5ec to 722975a Compare February 17, 2020 20:25

ronag changed the title ~~stream: throw on logic errors~~ stream: throw invalid argument errors Feb 17, 2020

ronag requested a review from lpinca February 17, 2020 20:25

lpinca reviewed Feb 17, 2020

View reviewed changes

test/parallel/test-stream-writable-invalid-chunk.js Show resolved Hide resolved

fixup: test

10283bb

ronag commented Feb 17, 2020

View reviewed changes

test/parallel/test-zlib-invalid-input.js

assert.throws(() => {

zlib.gunzip(input);

}, {

name: 'TypeError'

});

Copy link

Member Author

ronag Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attention here

fixup: test

93c4107

ronag force-pushed the stream-throw-logic-error branch from abecd9f to 93c4107 Compare February 17, 2020 22:58

ronag commented Feb 17, 2020

View reviewed changes

test/parallel/test-zlib-object-write.js

gunzip.on('error', common.expectsError({

name: 'TypeError'

}));

});

Copy link

Member Author

ronag Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attention here

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2020

ronag requested a review from lpinca February 18, 2020 18:34

lpinca approved these changes Feb 18, 2020

View reviewed changes

test/parallel/test-zlib-invalid-input.js Outdated Show resolved Hide resolved

test/parallel/test-zlib-object-write.js Outdated Show resolved Hide resolved

mcollina approved these changes Feb 18, 2020

View reviewed changes

Apply suggestions from code review

5398092

Co-Authored-By: Luigi Pinca <luigipinca@gmail.com>

mcollina approved these changes Feb 19, 2020

View reviewed changes

ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2020

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 24, 2020

Trott approved these changes Feb 24, 2020

View reviewed changes

ronag closed this Feb 25, 2020

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

ronag mentioned this pull request Apr 9, 2020

Errors are no longer emitted on stdout starting from v13.x nodejs/help#2578

Closed

ronag mentioned this pull request May 1, 2020

stream: throw instead of destroy? #33192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: throw invalid argument errors #31831

stream: throw invalid argument errors #31831

ronag commented Feb 17, 2020 •

edited

Loading

ronag commented Feb 17, 2020 •

edited

Loading

ronag commented Feb 17, 2020 •

edited

Loading

ronag Feb 17, 2020

ronag Feb 17, 2020

nodejs-github-bot commented Feb 17, 2020

mcollina left a comment

ronag commented Feb 18, 2020

ronag commented Feb 18, 2020 •

edited

Loading

mcollina left a comment

ronag commented Feb 19, 2020

ronag commented Feb 23, 2020

ronag commented Feb 24, 2020

ronag commented Feb 24, 2020

ronag commented Feb 24, 2020

Trott commented Feb 24, 2020

Trott commented Feb 24, 2020

Trott commented Feb 24, 2020

ronag commented Feb 25, 2020

stream: throw invalid argument errors #31831

stream: throw invalid argument errors #31831

Conversation

ronag commented Feb 17, 2020 • edited Loading

Checklist

ronag commented Feb 17, 2020 • edited Loading

ronag commented Feb 17, 2020 • edited Loading

ronag Feb 17, 2020

Choose a reason for hiding this comment

ronag Feb 17, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 17, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Feb 18, 2020

ronag commented Feb 18, 2020 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Feb 19, 2020

ronag commented Feb 23, 2020

ronag commented Feb 24, 2020

ronag commented Feb 24, 2020

ronag commented Feb 24, 2020

Trott commented Feb 24, 2020

Trott commented Feb 24, 2020

Trott commented Feb 24, 2020

ronag commented Feb 25, 2020

ronag commented Feb 17, 2020 •

edited

Loading

ronag commented Feb 17, 2020 •

edited

Loading

ronag commented Feb 17, 2020 •

edited

Loading

ronag commented Feb 18, 2020 •

edited

Loading