Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process.flushStdio: resume _consuming streams #4071

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
child_process.flushStdio: resume _consuming streams
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.
  • Loading branch information
Dave committed Nov 30, 2015
commit f2c8cb5a7fb321c3d814ddeefe6e10517360f73c
2 changes: 1 addition & 1 deletion lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ util.inherits(ChildProcess, EventEmitter);
function flushStdio(subprocess) {
if (subprocess.stdio == null) return;
subprocess.stdio.forEach(function(stream, fd, stdio) {
if (!stream || !stream.readable || stream._consuming)
if (!stream || !stream.readable)
return;
stream.resume();
});
Expand Down
13 changes: 13 additions & 0 deletions test/sequential/test-child-process-flush-stdio.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';
const cp = require('child_process');
const common = require('../common');

var p = cp.spawn('yes');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is yes a valid Windows command?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can also confirm it is not.


p.on('close', common.mustCall(function() {}));

p.stdout.read();

setTimeout(function() {
p.kill();
}, 100);