Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
stream: migrate stream errors to internal/errors #15665
Conversation
|
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'); |
mcollina
Sep 28, 2017
Member
I would keep this safety check in.
I would keep this safety check in.
BridgeAR
Sep 28, 2017
Author
Member
It is dead code at the moment. There is no way to trigger it right now and only a false refactoring could trigger this.
It is dead code at the moment. There is no way to trigger it right now and only a false refactoring could trigger this.
BridgeAR
Sep 29, 2017
Author
Member
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.
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.
mcollina
Sep 29, 2017
Member
This can be removed then, 👍 .
This can be removed then,
This condition could never be met because all calling functions guarded against this. PR-URL: nodejs#15665 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#15665 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This condition could never be met because all calling functions guarded against this. PR-URL: nodejs/node#15665 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#15665 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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