stream: complete pipeline with stdio #32373

ronag · 2020-03-19T21:41:04Z

stdio (stderr & stdout) should for compatibility
reasons not be closed/end():ed. However, this
causes pipeline with a stdio destination to
never finish. This commit fixes this issue at
a performance cost.

Refs: #7606

Fixes: #32363

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

stdio (stderr & stdout) should for compatibility reasons not be closed/end():ed. However, this causes pipeline with a stdio destination to never finish. This commit fixes this issue at a performance cost. Refs: nodejs#7606 Fixes: nodejs#32363

ronag · 2020-03-19T21:42:24Z

I'm not sure I understand/agree with not calling end() on stdio streams. However, this PR resolves the issue of pipeline not completing at a performance cost. This is the best solution I've found so far...

ronag · 2020-03-19T21:46:25Z

@vweevers

BridgeAR · 2020-03-19T21:54:30Z

lib/internal/streams/pipeline.js

+              err ? reject(err) : resolve();
+            });
+          })
+        ]);


It is likely faster to use the callbacks and manually handle everything. It could be wrapped in a single promise to allow to await the result for simplicity. Alternatively just also handle the finish call separately.

It is likely faster to use the callbacks and manually handle everything. It could be wrapped in a single promise to allow to await the result for simplicity

Yes, but that makes the implementation more complicated... at least as far as I can determine.

@BridgeAR PTAL

mcollina · 2020-03-20T08:38:55Z

What is the performance cost that you are mentioning?

ronag · 2020-03-20T08:56:34Z

New information from @vweevers which might make this PR outdated.

