stream: make finished try to wait for 'close' #32158

ronag · 2020-03-09T16:03:29Z

Pipeline uses eos which will invoke the callback
on 'finish' and 'end' before all streams have been
fully destroyed.

Fixes: #32032

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

ronag · 2020-03-09T16:03:39Z

This is waiting for/blocked by #32051

ronag · 2020-03-10T07:16:33Z

@vweevers

lib/internal/streams/pipeline.js

ronag · 2020-03-10T10:07:30Z

lib/internal/streams/end-of-stream.js

  let writableFinished = stream.writableFinished ||
    (wState && wState.finished);
  const onfinish = () => {
    writableFinished = true;
+    if (willEmitClose && (!stream.readable || readable)) return;


!stream.readable || readable is for the case where the stream is Duplex, but we might not want to wait for 'end' in which case autoDestroy won't be called and 'close' won't be emitted. An example of this is:

await pipeline(readable, duplex)

For the last argument duplex, pipeline will set readable: false explicitly for eos to indicate this.

mcollina

LGTM, I'm ok with this.

If the last stream in a pipeline is still usable/readable don't destroy it to allow further composition. Fixes: #32105 PR-URL: #32110 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

addaleax · 2020-03-13T08:57:17Z

@ronag I guess this is no longer blocked now, but needs a rebase?

addaleax · 2020-03-13T09:53:28Z

@ronag I’m not sure if this is a new conflict, but the Github UI still says that there are some… also, when adding author ready, make sure that the PR has a fresh CI run :)

nodejs-github-bot · 2020-03-13T10:31:57Z

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

nodejs-github-bot · 2020-03-13T12:52:59Z

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

nodejs-github-bot · 2020-03-20T22:24:48Z

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

ronag · 2020-03-23T16:14:32Z

needs another @nodejs/tsc approval

nodejs-github-bot · 2020-03-23T16:15:43Z

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

test/parallel/test-stream-pipeline.js

nodejs-github-bot · 2020-03-23T22:25:23Z

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

ronag · 2020-03-25T19:27:19Z

rebased to fix conflicts

Pipeline uses eos which will invoke the callback on 'finish' and 'end' before all streams have been fully destroyed. Fixes: nodejs#32032

nodejs-github-bot · 2020-03-25T20:22:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30066/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2327/

nodejs-github-bot · 2020-03-25T21:43:16Z

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

nodejs-github-bot · 2020-03-26T00:38:20Z

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

nodejs-github-bot · 2020-03-26T09:23:47Z

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

nodejs-github-bot · 2020-03-26T10:06:28Z

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

mcollina

lgtm

nodejs-github-bot · 2020-03-26T12:46:40Z

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

ronag · 2020-03-26T15:49:54Z

green CI & CITGM, waiting for another @nodejs/tsc approval

Pipeline uses eos which will invoke the callback on 'finish' and 'end' before all streams have been fully destroyed. Fixes: #32032 PR-URL: #32158 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2020-03-27T14:56:00Z

Landed in 1428a92

ronag added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. blocked PRs that are blocked by other issues or PRs. labels Mar 9, 2020

ronag force-pushed the pipeline-wait-for-close branch 3 times, most recently from 18c3de4 to c13abc8 Compare March 9, 2020 16:33

ronag force-pushed the pipeline-wait-for-close branch 2 times, most recently from 747fbf7 to 059b055 Compare March 10, 2020 07:21

vweevers reviewed Mar 10, 2020

View reviewed changes

lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved

ronag force-pushed the pipeline-wait-for-close branch from 059b055 to 8f14903 Compare March 10, 2020 10:00

ronag changed the title ~~stream: make pipeline try to wait for 'close'~~ stream: make finished try to wait for 'close' Mar 10, 2020

ronag force-pushed the pipeline-wait-for-close branch 2 times, most recently from 9725ba8 to 2fc0856 Compare March 10, 2020 10:03

ronag commented Mar 10, 2020

View reviewed changes

mcollina approved these changes Mar 10, 2020

View reviewed changes

ronag force-pushed the pipeline-wait-for-close branch from 2fc0856 to c6cb45b Compare March 11, 2020 15:50

This comment has been minimized.

Sign in to view

szmarczak mentioned this pull request Mar 11, 2020

Broken with Node.js ≥ 13.10.0 sindresorhus/got#1107

Closed

2 tasks

ronag force-pushed the pipeline-wait-for-close branch from c6cb45b to d244303 Compare March 13, 2020 09:23

ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Mar 13, 2020

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2020

ronag force-pushed the pipeline-wait-for-close branch from d244303 to 4c83a29 Compare March 13, 2020 10:06

jasnell reviewed Mar 23, 2020

View reviewed changes

test/parallel/test-stream-pipeline.js Outdated Show resolved Hide resolved

ronag requested a review from jasnell March 24, 2020 08:49

ronag force-pushed the pipeline-wait-for-close branch from 735f412 to 81bdea7 Compare March 25, 2020 19:27

This comment has been minimized.

Sign in to view

stream: make pipeline try to wait for 'close'

4f76b58

Pipeline uses eos which will invoke the callback on 'finish' and 'end' before all streams have been fully destroyed. Fixes: nodejs#32032

ronag force-pushed the pipeline-wait-for-close branch from 81bdea7 to 4f76b58 Compare March 25, 2020 19:58

fixup: remove no longer necessary check

c8020c4

mcollina approved these changes Mar 26, 2020

View reviewed changes

addaleax approved these changes Mar 27, 2020

View reviewed changes

addaleax closed this Mar 27, 2020

BethGriggs mentioned this pull request Apr 7, 2020

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

Merged

ronag mentioned this pull request Apr 20, 2020

stream: ensure pipeline always destroys streams #31940

Closed

4 tasks

aduh95 mentioned this pull request Apr 24, 2020

Undocumented breaking change in stream.pipeline? #33050

Closed

ronag mentioned this pull request Apr 25, 2020

doc: document major finished changes in v14 #33065

Closed

4 tasks

vweevers mentioned this pull request Jan 24, 2022

stream: add reduce #41669

Merged

wh0 mentioned this pull request Jan 22, 2024

stream.pipeline doesn't wait for 'close' on error #51540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: make finished try to wait for 'close' #32158

stream: make finished try to wait for 'close' #32158

ronag commented Mar 9, 2020 •

edited

Loading

ronag commented Mar 9, 2020 •

edited

Loading

ronag commented Mar 10, 2020

ronag Mar 10, 2020 •

edited

Loading

mcollina left a comment

This comment has been minimized.

addaleax commented Mar 13, 2020

addaleax commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 20, 2020

ronag commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

ronag commented Mar 25, 2020

This comment has been minimized.

nodejs-github-bot commented Mar 25, 2020 •

edited by ronag

Loading

nodejs-github-bot commented Mar 25, 2020

nodejs-github-bot commented Mar 26, 2020

nodejs-github-bot commented Mar 26, 2020

nodejs-github-bot commented Mar 26, 2020

mcollina left a comment

nodejs-github-bot commented Mar 26, 2020

ronag commented Mar 26, 2020

addaleax commented Mar 27, 2020

stream: make finished try to wait for 'close' #32158

stream: make finished try to wait for 'close' #32158

Conversation

ronag commented Mar 9, 2020 • edited Loading

Checklist

ronag commented Mar 9, 2020 • edited Loading

ronag commented Mar 10, 2020

ronag Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

This comment has been minimized.

addaleax commented Mar 13, 2020

addaleax commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 20, 2020

ronag commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

ronag commented Mar 25, 2020

This comment has been minimized.

nodejs-github-bot commented Mar 25, 2020 • edited by ronag Loading

nodejs-github-bot commented Mar 25, 2020

nodejs-github-bot commented Mar 26, 2020

nodejs-github-bot commented Mar 26, 2020

nodejs-github-bot commented Mar 26, 2020

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 26, 2020

ronag commented Mar 26, 2020

addaleax commented Mar 27, 2020

ronag commented Mar 9, 2020 •

edited

Loading

ronag commented Mar 9, 2020 •

edited

Loading

ronag Mar 10, 2020 •

edited

Loading

nodejs-github-bot commented Mar 25, 2020 •

edited by ronag

Loading