child_process.flushStdio: resume _consuming streams by davidvgalbraith · Pull Request #4071 · nodejs/node

davidvgalbraith · 2015-11-30T10:53:51Z

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 · 2015-11-30T15:40:38Z

cc @nodejs/streams

Fishrock123 · 2015-11-30T15:41:44Z

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

cjihrig · 2015-11-30T16:08:44Z

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 · 2015-11-30T16:15:05Z

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

cjihrig · 2015-11-30T16:19:50Z

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

davidvgalbraith · 2015-11-30T18:34:27Z

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 · 2015-11-30T19:08:05Z

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 · 2015-11-30T19:57:45Z

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

cjihrig · 2015-11-30T21:04:35Z

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 · 2015-11-30T21:10:44Z

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

davidvgalbraith · 2015-11-30T21:13:31Z

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

Fishrock123 · 2015-11-30T21:20:07Z

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

davidvgalbraith · 2015-11-30T21:23:16Z

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

cjihrig · 2015-11-30T21:30:33Z

@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 · 2015-12-01T15:03:15Z

Thanks! Landed in 34b535f

jasnell · 2015-12-02T07:12:26Z

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

cjihrig · 2015-12-02T15:55:49Z

I would consider this a bug fix.

silverwind · 2015-12-03T18:42:00Z

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 · 2015-12-03T18:43:41Z

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 · 2015-12-29T23:21:52Z

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

Comment threadtest/parallel/test-child-process-flush-stdio.js Outdated
cjihrigNov 30, 2015
Copy link
Copy Markdown
Contributor
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

Uh oh!

Uh oh!

Conversation

davidvgalbraith commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

Fishrock123 commented Nov 30, 2015

Uh oh!

cjihrigNov 30, 2015

Choose a reason for hiding this comment

Uh oh!

calvinmetcalfNov 30, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123Nov 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!

cjihrigNov 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