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#7606Fixes: 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: #7606Fixes: #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: #7606Fixes: #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#7606Fixes: 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 e9e4444Compare 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 d9d992aCompare 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

Uh oh!

stream: complete pipeline with stdio#32373

stream: complete pipeline with stdio #32373

Uh oh!

Conversation

ronag commented Mar 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Mar 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Mar 19, 2020

Uh oh!

BridgeARMar 19, 2020

Choose a reason for hiding this comment

Uh oh!

ronagMar 19, 2020

Choose a reason for hiding this comment

Uh oh!

ronagMar 19, 2020

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 20, 2020

Uh oh!

ronag commented Mar 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Mar 20, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 5, 2020

Uh oh!

nodejs-github-bot commented Apr 5, 2020

Uh oh!

ronag commented Apr 6, 2020

Uh oh!

vweevers commented Apr 6, 2020

Uh oh!

ronag commented Apr 6, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

vweevers commented Apr 6, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 6, 2020

Uh oh!

ronag commented Apr 6, 2020

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Apr 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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