Skip to content

stream: ensure finish is emitted in next tick#30733

Closed
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:fix-stream-finish
Closed

stream: ensure finish is emitted in next tick#30733
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:fix-stream-finish

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 force-pushed the fix-stream-finish branch from e197831 to a9e54cc Compare November 30, 2019 15:02
@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 ronag force-pushed the fix-stream-finish branch from a9e54cc to 7ee659f Compare November 30, 2019 17:51
@ronag
stream: ensure finish is emitted in next tick
cdc12a6 
When using end() it was possible for 'finish' to
be emitted synchronously.
@ronag ronag force-pushed the fix-stream-finish branch from 7ee659f to cdc12a6 Compare November 30, 2019 17:57
@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

nodejs-github-bot commented Dec 1, 2019

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

benjamingr commented Dec 7, 2019

@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