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: always invoke end(cb) callback #29747

Closed
wants to merge 9 commits into from
Closed

stream: always invoke end(cb) callback #29747

wants to merge 9 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 28, 2019 •
edited
Loading

Ensure that the callback passed into end() is always invoked in order to avoid bug such as deadlock the user.

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 28, 2019
@ronag ronag mentioned this pull request Sep 28, 2019
Closed
2 tasks
@ronag ronag force-pushed the stream-end-err-cb branch 4 times, most recently from 64bf430 to 9a45936 Compare September 28, 2019 14:19
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Oct 1, 2019

@nodejs/streams

mcollina
mcollina approved these changes Oct 1, 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, out of precaution I would mark this semver-major.

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 1, 2019
@Trott
Copy link
Member

Trott commented Oct 1, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2022/ (queued)

@mcollina
Copy link
Member

mcollina commented Oct 2, 2019

I'm worried about https://www.npmjs.com/package/csv-parser failing on CITGM

@targos
Copy link
Member

targos commented Oct 2, 2019 •
edited
Loading

csv-parser fails in all recent CITGM runs. It's not related to this change

mcollina
mcollina requested changes Oct 2, 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.

This change has an interesting side effect, and I think we should try to avoid it. Calling stream.end('hello', cb) is going to prevent the error to bubble up to 'uncaughtException' if there are no other 'error' handlers.
You might want to re-emit the error if there are no other 'error' listeners.

@ronag
Copy link
Member Author

ronag commented Oct 2, 2019

You might want to re-emit the error if there are no other 'error' listeners.

Excellent point. I'll take care of it.

@ronag ronag force-pushed the stream-end-err-cb branch 4 times, most recently from 1c093e6 to 35f00ac Compare October 2, 2019 15:01
@ronag ronag requested a review from mcollina October 2, 2019 17:39
@Trott
Copy link
Member

Trott commented Oct 2, 2019

For anyone following along in the conversation: csv-parser should be fixed by sindresorhus/execa#370 and is indeed unrelated to these changes

@ronag ronag requested review from jasnell and trivikr October 3, 2019 08:08
@ronag ronag mentioned this pull request Oct 6, 2019
Closed
4 tasks
@ronag
Copy link
Member Author

ronag commented Oct 6, 2019

Found another case where the callback is not called. Fixed.

mcollina
mcollina approved these changes Oct 11, 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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Oct 11, 2019

Relevant test failures in test.parallel/test-stream-transform-final-sync and test.parallel/test-stream-transform-final.

@ronag
Copy link
Member Author

ronag commented Oct 11, 2019

@Trott: My mistake. Should be fixed now.

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina
Copy link
Member

@nodejs/tsc this needs some review

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Nov 11, 2019 •
edited
Loading

@Trott: This needs TSC review (see Matteo's comment)? Can we put tsc label on this or how does that work?

@Trott
Copy link
Member

Trott commented Nov 12, 2019

@Trott: This needs TSC review (see Matteo's comment)? Can we put tsc label on this or how does that work?

I think @mcollina just meant that this is semver-major so it needed a second approval from a TSC member other than him. Subsequently, @jasnell approved it, and that would be the second approval. This can land if CITGM and CI are good.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Nov 12, 2019

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

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

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

addaleax pushed a commit that referenced this pull request Nov 19, 2019
@ronag @addaleax
stream: always invoke end callback
9d09969 
Ensure that the callback passed into end() is always invoke in
order to avoid bug such as deadlock the user.

PR-URL: #29747
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax
Copy link
Member

Landed in 9d09969

@addaleax addaleax closed this Nov 19, 2019
@ronag ronag mentioned this pull request Dec 8, 2019
Closed
4 tasks
@ronag ronag mentioned this pull request Jan 6, 2020
Closed
4 tasks
@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
@addaleax addaleax mentioned this pull request Mar 29, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@jasnell jasnell jasnell 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. 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.
8 participants
@ronag @nodejs-github-bot @Trott @mcollina @targos @addaleax @jasnell @trivikr