Skip to content

stream: don't call _read after destroy() #29491

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
Closed
wants to merge 2 commits into from
Closed

stream: don't call _read after destroy() #29491

wants to merge 2 commits into from

Conversation

@ronag
Copy link
Member

@ronag ronag commented Sep 8, 2019 •
edited
Loading

Partly fixes #29477.

Currently stream implementations MUST check for destroyed inside _read.

Found this while investigating a suspicious destroyed conditional in fs streams.

"Full" fix in #29485 but it is way too breaking at this point and needs more updated tests.

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 8, 2019
@ronag ronag mentioned this pull request Sep 8, 2019
Closed
@ronag ronag force-pushed the no-read-after-destroy branch 3 times, most recently from 0b2d7a0 to c65a3a3 Compare September 8, 2019 08:44
mcollina
mcollina approved these changes Sep 8, 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

@ronag
Copy link
Member Author

ronag commented Sep 8, 2019

@Trott: I'm a little confused. That test does not fail for me locally?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 8, 2019

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

@ronag ronag force-pushed the no-read-after-destroy branch from c65a3a3 to 4f09ffc Compare September 8, 2019 23:12
@Trott

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 10, 2019

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

@ronag ronag force-pushed the no-read-after-destroy branch from 4f09ffc to fd8f2b6 Compare September 11, 2019 19:56
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 12, 2019

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

lpinca
lpinca approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM, not sure about semverness

@mcollina
Copy link
Member

mcollina commented Sep 13, 2019

There seem to be some tests failures/regressions on specific environments :/.
Maybe @Trott can help.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 13, 2019

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

@ronag
Copy link
Member Author

ronag commented Sep 20, 2019

@Trott: This looks ready?

@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 22, 2019
Trott pushed a commit that referenced this pull request Sep 22, 2019
@ronag @Trott
stream: do not call _read() after destroy()
ec390b6 
PR-URL: #29491
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Trott
Copy link
Member

Trott commented Sep 22, 2019

Landed in ec390b6

@Trott Trott closed this Sep 22, 2019
targos pushed a commit that referenced this pull request Sep 23, 2019
@ronag @targos
stream: do not call _read() after destroy()
a68a909 
PR-URL: #29491
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@ronag @BridgeAR
stream: do not call _read() after destroy()
322bc6f 
PR-URL: #29491
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@JungMinu JungMinu JungMinu approved these changes

+1 more reviewer

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

stream: read after destroy?

9 participants

@ronag @nodejs-github-bot @Trott @mcollina @jasnell @Fishrock123 @benjamingr @lpinca @JungMinu