Skip to content

stream: don't emit finish after close - #32933

Closed
ronag wants to merge 6 commits into
nodejs:masterfrom
nxtedition:stream-finish-after-close
Closed

stream: don't emit finish after close#32933
ronag wants to merge 6 commits into
nodejs:masterfrom
nxtedition:stream-finish-after-close

Conversation

@ronag

Copy link
Copy Markdown
Member

Writable stream could emit 'finish' after 'close'.

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

@ronagronag added the stream Issues and PRs related to the stream subsystem. label Apr 19, 2020
@ronag
ronagforce-pushed the stream-finish-after-close branch 3 times, most recently from 8bf2442 to ec3e537CompareApril 19, 2020 21:57
@ronag

Copy link
Copy Markdown
MemberAuthor

@nodejs/streams

@mcollinamcollina 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.

Given that the PR mention 'close', it would be good to add a few mustCall()/mustNotCall() for that as well.

Comment threadtest/parallel/test-stream-writable-finish-destroyed.js Outdated
Comment threadtest/parallel/test-stream-writable-finish-destroyed.js Outdated
@ronag
ronagforce-pushed the stream-finish-after-close branch from f9154ec to 604191bCompareApril 25, 2020 17:35
@ronag

Copy link
Copy Markdown
MemberAuthor

added some missing mustCalls

@ronag
ronagforce-pushed the stream-finish-after-close branch from 604191b to 7da4656CompareApril 25, 2020 17:37
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadlib/_stream_readable.js Outdated
@ronag
ronagforce-pushed the stream-finish-after-close branch from 065eb38 to 706e189CompareApril 26, 2020 09:09
@ronag
ronag requested a review from mcollinaApril 26, 2020 21:56

@mcollinamcollina 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.

LGTM

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

ronag added a commit that referenced this pull request Apr 27, 2020
Writable stream could emit 'finish' after 'close'.
PR-URL: #32933
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ronag

Copy link
Copy Markdown
MemberAuthor

Landed in f5c11a1

@ronagronag closed this Apr 27, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
Writable stream could emit 'finish' after 'close'.
PR-URL: #32933
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Apr 27, 2020
@ronag

Copy link
Copy Markdown
MemberAuthor

@targos

Copy link
Copy Markdown
Member

It seems #34887 should unblock this backport. @ronag would you still like it to happen?

@ronag

Copy link
Copy Markdown
MemberAuthor

Yes. I'm a little overloaded at the moment though. If you need help ping me again in a. week :).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ronag@nodejs-github-bot@targos@mcollina@lpinca@BridgeAR