stream: always invoke end(cb) callback by ronag · Pull Request #29747 · nodejs/node · GitHub

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

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

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

@nodejs/streams

LGTM, out of precaution I would mark this semver-major.

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

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

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

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.

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

Excellent point. I'll take care of it.

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

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

LGTM

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

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

@Trott: My mistake. Should be fixed now.

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

@nodejs/tsc this needs some review

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

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

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

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

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

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

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

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>

Landed in 9d09969

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2019

ronag force-pushed the stream-end-err-cb branch from e57bf46 to a7008cd Compare September 28, 2019 08:26

ronag mentioned this pull request Sep 28, 2019

doc: error modes of writable #29745

Closed

2 tasks

ronag force-pushed the stream-end-err-cb branch 4 times, most recently from 64bf430 to 9a45936 Compare September 28, 2019 14:19

mcollina approved these changes Oct 1, 2019

View reviewed changes

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

jasnell approved these changes Oct 1, 2019

View reviewed changes

trivikr approved these changes Oct 1, 2019

View reviewed changes

mcollina requested changes Oct 2, 2019

View reviewed changes

ronag force-pushed the stream-end-err-cb branch 4 times, most recently from 1c093e6 to 35f00ac Compare October 2, 2019 15:01

ronag requested a review from mcollina October 2, 2019 17:39

ronag requested review from jasnell and trivikr October 3, 2019 08:08

ronag mentioned this pull request Oct 6, 2019

stream: call end(cb) callback after destroy #29859

Closed

4 tasks

ronag force-pushed the stream-end-err-cb branch from 4f42666 to fc45bf8 Compare October 6, 2019 16:31

ronag added 4 commits October 11, 2019 16:25

fixup: linting

c00303f

fixup: another deadlock case

e041ae3

fixup: don't use EE.listenerCount

f71dcd9

fixup: prepend error listener + test

aee2077

ronag force-pushed the stream-end-err-cb branch from c399fe1 to aee2077 Compare October 11, 2019 14:32

mcollina approved these changes Oct 11, 2019

View reviewed changes

fixup: fix finish order

0a482b7

jasnell approved these changes Oct 15, 2019

View reviewed changes

trivikr approved these changes Oct 15, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 12, 2019

addaleax closed this Nov 19, 2019

ronag mentioned this pull request Dec 8, 2019

http2: fix graceful session close #30854

Closed

4 tasks

ronag mentioned this pull request Jan 6, 2020

[v13.x backport] stream: invoke buffered write callbacks on error #31179

Closed

4 tasks

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

addaleax mentioned this pull request Mar 29, 2020

docs: end(cb) cb can be invoked with error #32238

Closed

3 tasks

stream: always invoke end(cb) callback by ronag · Pull Request #29747 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: always invoke end(cb) callback - #29747

stream: always invoke end(cb) callback#29747ronag wants to merge 9 commits intonodejs:masternodejs/node:masterfrom nxtedition:stream-end-err-cbnxtedition/node:stream-end-err-cbCopy head branch name to clipboard

Conversation

ronag commented Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Sep 28, 2019

Uh oh!

nodejs-github-bot commented Oct 1, 2019

Uh oh!

Trott commented Oct 1, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 1, 2019

Uh oh!

mcollina commented Oct 2, 2019

Uh oh!

targos commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Oct 2, 2019

Uh oh!

Trott commented Oct 2, 2019

Uh oh!

ronag commented Oct 6, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 11, 2019

Uh oh!

Trott commented Oct 11, 2019

Uh oh!

ronag commented Oct 11, 2019

Uh oh!

nodejs-github-bot commented Oct 12, 2019

Uh oh!

mcollina commented Oct 15, 2019

Uh oh!

nodejs-github-bot commented Oct 15, 2019

Uh oh!

nodejs-github-bot commented Oct 15, 2019

Uh oh!

ronag commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 12, 2019

Uh oh!

nodejs-github-bot commented Nov 12, 2019

Uh oh!

Trott commented Nov 12, 2019

Uh oh!

nodejs-github-bot commented Nov 15, 2019

Uh oh!

addaleax commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stream: always invoke end(cb) callback#29747
ronag wants to merge 9 commits into
nodejs:masterfrom
nxtedition:stream-end-err-cb

ronag commented Sep 28, 2019 •
edited
Loading

targos commented Oct 2, 2019 •
edited
Loading

ronag commented Nov 11, 2019 •
edited
Loading