Skip to content

/node

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: make virtual methods errors consistent #18813

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@BridgeAR BridgeAR

@mcollina mcollina

@jasnell jasnell

@targos targos

Assignees
No one assigned
Labels
errors semver-major stream
Projects
None yet
Milestone
  10.0.0
7 participants
@lpinca @mcollina @jasnell @BridgeAR @joyeecheung @targos @nodejs-github-bot
@lpinca
Copy link
Member

lpinca commented Feb 16, 2018 •
edited

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

@nodejs-github-bot nodejs-github-bot added errors stream labels Feb 16, 2018

@lpinca lpinca added the semver-major label Feb 16, 2018

@lpinca

@lpinca lpinca added the work in progress (WIP) label Feb 16, 2018

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

@lpinca lpinca removed the work in progress (WIP) label Feb 16, 2018

@jasnell

jasnell approved these changes Feb 16, 2018
View changes

@BridgeAR

BridgeAR approved these changes Feb 16, 2018
View changes

Copy link
Member

BridgeAR left a comment

LGTM

@BridgeAR BridgeAR added the author ready label Feb 16, 2018

@BridgeAR BridgeAR requested a review from nodejs/tsc Feb 16, 2018

@targos

targos approved these changes Feb 16, 2018
View changes

@mcollina

mcollina requested changes Feb 16, 2018
View changes

Copy link
Member

mcollina left a comment

I think the correct behavior for these is to emit('error'). Note that calling the callback with an error will have that result.
lib/_stream_readable.js Outdated
@@ -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()');

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Feb 16, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Feb 16, 2018

Member

Can you elaborate on the reasons?
lib/_stream_transform.js Outdated
@@ -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()');

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Feb 16, 2018

Member

I would change this as cb(new errors.Error()).
lib/_stream_writable.js Outdated
@@ -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()');

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Feb 16, 2018

Member

I would leave this as it was.
lib/internal/errors.js Outdated
@@ -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');

This comment has been minimized.

Sign in to view
Copy link
@mcollina

mcollina Feb 16, 2018

Member

👍!

@BridgeAR BridgeAR removed the author ready label Feb 16, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 16, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Feb 16, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Feb 16, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 17, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Feb 17, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 17, 2018 •
edited

I still fail to understand.

  1. 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.
  2. 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 lpinca force-pushed the lpinca:uniform/virtual-methods-errors branch from 90809ef to e19a9d1 Feb 19, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 19, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 19, 2018

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

mcollina approved these changes Feb 23, 2018
View changes

Copy link
Member

mcollina left a comment

LGTM
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Feb 23, 2018

This needs a rebase

@lpinca lpinca force-pushed the lpinca:uniform/virtual-methods-errors branch from e19a9d1 to 54c0ab1 Feb 23, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Feb 23, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented Mar 2, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Mar 2, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Mar 2, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

mcollina commented Mar 2, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

joyeecheung commented Mar 2, 2018

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.

@lpinca lpinca added this to the 10.0.0 milestone Mar 11, 2018

@lpinca
stream: make virtual methods errors consistent 
Use the same error code and always emit the error instead of
throwing it.
Loading status checks…
f8bd8ec

@lpinca lpinca force-pushed the lpinca:uniform/virtual-methods-errors branch from 54c0ab1 to f8bd8ec Mar 11, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Mar 11, 2018 •
edited

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/

lpinca added a commit that referenced this pull request Mar 12, 2018

@lpinca
stream: make virtual methods errors consistent 
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>
c979488
@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member Author

lpinca commented Mar 12, 2018

Landed in c979488.

@lpinca lpinca closed this Mar 12, 2018

@lpinca lpinca deleted the lpinca:uniform/virtual-methods-errors branch Mar 12, 2018

FallenRiteMonk added a commit to FallenRiteMonk/node that referenced this pull request Mar 23, 2018

@lpinca @FallenRiteMonk
stream: make virtual methods errors consistent 
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>
d8256f6

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@lpinca @MayaLekova
stream: make virtual methods errors consistent 
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>
fb7552a

@ChALkeR ChALkeR referenced this pull request Jun 23, 2018

Merged

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

2 of 4 tasks complete

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Aug 1, 2018

@ChALkeR
doc: remove unused error codes from errors.md 
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
d7f1e4b

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Aug 5, 2018

@ChALkeR
doc: remove unused error codes from errors.md 
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>
492ac41

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Aug 5, 2018

@ChALkeR
doc: remove unused error codes from errors.md 
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>
f753266

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Aug 6, 2018

@ChALkeR
doc: remove unused error codes from errors.md 
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>
214844e

targos added a commit that referenced this pull request Aug 6, 2018

@ChALkeR @targos
doc: remove unused error codes from errors.md 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
38dd407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.