stream: make virtual methods errors consistent#18813
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added errors 
lpinca added the semver-major lpincaforce-pushed the uniform/virtual-methods-errors branch 2 times, most recently from e5aa300 to 90809efCompare
BridgeAR added the author ready 
mcollina left a comment
mcollina left a comment There was a problem hiding this 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
There was a problem hiding this comment.
I would leave this as this.emit('error').
lib/_stream_transform.js Outdated
There was a problem hiding this comment.
I would change this as cb(new errors.Error()).
lib/_stream_writable.js Outdated
lib/internal/errors.js Outdated
BridgeAR removed the author ready
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 Anyway happy to always emit the error instead of throwing if there is consensus. |
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 commented Feb 16, 2018
I agree with @mcollina that these should be |
lpinca commented Feb 17, 2018
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 Line 608 in 6c9774f |
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. |
I still fail to understand.
That said, I will change this PR to always emit if this is the right thing to do. Pinging @nodejs/collaborators. |
lpincaforce-pushed the uniform/virtual-methods-errors branch from 90809ef to e19a9d1Comparelpinca commented Feb 19, 2018
No one commented and there are 2 TSC members that prefer to emit, so I've updated accordingly. |
lpinca commented Feb 19, 2018
mcollina commented Feb 23, 2018
This needs a rebase |
lpincaforce-pushed the uniform/virtual-methods-errors branch from e19a9d1 to 54c0ab1Comparelpinca commented Feb 23, 2018
BridgeAR commented Mar 2, 2018
@mcollina@jasnell e.g. all our |
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 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 commented Mar 2, 2018
All Node APIs can be called async from user code. These are called async from Node.js code. |
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 |
Use the same error code and always emit the error instead of throwing it.
lpincaforce-pushed the uniform/virtual-methods-errors branch from 54c0ab1 to f8bd8ecCompareI will land this tomorrow, got bored of rebasing :) |
Use the same error code and always emit the error instead of throwing it. PR-URL: #18813 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaë Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
lpinca commented Mar 12, 2018
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaë Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
7 participants
Use the same error code and always emit the error instead of throwing it.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
errors, stream