stream: throw invalid argument errors by ronag · Pull Request #31831 · nodejs/node

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.

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

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

attention here

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

-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/

mcollina

lgtm

Co-Authored-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 lpinca and mcollina 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

Comment thread lib/_stream_writable.js Outdated

ronag commented Feb 17, 2020

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

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

ronag commented Feb 17, 2020

View reviewed changes

Comment thread test/parallel/test-stream-writable-null.js Outdated

lpinca reviewed Feb 17, 2020

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

lpinca reviewed Feb 17, 2020

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

lpinca reviewed Feb 17, 2020

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

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

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

fixup: test

10283bb

ronag commented Feb 17, 2020

View reviewed changes

Comment thread test/parallel/test-zlib-invalid-input.js Outdated

Copy link
Copy Markdown

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

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

Comment thread test/parallel/test-zlib-invalid-input.js Outdated

Comment thread test/parallel/test-zlib-object-write.js Outdated

mcollina approved these changes Feb 18, 2020

View reviewed changes

Apply suggestions from code review

5398092

Co-Authored-By: Luigi Pinca <[email protected]>

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

iitalics mentioned this pull request Aug 11, 2025

Fix test suite tulip/node-open-protocol#2

Merged

Uh oh!

Conversation

ronag commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ronag Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 17, 2020

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Feb 18, 2020

Uh oh!

ronag commented Feb 18, 2020 • 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!

ronag commented Feb 19, 2020

Uh oh!

ronag commented Feb 23, 2020

Uh oh!

ronag commented Feb 24, 2020

Uh oh!

ronag commented Feb 24, 2020

Uh oh!

ronag commented Feb 24, 2020

Uh oh!

Trott commented Feb 24, 2020

Uh oh!

Trott commented Feb 24, 2020

Uh oh!

Trott commented Feb 24, 2020

Uh oh!

ronag commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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