child_process: close pipe ends that are re-piped by gireeshpunathil · Pull Request #21209 · nodejs/node

gireeshpunathil · 2018-06-08T12:38:22Z

when t0 and t1 are spawned with t0's outputstream [1, 2] is piped into
t1's input, a new pipe is created which uses a copy of the t0's fd.
This leaves the original copy in Node parent, unattended. Net result is
that when t0 produces data, it gets bifurcated into both the copies

Detect the passed handle to be of 'wrap' type and close after the
native spawn invocation by which time piping would have been over.

Fixes: #9413
Fixes: #18016

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

addaleax · 2018-06-08T19:36:55Z

Can we add regression tests here?

gireeshpunathil · 2018-06-11T07:24:14Z

@addaleax - sure.

gireeshpunathil · 2018-06-11T12:28:56Z

@addaleax - done, please have a look!

addaleax

Makes sense!

mcollina

LGTM

mcollina · 2018-06-25T09:41:07Z

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

mcollina · 2018-06-25T09:49:06Z

@evanlucas @cjihrig are you ok with this landing?

cjihrig · 2018-06-25T14:36:39Z

In theory this LGTM, but the test introduced by this PR is failing in the CI.

gireeshpunathil · 2018-06-25T15:07:28Z

my test logic is very straight forward:

echo ${X} | grep '1' | wc -l === 272 where X is 0 to 1000

in windows (with vs2017) this seems to produce 1272.

could it be related to different semantics for vs2017?
could it be related to 4 parallel runs in widnwos? can they influence each other?
could it be related to the unix command emulators (echo, grep, wc etc.) are implemented by vs differently?

I will debug.

elibarzilay · 2018-07-08T22:24:58Z

@gireeshpunathil -- what's the status of this? (I tried to see the failure, but looks like the build results expired.)

gireeshpunathil · 2018-07-09T01:36:34Z

@elibarzilay - it is stuck on (some flavors of) windows failure of its own test case. I am running very low on bandwidth these weeks so kept it in the back burner. If you have some spare time to look at, please see why it fails on windows with VS 2017.

elibarzilay · 2018-07-09T02:27:22Z

@gireeshpunathil -- is there a way to re-run a build so I can see the failure? If not, then I'll try doing a Windows build, but that will obviously take more time...

gireeshpunathil · 2018-07-09T04:24:17Z

sure. @Trott or anyone from build team - can you please help us with running Windows CI against this PR (even better, if this single test is run in Windows) ? I can run full CI but doesn't look like the best way to use machines.

Trott · 2018-07-09T04:36:15Z

@gireeshpunathil https://ci.nodejs.org/job/node-stress-single-test/1933/nodes=win2016-1p-vs2017/ If I didn't mess up the parameters, that should build from this PR and run the test 10 times.

Trott · 2018-07-09T04:38:42Z

(Although due to the cross-compiling or whatever is used to speed up build in node-test-commit-windows-fanned, it might still be faster to run that. So here's one of those two: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19162/ Again, hopefully I didn't mess up any of the parameters...)

Trott

Marking with a red X just to make sure no one lands this with the test not working on Windows...

jasnell · 2018-09-10T18:19:11Z

Ping... any updates on this one?

gireeshpunathil · 2018-09-11T05:12:51Z

@jasnell - here is where it is stuck: I have developed a test case for this PR, that is consistently failing in some windows flavors (I wasn't successful in recreating it in my local machines). I am unable to correlate the failure with my test logic, as explained in
#21209 (comment).

I should admit I left it unattended for few weeks now. I shall work towards resolving this soon.

gireeshpunathil · 2019-02-04T03:41:25Z

The original test case posed challenge in windows: when the spawned child (cmd.exe) echo text, the echo data command as well as the echo'ed data, both were appearing in the data channel to the parent. If I do echo off, then both were turned off! I could not find a way to suppress the command alone while its output is printed.

So in the new version I made it simple, and platform independent - just write an MB of data in a file, with lines of one KB.

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

gireeshpunathil · 2019-02-04T05:01:12Z

CI is good.

@mcollina @jasnell @addaleax @cjihrig - I re-wrote the test case to fit windows; PTAL

@Trott - Windows is passing now, PTAL and dismiss your X mark.

mcollina

LGTM

Windows is passing

when t0 and t1 are spawned with t0's outputstream [1, 2] is piped into t1's input, a new pipe is created which uses a copy of the t0's fd. This leaves the original copy in Node parent, unattended. Net result is that when t0 produces data, it gets bifurcated into both the copies Detect the passed handle to be of 'wrap' type and close after the native spawn invocation by which time piping would have been over. Fixes: nodejs#9413Fixes: nodejs#18016 PR-URL: nodejs#21209 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

gireeshpunathil · 2019-02-06T12:24:06Z

landed as b1f82e4

when t0 and t1 are spawned with t0's outputstream [1, 2] is piped into t1's input, a new pipe is created which uses a copy of the t0's fd. This leaves the original copy in Node parent, unattended. Net result is that when t0 produces data, it gets bifurcated into both the copies Detect the passed handle to be of 'wrap' type and close after the native spawn invocation by which time piping would have been over. Fixes: #9413Fixes: #18016 PR-URL: #21209 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

refack · 2019-02-07T16:58:33Z

FYI: #25988 test is flaky on windows

gireeshpunathil · 2019-02-07T17:07:15Z

sure @refack , will have a look.

Closing the underlying resource completely has the unwanted side effect that the stream can no longer be used at all, including passing it to other child processes. What we want to avoid is accidentally reading from the stream; accordingly, it should be sufficient to stop its readable side manually, and otherwise leave the underlying resource intact. Fixes: nodejs#27097 Refs: nodejs#21209

Closing the underlying resource completely has the unwanted side effect that the stream can no longer be used at all, including passing it to other child processes. What we want to avoid is accidentally reading from the stream; accordingly, it should be sufficient to stop its readable side manually, and otherwise leave the underlying resource intact. Fixes: #27097 Refs: #21209 PR-URL: #27373 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 8, 2018

jasnell requested review from cjihrig and evanlucas June 9, 2018 00:59

addaleax approved these changes Jun 12, 2018
View reviewed changes

mcollina approved these changes Jun 25, 2018
View reviewed changes

jasnell approved these changes Jul 10, 2018
View reviewed changes

Trott previously requested changes Jul 10, 2018
View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

gireeshpunathil removed the stalled Issues and PRs that are stalled. label Feb 3, 2019

mcollina approved these changes Feb 4, 2019
View reviewed changes

jasnell approved these changes Feb 4, 2019
View reviewed changes

gireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2019

gireeshpunathil removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2019

gireeshpunathil force-pushed the close-bifurcated-stdio branch from 17258ca to b1f82e4Compare February 6, 2019 12:22

gireeshpunathil merged commit b1f82e4 into nodejs:masterFeb 6, 2019

targos mentioned this pull request Feb 14, 2019
v11.10.0 proposal #26098
Merged

gireeshpunathil mentioned this pull request Apr 5, 2019
Piping w/ spawn is broken in Node 11 / 12 #27097
Closed

addaleax mentioned this pull request Apr 23, 2019
child_process: only stop readable side of stream passed to process #27373
Closed
3 tasks

Uh oh!

Uh oh!

Conversation

gireeshpunathil commented Jun 8, 2018

Checklist

Uh oh!

addaleax commented Jun 8, 2018

Uh oh!

gireeshpunathil commented Jun 11, 2018

Uh oh!

gireeshpunathil commented Jun 11, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 25, 2018

Uh oh!

mcollina commented Jun 25, 2018

Uh oh!

cjihrig commented Jun 25, 2018

Uh oh!

gireeshpunathil commented Jun 25, 2018

Uh oh!

elibarzilay commented Jul 8, 2018

Uh oh!

gireeshpunathil commented Jul 9, 2018

Uh oh!

elibarzilay commented Jul 9, 2018

Uh oh!

gireeshpunathil commented Jul 9, 2018

Uh oh!

Trott commented Jul 9, 2018

Uh oh!

Trott commented Jul 9, 2018

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

gireeshpunathil commented Sep 11, 2018

Uh oh!

gireeshpunathil commented Feb 4, 2019

Uh oh!

gireeshpunathil commented Feb 4, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Feb 6, 2019

Uh oh!

refack commented Feb 7, 2019

Uh oh!

gireeshpunathil commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants