-
Notifications
You must be signed in to change notification settings - Fork 30k
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: migrate stream errors to internal/errors #15665
Conversation
This condition could never be met because all calling functions guarded against this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// If we get here before consuming all the bytes, then that is a | ||
// bug in node. Should never happen. | ||
if (state.length > 0) | ||
throw new Error('"endReadable()" called on non-empty stream'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep this safety check in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is dead code at the moment. There is no way to trigger it right now and only a false refactoring could trigger this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as reference - there are three occurrences of endReadable
right now. Each of those are only triggered in case state.length === 0
. Those guards are here
https://github.com/BridgeAR/node/blob/615ab68c02e81bc17dbfb15d489a3558f667dc67/lib/_stream_readable.js#L384
https://github.com/BridgeAR/node/blob/615ab68c02e81bc17dbfb15d489a3558f667dc67/lib/_stream_readable.js#L395
https://github.com/BridgeAR/node/blob/615ab68c02e81bc17dbfb15d489a3558f667dc67/lib/_stream_readable.js#L466
The function can also not be triggered from the outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed then, 👍 .
cc @nodejs/streams |
This condition could never be met because all calling functions guarded against this. PR-URL: nodejs#15665 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#15665 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
This condition could never be met because all calling functions guarded against this. PR-URL: nodejs/node#15665 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#15665 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
I removed some dead code and migrated the rest of the stream errors.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
stream