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: ensure finish is emitted in next tick #30733

Closed
wants to merge 1 commit into from
Closed

stream: ensure finish is emitted in next tick #30733

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 30, 2019 •
edited
Loading

When using end() it was possible for 'finish' to be emitted synchronously causing incorrect behaviour and subtle bugs.

This is an edge case not usually encountered since a write() is usually pending and will be the one to initiate finishMaybe() through afterWrite which is always async.

This PR also cause _destroy() to be called in nextTick when autoDestroy is enabled during this edge case.

NOTE: 'prefinish' must be synchronous in order to not break Transform. I've got another PR in the works to sort this out.

Should probably be semver major.

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 Nov 30, 2019
@ronag
Copy link
Member Author

ronag commented Nov 30, 2019 •
edited
Loading

@Trott: Who's familiar with net? When thinking about this I just realized that I'm not sure whether net ensures that it won't call _undestroy before 'close' (which I believe is a required invariant regardless of this PR, this PR might make it worse though if this is not the case). I wasn't able to determine this.

@ronag ronag mentioned this pull request Nov 30, 2019
Closed
4 tasks
@Trott
Copy link
Member

Trott commented Nov 30, 2019

@Trott: Who's familiar with net?

Good question. I'm surprised there isn't a @nodejs/net team. Using git blame to identify the most recent author of lines in lib/net.js that call _undestroy(), that person is @mcollina so let's start with them.

@ronag
stream: ensure finish is emitted in next tick
cdc12a6 
When using end() it was possible for 'finish' to
be emitted synchronously.
@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 1, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
Copy link
Member

lpinca commented Dec 1, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2100/

@ronag ronag mentioned this pull request Dec 7, 2019
Closed
4 tasks
@ronag
Copy link
Member Author

ronag commented Dec 7, 2019

@benjamingr can I get a nodejs/streams ping on this one?

@ronag
Copy link
Member Author

ronag commented Dec 7, 2019 •
edited
Loading

net has a synchronous _destroy so _undestroy should be somewhat safe where it's called. See, #30832.

@benjamingr
Copy link
Member

@nodejs/streams also this is terrifying :D

@addaleax
Copy link
Member

addaleax commented Dec 7, 2019

This needs another @nodejs/tsc approval (@mcollina?) but should otherwise be good to go

mcollina
mcollina approved these changes Dec 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 Dec 12, 2019 •
edited
Loading

@Trott: I think this needs a CITGM and then might be able to land? If CITGM is ok I think this can land?

@Trott
Copy link
Member

Trott commented Dec 14, 2019

CITGM looks good.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2019
Trott pushed a commit that referenced this pull request Dec 14, 2019
@ronag @Trott
stream: ensure finish is emitted in next tick
e13a37e 
When using end() it was possible for 'finish' to
be emitted synchronously.

PR-URL: #30733
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 14, 2019

Landed in e13a37e

@Trott Trott closed this Dec 14, 2019
@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
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

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax 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. 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.
7 participants
@ronag @Trott @nodejs-github-bot @lpinca @benjamingr @addaleax @mcollina