Ah, my understanding of it was outdated. Since node v10.12.0 (see #23053 and ERR_STDOUT_CLOSE) you can indeed close these streams. Before they used to throw an error, now they allow it but "secretly" don't close the underlying fd.

ronag · 2020-03-20T09:13:53Z

I'm putting this as blocked until we have gotten further clarification in the issue:

#32363

mcollina

lgtm

nodejs-github-bot · 2020-04-05T22:22:00Z

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

nodejs-github-bot · 2020-04-05T22:52:18Z

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

ronag · 2020-04-06T16:25:25Z

@addaleax calling end() on stdout seems to have different behavior on Windows.

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/2974/RUN_SUBSET=0,nodes=win10-COMPILED_BY-vs2019/testReport/junit/(root)/test/parallel_test_stream_pipeline_process/

Any thoughts?

vweevers · 2020-04-06T17:56:17Z

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '"hello" \r\n'
- 'hello\n'

@ronag That's the behavior of echo "hello" on Windows, you'd get the same result with echo "hello" > test. I suggest removing the double quotes and trimming the result.

ronag · 2020-04-06T17:58:06Z

@ronag That's the behavior of echo "hello" on Windows, you'd get the same result with echo "hello" > test. I suggest removing the double quotes and trimming the result.

So we expect different results on different platforms?

I suggest removing the double quotes and trimming the result.

I don't think that's enough, note the extra space and \r.

vweevers · 2020-04-06T18:03:50Z

The default newline on windows is \r\n. The extra space is because echo foo > test adds a space but echo foo> test does not. There are ways around this but normalizing such shell behaviors would not make this node.js test more meaningful.

nodejs-github-bot · 2020-04-06T18:23:15Z

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

ronag · 2020-04-06T19:41:25Z

@mcollina @addaleax PTAL, made test compatible with Windows

addaleax

Still LGTM

stdio (stderr & stdout) should for compatibility reasons not be closed/end():ed. However, this causes pipeline with a stdio destination to never finish. This commit fixes this issue at a performance cost. Refs: #7606 Fixes: #32363 PR-URL: #32373 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

ronag · 2020-04-06T19:49:01Z

Landed in 98b6b2d

stdio (stderr & stdout) should for compatibility reasons not be closed/end():ed. However, this causes pipeline with a stdio destination to never finish. This commit fixes this issue at a performance cost. Refs: #7606 Fixes: #32363 PR-URL: #32373 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

stream: complete pipeline with stdio

571e867

stdio (stderr & stdout) should for compatibility reasons not be closed/end():ed. However, this causes pipeline with a stdio destination to never finish. This commit fixes this issue at a performance cost. Refs: nodejs#7606 Fixes: nodejs#32363

ronag requested a review from mcollina March 19, 2020 21:41

ronag added the stream Issues and PRs related to the stream subsystem. label Mar 19, 2020

jasnell approved these changes Mar 19, 2020

View reviewed changes

ronag added 2 commits March 19, 2020 22:46

fixup: isStdio helper

ab76605

fixup: test

84cf2aa

ronag added 2 commits March 19, 2020 22:47

fixup: typo

b2925d8

fixup: test

bb882a2

BridgeAR reviewed Mar 19, 2020

View reviewed changes

ronag added 2 commits March 19, 2020 22:55

fixup: unhandledRejection

1d0a7a5

fixup: faster version

3444617

ronag added the wip Issues and PRs that are still a work in progress. label Mar 19, 2020

fixup: unify

e9e4444

ronag force-pushed the pipeline-stdio branch from 327a0a9 to e9e4444 Compare March 19, 2020 23:42

ronag removed the wip Issues and PRs that are still a work in progress. label Mar 19, 2020

ronag added the wip Issues and PRs that are still a work in progress. label Mar 20, 2020

ronag added the blocked PRs that are blocked by other issues or PRs. label Mar 20, 2020

fixup: end stdio

d9d992a

ronag force-pushed the pipeline-stdio branch from 8dc9d3a to d9d992a Compare March 20, 2020 09:16

addaleax approved these changes Apr 5, 2020

View reviewed changes

mcollina approved these changes Apr 5, 2020

View reviewed changes

ronag removed blocked PRs that are blocked by other issues or PRs. wip Issues and PRs that are still a work in progress. labels Apr 5, 2020

ronag mentioned this pull request Apr 6, 2020

Different behavior between transform function and async generator using pipelines #32363

Closed

ronag added 2 commits April 6, 2020 20:01

fixup

1498bb3

fixup

2feb611

This comment has been minimized.

Sign in to view

ronag added 2 commits April 6, 2020 20:05

fixup

e2ef9c9

fixup

51d45eb

nodejs deleted a comment from nodejs-github-bot Apr 6, 2020

addaleax approved these changes Apr 6, 2020

View reviewed changes

ronag closed this Apr 6, 2020

targos mentioned this pull request Apr 13, 2020

v13.13.0 release proposal #32813

Merged

targos added the backport-blocked-v12.x label Apr 22, 2020

ronag mentioned this pull request May 4, 2020

stdout not usable after pipeline? #33237

Closed

ronag mentioned this pull request Jun 26, 2020

Stream.pipeline() does not call the callback on completion #34059

Closed

mcollina mentioned this pull request Jul 19, 2021

stream.pipeline does not invoke callback when error happens #39447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: complete pipeline with stdio #32373

stream: complete pipeline with stdio #32373

ronag commented Mar 19, 2020 •

edited

Loading

ronag commented Mar 19, 2020 •

edited

Loading

ronag commented Mar 19, 2020

BridgeAR Mar 19, 2020

ronag Mar 19, 2020

ronag Mar 19, 2020

mcollina commented Mar 20, 2020

ronag commented Mar 20, 2020 •

edited

Loading

ronag commented Mar 20, 2020

mcollina left a comment

nodejs-github-bot commented Apr 5, 2020

nodejs-github-bot commented Apr 5, 2020

ronag commented Apr 6, 2020

vweevers commented Apr 6, 2020

ronag commented Apr 6, 2020 •

edited

Loading

This comment has been minimized.

vweevers commented Apr 6, 2020 •

edited

Loading

nodejs-github-bot commented Apr 6, 2020

ronag commented Apr 6, 2020

addaleax left a comment

ronag commented Apr 6, 2020

stream: complete pipeline with stdio #32373

stream: complete pipeline with stdio #32373

Conversation

ronag commented Mar 19, 2020 • edited Loading

Checklist

ronag commented Mar 19, 2020 • edited Loading

ronag commented Mar 19, 2020

BridgeAR Mar 19, 2020

Choose a reason for hiding this comment

ronag Mar 19, 2020

Choose a reason for hiding this comment

ronag Mar 19, 2020

Choose a reason for hiding this comment

mcollina commented Mar 20, 2020

ronag commented Mar 20, 2020 • edited Loading

ronag commented Mar 20, 2020

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 5, 2020

nodejs-github-bot commented Apr 5, 2020

ronag commented Apr 6, 2020

vweevers commented Apr 6, 2020

ronag commented Apr 6, 2020 • edited Loading

This comment has been minimized.

vweevers commented Apr 6, 2020 • edited Loading

nodejs-github-bot commented Apr 6, 2020

ronag commented Apr 6, 2020

addaleax left a comment

Choose a reason for hiding this comment

ronag commented Apr 6, 2020

ronag commented Mar 19, 2020 •

edited

Loading

ronag commented Mar 19, 2020 •

edited

Loading

ronag commented Mar 20, 2020 •

edited

Loading

ronag commented Apr 6, 2020 •

edited

Loading

vweevers commented Apr 6, 2020 •

edited

Loading