stream: make virtual methods errors consistent by lpinca · Pull Request #18813 · nodejs/node · GitHub

Use the same error code and always emit the error instead of throwing it.

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

Affected core subsystem(s)

errors, stream

CI: https://ci.nodejs.org/job/node-test-pull-request/13194/
CI: https://ci.nodejs.org/job/node-test-pull-request/13195/
CI: https://ci.nodejs.org/job/node-test-pull-request/13196/

LGTM

I think the correct behavior for these is to emit('error'). Note that calling the callback with an error will have that result.

I would leave this as this.emit('error').

Can you elaborate on the reasons?

I would change this as cb(new errors.Error()).

I would leave this as it was.

👍!

Note that calling the callback with an error will have that result.

Yes, I changed it because I think it doesn't make sense to emit the error, it should just crash as the method has not been implemented. If there is an 'error' listener this may go unnoticed.

Anyway happy to always emit the error instead of throwing if there is consensus.

I think throwing is not the way to go, mainly because that error is likely not to be catchable in any way.

I agree with @mcollina that these should be emit('error', ...) ... it's much more consistent with what users expect with stream implementations.

I think throwing is not the way to go, mainly because that error is likely not to be catchable in any way.

Isn't this the point of the error? We throw for invalid arguments or options, in my opinion this is the same kind of error (invalid implementation).

I guess this

node/lib/_http_outgoing.js

Line 608 in 6c9774f

throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_implicitHeader()');

should also be emitted if we agree that these errors should be emitted.

The reason why these needs to be emitted is because they could be called asynchronously. In such cases it would be hard to track to which stream they belong.

I still fail to understand.

If there is no 'error' listener the error will be thrown and the stack trace will be exactly the same as if the error was thrown in the first place.
If there is at least an 'error' listener, the developer has to stop the process, add a proper implementation and restart it, and again the stack trace will be exactly the same as if the error was thrown. How can this help determine which stream generated the error?

That said, I will change this PR to always emit if this is the right thing to do.

Pinging @nodejs/collaborators.

No one commented and there are 2 TSC members that prefer to emit, so I've updated accordingly.

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

LGTM

This needs a rebase

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

@mcollina @jasnell e.g. all our fs errors throw sync instead of returning the error in the callback. That is also true for any other part of the code. So I am still not convinced about using emit here. All of that code could be used async. The only way I can imagine how these functions could be called is by faulty user code. And for me that is the same as the fs case.

@BridgeAR I would agree if those function were called sync all the time. In non-trivial cases, those methods are called async.

@mcollina it doesn't matter imho, what's the difference, any method or function that throws synchronously can be called async.

All Node APIs can be called async from user code. These are called async from Node.js code.

I think when to throw such errors depends on the nature of those errors: can they be expected from a user, or are they just bugs/incorrect usage of APIs. In the first case it make sense to throw asynchronously because we cannot know the result of an async operation synchronously anyway. In the second case it makes sense to throw synchronously because those errors are more like assertions in nature. The async operations cannot even be initiated so there is not really much point to delay the notification.

Also the user cannot really handle those errors (type checking, streams not implemented, etc) in their code anyway. The only sane way to handle those errors is probably just throw it again. If they try to handle a ERR_INVALID_* or ERR_METHOD_NOT_IMPLEMENTED they are probably just going to leave their program in a really bad state.

Use the same error code and always emit the error instead of throwing it.

I will land this tomorrow, got bored of rebasing :)
CI: https://ci.nodejs.org/job/node-test-pull-request/13619/
CI: https://ci.nodejs.org/job/node-test-pull-request/13642/

Use the same error code and always emit the error instead of throwing it. PR-URL: #18813 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaë Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Landed in c979488.

Use the same error code and always emit the error instead of throwing it. PR-URL: nodejs#18813 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaë Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This removes two unused error codes: * ERR_STREAM_READ_NOT_IMPLEMENTED, removed in c979488 (PR nodejs#18813). * ERR_VALUE_OUT_OF_RANGE, removed in d022cb1 (PR nodejs#17648). PR-URL: nodejs#21491 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

This removes two unused error codes: * ERR_STREAM_READ_NOT_IMPLEMENTED, removed in c979488 (PR #18813). * ERR_VALUE_OUT_OF_RANGE, removed in d022cb1 (PR #17648). PR-URL: #21491 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Feb 16, 2018

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 16, 2018

lpinca added the wip Issues and PRs that are still a work in progress. label Feb 16, 2018

lpinca force-pushed the uniform/virtual-methods-errors branch 2 times, most recently from e5aa300 to 90809ef Compare February 16, 2018 12:52

lpinca removed the wip Issues and PRs that are still a work in progress. label Feb 16, 2018

jasnell approved these changes Feb 16, 2018

View reviewed changes

BridgeAR approved these changes Feb 16, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 16, 2018

BridgeAR requested a review from a team February 16, 2018 17:35

targos approved these changes Feb 16, 2018

View reviewed changes

mcollina requested changes Feb 16, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 16, 2018

lpinca force-pushed the uniform/virtual-methods-errors branch from 90809ef to e19a9d1 Compare February 19, 2018 11:03

mcollina approved these changes Feb 23, 2018

View reviewed changes

lpinca force-pushed the uniform/virtual-methods-errors branch from e19a9d1 to 54c0ab1 Compare February 23, 2018 13:18

lpinca added this to the 10.0.0 milestone Mar 11, 2018

stream: make virtual methods errors consistent

f8bd8ec

Use the same error code and always emit the error instead of throwing it.

lpinca force-pushed the uniform/virtual-methods-errors branch from 54c0ab1 to f8bd8ec Compare March 11, 2018 09:55

lpinca closed this Mar 12, 2018

lpinca deleted the uniform/virtual-methods-errors branch March 12, 2018 13:30

ChALkeR mentioned this pull request Jun 23, 2018

doc: remove 2 unused error codes from errors.md #21491

Merged

4 tasks

stream: make virtual methods errors consistent by lpinca · Pull Request #18813 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: make virtual methods errors consistent - #18813

stream: make virtual methods errors consistent#18813lpinca wants to merge 1 commit intonodejs:masternodejs/node:masterfrom lpinca:uniform/virtual-methods-errorslpinca/node:uniform/virtual-methods-errorsCopy head branch name to clipboard

Conversation

lpinca commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

lpinca commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

lpinca commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Feb 16, 2018

Uh oh!

jasnell commented Feb 16, 2018

Uh oh!

lpinca commented Feb 17, 2018

Uh oh!

mcollina commented Feb 17, 2018

Uh oh!

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

Uh oh!

Uh oh!

lpinca commented Feb 19, 2018

Uh oh!

lpinca commented Feb 19, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Feb 23, 2018

Uh oh!

lpinca commented Feb 23, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

lpinca commented Mar 2, 2018

Uh oh!

mcollina commented Mar 2, 2018

Uh oh!

joyeecheung commented Mar 2, 2018

Uh oh!

lpinca commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

stream: make virtual methods errors consistent#18813
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:uniform/virtual-methods-errors

lpinca commented Feb 16, 2018 •
edited
Loading

lpinca commented Feb 16, 2018 •
edited
Loading

lpinca commented Feb 16, 2018 •
edited
Loading

lpinca commented Feb 17, 2018 •
edited
Loading

lpinca commented Mar 11, 2018 •
edited
Loading