stream: make virtual methods errors consistent #18813

lpinca · 2018-02-16T11:57:35Z

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

lpinca · 2018-02-16T11:59:19Z

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

BridgeAR

LGTM

mcollina

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

mcollina · 2018-02-16T20:39:52Z

lib/_stream_readable.js

@@ -561,7 +561,7 @@ function maybeReadMore_(stream, state) {
 // for virtual (non-string, non-buffer) streams, "length" is somewhat
 // arbitrary, and perhaps not very meaningful.
 Readable.prototype._read = function(n) {
-  this.emit('error', new errors.Error('ERR_STREAM_READ_NOT_IMPLEMENTED'));
+  throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_read()');


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

Can you elaborate on the reasons?

mcollina · 2018-02-16T20:40:13Z

lib/_stream_transform.js

@@ -157,7 +157,7 @@ Transform.prototype.push = function(chunk, encoding) {
 // an error, then that'll put the hurt on the whole operation.  If you
 // never call cb(), then you'll never get another chunk.
 Transform.prototype._transform = function(chunk, encoding, cb) {
-  throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_transform');
+  throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_transform()');


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

mcollina · 2018-02-16T20:40:23Z

lib/_stream_writable.js

@@ -541,7 +541,7 @@ function clearBuffer(stream, state) {
 }

 Writable.prototype._write = function(chunk, encoding, cb) {
-  cb(new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_write'));
+  throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', '_write()');


I would leave this as it was.

mcollina · 2018-02-16T20:40:36Z

lib/internal/errors.js

@@ -761,7 +761,6 @@ E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
 E('ERR_STREAM_CANNOT_PIPE', 'Cannot pipe, not readable');
 E('ERR_STREAM_NULL_VALUES', 'May not write null values to stream');
 E('ERR_STREAM_PUSH_AFTER_EOF', 'stream.push() after EOF');
-E('ERR_STREAM_READ_NOT_IMPLEMENTED', '_read() is not implemented');


lpinca · 2018-02-16T21:10:39Z

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.

mcollina · 2018-02-16T22:07:59Z

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

jasnell · 2018-02-16T22:10:25Z

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

lpinca · 2018-02-17T06:21:29Z

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.

mcollina · 2018-02-17T09:51:49Z

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.

lpinca · 2018-02-17T11:05:14Z

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.

lpinca · 2018-02-19T11:04:30Z

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

lpinca · 2018-02-19T11:07:35Z

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

mcollina

LGTM

mcollina · 2018-02-23T12:28:08Z

This needs a rebase

lpinca · 2018-02-23T13:20:49Z

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

BridgeAR · 2018-03-02T01:16:34Z

@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.

mcollina · 2018-03-02T06:40:49Z

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

lpinca · 2018-03-02T06:47:09Z

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

mcollina · 2018-03-02T06:55:04Z

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

joyeecheung · 2018-03-02T13:07:50Z

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.

lpinca · 2018-03-11T09:59:52Z

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>

lpinca · 2018-03-12T13:30:29Z

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 pending requests for changes, 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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: make virtual methods errors consistent #18813

stream: make virtual methods errors consistent #18813

lpinca commented Feb 16, 2018 •

edited

Loading

lpinca commented Feb 16, 2018 •

edited

Loading

BridgeAR left a comment

mcollina left a comment

mcollina Feb 16, 2018

BridgeAR Feb 16, 2018

mcollina Feb 16, 2018

mcollina Feb 16, 2018

mcollina Feb 16, 2018

lpinca commented Feb 16, 2018 •

edited

Loading

mcollina commented Feb 16, 2018

jasnell commented Feb 16, 2018

lpinca commented Feb 17, 2018

mcollina commented Feb 17, 2018

lpinca commented Feb 17, 2018 •

edited

Loading

lpinca commented Feb 19, 2018

lpinca commented Feb 19, 2018

mcollina left a comment

mcollina commented Feb 23, 2018

lpinca commented Feb 23, 2018

BridgeAR commented Mar 2, 2018

mcollina commented Mar 2, 2018

lpinca commented Mar 2, 2018

mcollina commented Mar 2, 2018

joyeecheung commented Mar 2, 2018

lpinca commented Mar 11, 2018 •

edited

Loading

lpinca commented Mar 12, 2018

stream: make virtual methods errors consistent #18813

stream: make virtual methods errors consistent #18813

Conversation

lpinca commented Feb 16, 2018 • edited Loading

Checklist

Affected core subsystem(s)

lpinca commented Feb 16, 2018 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina Feb 16, 2018

Choose a reason for hiding this comment

BridgeAR Feb 16, 2018

Choose a reason for hiding this comment

mcollina Feb 16, 2018

Choose a reason for hiding this comment

mcollina Feb 16, 2018

Choose a reason for hiding this comment

mcollina Feb 16, 2018

Choose a reason for hiding this comment

lpinca commented Feb 16, 2018 • edited Loading

mcollina commented Feb 16, 2018

jasnell commented Feb 16, 2018

lpinca commented Feb 17, 2018

mcollina commented Feb 17, 2018

lpinca commented Feb 17, 2018 • edited Loading

lpinca commented Feb 19, 2018

lpinca commented Feb 19, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 23, 2018

lpinca commented Feb 23, 2018

BridgeAR commented Mar 2, 2018

mcollina commented Mar 2, 2018

lpinca commented Mar 2, 2018

mcollina commented Mar 2, 2018

joyeecheung commented Mar 2, 2018

lpinca commented Mar 11, 2018 • edited Loading

lpinca commented Mar 12, 2018

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