Skip to content
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.

Sign up for GitHub

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

Jump to bottom

stream: cleanup use of _readableState.ended #29645

Closed
wants to merge 2 commits into from
Closed

stream: cleanup use of _readableState.ended #29645

wants to merge 2 commits into from

Conversation

ckarande
Copy link
Contributor

Replaces references to Readable stream's internal state
_readableState.ended with readableEnded.

One thing to highlight that readable stream internally (L#216)
sets the readableEnded property using _readableState.endEmitted and
not _readableState.ended. The files changed in this PR used
_readableState.ended state.

Although all existing tests pass and it seems correct to rely on the
_readableState.endEmitted to know when the stream ended, please
suggest if this could be a potential issue.

cc: @addaleax @mcollina

Refs: #445

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. worker Issues and PRs related to Worker support. labels Sep 21, 2019
addaleax
addaleax previously approved these changes Sep 21, 2019
View reviewed changes
@addaleax
Copy link
Member

One thing to highlight that readable stream internally (L#216)
sets the readableEnded property using _readableState.endEmitted and
not _readableState.ended. The files changed in this PR used
_readableState.ended state.

Although all existing tests pass and it seems correct to rely on the
_readableState.endEmitted to know when the stream ended, please
suggest if this could be a potential issue.

Does that mean that e.g. the .push(null) in the worker code can fail because .push(null) has already been called earlier, but the 'end' event just hasn’t been emitted yet?

@ckarande
Copy link
Contributor Author

I don't this so. As per the readable stream current implementation, any subsequent .push(null) invocations after first .push(null) until the end event is emitted would have no impact on the state of the stream or cause any events or error.

addaleax
addaleax approved these changes Sep 21, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM but I’d feel more comfortable if there was a test somewhere (whether it already exists or not) that makes sure that repeated .push(null) calls do not lead to errors :)

@ckarande
Copy link
Contributor Author

Yes, makes sense. I will look for one if exists or add it as part of this PR if missing. Thanks.

@ckarande
Copy link
Contributor Author

ckarande commented Sep 21, 2019 •
edited
Loading

I couldn't find an existing test verifying multiple .push(null) is safe. I just added one as part of this PR.

mcollina
mcollina approved these changes Sep 21, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 21, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@danbev
Copy link
Contributor

danbev commented Sep 24, 2019

Landed in fed05cc, and e078e48.

@danbev danbev closed this Sep 24, 2019
danbev pushed a commit that referenced this pull request Sep 24, 2019
@ckarande @danbev
stream: cleanup use of internal ended state
fed05cc 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
danbev pushed a commit that referenced this pull request Sep 24, 2019
@ckarande @danbev
stream: add test for multiple .push(null)
e078e48 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mscdex
Copy link
Contributor

mscdex commented Sep 24, 2019 •
edited
Loading

The subsystem prefix on the second commit should have been test instead of stream.

BridgeAR pushed a commit that referenced this pull request Sep 24, 2019
@ckarande @BridgeAR
stream: cleanup use of internal ended state
4f00ef5 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Sep 24, 2019
@ckarande @BridgeAR
stream: add test for multiple .push(null)
83fff25 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@ckarande @BridgeAR
stream: cleanup use of internal ended state
b100897 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@ckarande @BridgeAR
stream: add test for multiple .push(null)
f016823 
PR-URL: #29645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Sep 25, 2019
Closed
Closed
This was referenced Sep 26, 2019
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ckarande @addaleax @nodejs-github-bot @danbev @mscdex @mcollina @BridgeAR @trivikr @ZYSzys