FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: migrate stream errors to internal/errors by BridgeAR · Pull Request #15665 · nodejs/node · GitHub

/ node Public

stream: migrate stream errors to internal/errors - #15665

Closed
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:migrate-stream-errors
ConversationCommits (2)ChecksFiles changed
Closed

stream: migrate stream errors to internal/errors#15665
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:migrate-stream-errors

Conversation

BridgeAR commented Sep 28, 2017

Copy link
Copy Markdown
Member

I removed some dead code and migrated the rest of the stream errors.

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)

stream

stream: remove dead code
1fbf3dc 
This condition could never be met because all calling functions
guarded against this.
stream: migrate to internal/errors
615ab68
BridgeAR added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Sep 28, 2017
BridgeAR requested a review from mcollina September 28, 2017 21:17
nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2017
mcollina approved these changes Sep 28, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Comment thread lib/_stream_readable.js
// 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would keep this safety check in.

BridgeAR Sep 28, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This can be removed then, 👍 .

mcollina requested a review from jasnell September 28, 2017 23:45

mcollina commented Sep 28, 2017

Copy link
Copy Markdown
Member

cc @nodejs/streams

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 29, 2017

BridgeAR commented Oct 1, 2017

Copy link
Copy Markdown
Member Author

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

BridgeAR commented Oct 1, 2017

Copy link
Copy Markdown
Member Author

Landed in 4536128 and db7d133

BridgeAR closed this Oct 1, 2017
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 1, 2017
stream: remove dead code
4536128 
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 1, 2017
stream: migrate to internal/errors
db7d133 
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>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
stream: remove dead code
061f880 
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>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
stream: migrate to internal/errors
4347d7c 
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>
jasnell mentioned this pull request Oct 30, 2017
Merged
BridgeAR deleted the migrate-stream-errors branch April 1, 2019 23:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL