child_process.flushStdio: resume _consuming streams #4071

davidvgalbraith · Nov 30, 2015

Hey! I'm new here. Tried to follow all the guidelines. Thanks for your attention!

When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
child_process.flushStdio currently doesn't flush
any streams where _consuming is truthy. But that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049.

child_process.flushStdio should flush streams even if their
_consuming is set to true. Then it will close even after a read.

When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. child_process.flushStdio currently doesn't flush any streams where _consuming is truthy. But that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue nodejs#4049. child_process.flushStdio should flush streams even if their _consuming is set to true. Then it will close even after a read.

Fishrock123 · Nov 30, 2015

cc @nodejs/streams

Fishrock123 · Nov 30, 2015

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

cjihrig · Nov 30, 2015

test/sequential/test-child-process-flush-stdio.js

Is yes a valid Windows command?

I had my coworker on windows type yes into the prompt and it is not a command

Can also confirm it is not.

cjihrig · Nov 30, 2015

Is there a reason the test needs to be in sequential instead of parallel?

cjihrig · Nov 30, 2015

A couple of questions about the test, but mostly LGTM. I would like sign off from a streams person if possible.

davidvgalbraith · Nov 30, 2015

I moved the test into parallel, and I changed its use of yes to the cross-platform echo, double-checking that it still fails on master and passes on this branch.

cjihrig · Nov 30, 2015

test/parallel/test-child-process-flush-stdio.js

Can you please make this const.

Make the spawned process object a const, and add assertions to the event handler that the arguments are as expected

davidvgalbraith · Nov 30, 2015

I made p a const, and I added validating assertions to the close event handler.

cjihrig · Nov 30, 2015

Started a CI run: https://ci.nodejs.org/job/node-test-pull-request/883/

The linter doesn't like your indentation - https://ci.nodejs.org/job/node-test-linter/434/console

Fishrock123 · Nov 30, 2015

@cjihrig note: currently the CI is private. See #4029 (comment)

davidvgalbraith · Nov 30, 2015

How embarrassing. I've indented the requisite two spaces. Forsooth I can't see anything at the CI link.

Fishrock123 · Nov 30, 2015

@davidvgalbraith could you run make jslint? That should do the trick. :)

davidvgalbraith · Nov 30, 2015

make jshint is looking green as of davidvgalbraith@127a82e.

cjihrig · Nov 30, 2015

@Fishrock123 good catch about the CI being private.

@davidvgalbraith the CI looks fine. There were a couple failures unrelated to this change. LGTM

When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. ChildProcess.flushStdio() currently doesn't flush any streams where _consuming is truthy. But, that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue #4049. This commit allows consuming streams to be flushed, and the child process to emit a close event. Fixes: #4049 PR-URL: #4071 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig · Dec 1, 2015

Thanks! Landed in 34b535f

jasnell · Dec 2, 2015

@cjihrig ... would you consider this a bug fix (and thus suitable for LTS) or an add?

cjihrig · Dec 2, 2015

I would consider this a bug fix.

silverwind · Dec 3, 2015

Looks like this new test here is failing on CentOS:

https://ci.nodejs.org/job/node-test-commit-linux/1387/nodes=centos5-32/tapResults/
https://ci.nodejs.org/job/node-test-commit-linux/1387/nodes=centos5-64/tapResults/

    test-child-process-flush-stdio.js   
    # Mismatched &lt;anonymous&gt; function calls. Expected 1, actual 0.
    # at Object.&lt;anonymous&gt; (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/test/parallel/test-child-process-flush-stdio.js:8:22)
    # at Module._compile (module.js:399:26)
    # at Object.Module._extensions..js (module.js:406:10)
    # at Module.load (module.js:345:32)
    # at Function.Module._load (module.js:302:12)
    # at Function.Module.runMain (module.js:431:10)
    # at startup (node.js:138:18)
    # at node.js:976:3

cjihrig · Dec 3, 2015

Yes. See #4125

When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. ChildProcess.flushStdio() currently doesn't flush any streams where _consuming is truthy. But, that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue #4049. This commit allows consuming streams to be flushed, and the child process to emit a close event. Fixes: #4049 PR-URL: #4071 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · Dec 29, 2015

Not going to land this until we are ready to land #4215 into LTS... I think it needs a bit more time

When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. ChildProcess.flushStdio() currently doesn't flush any streams where _consuming is truthy. But, that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue #4049. This commit allows consuming streams to be flushed, and the child process to emit a close event. Fixes: #4049 PR-URL: #4071 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. ChildProcess.flushStdio() currently doesn't flush any streams where _consuming is truthy. But, that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue nodejs#4049. This commit allows consuming streams to be flushed, and the child process to emit a close event. Fixes: nodejs#4049 PR-URL: nodejs#4071 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

davidvgalbraith mentioned this pull request Nov 30, 2015

Child process/streams memory leak #4049

Closed

ChALkeR added the child_process Issues and PRs related to the child_process subsystem. label Nov 30, 2015

cjihrig reviewed Nov 30, 2015
View reviewed changes

cjihrig mentioned this pull request Nov 30, 2015

child_process: closes before flushing all data #4074

Closed

child-process-flush-stdio-test: rename and use echo

d13f74f

cjihrig reviewed Nov 30, 2015
View reviewed changes

test/parallel/test-child-process-flush-stdio.js Outdated

Copy link

Contributor

cjihrig Nov 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please make this const.

test-child-process-flush-stdio: review changes

4becc69

Make the spawned process object a const, and add assertions to the event handler that the arguments are as expected

test-child-process-flush-stdio: indent 2 spaces

127a82e

cjihrig closed this Dec 1, 2015

This was referenced Dec 8, 2015

Release proposal: v5.2.0 #4180

Closed

Release proposal: v5.2.0 (Stable) #4181

Merged

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

This was referenced Jun 8, 2016

'close' event for spawn() isn't emitted under unknown conditions #7159

Closed

Streams Regression in v4.x nodejs/Release#112

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

child_process.flushStdio: resume _consuming streams #4071

child_process.flushStdio: resume _consuming streams #4071

Uh oh!

Conversation

davidvgalbraith commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

cjihrig Nov 30, 2015

Choose a reason for hiding this comment

Uh oh!

calvinmetcalf Nov 30, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Nov 30, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 30, 2015

Uh oh!

cjihrig commented Nov 30, 2015

Uh oh!

davidvgalbraith commented Nov 30, 2015

Uh oh!

cjihrig Nov 30, 2015

Choose a reason for hiding this comment

Uh oh!

davidvgalbraith commented Nov 30, 2015

Uh oh!

cjihrig commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

davidvgalbraith commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

davidvgalbraith commented Nov 30, 2015

Uh oh!

cjihrig commented Nov 30, 2015

Uh oh!

cjihrig commented Dec 1, 2015

Uh oh!

jasnell commented Dec 2, 2015

Uh oh!

cjihrig commented Dec 2, 2015

Uh oh!

silverwind commented Dec 3, 2015

Uh oh!

cjihrig commented Dec 3, 2015

Uh oh!

MylesBorins commented Dec 29, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants