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.

Sign up for GitHub

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

Jump to bottom

Lost input from child processes due to initially-flowing net.Socket. #5034

Closed
awpr opened this issue Feb 2, 2016 · 3 comments
Closed

Lost input from child processes due to initially-flowing net.Socket. #5034

awpr opened this issue Feb 2, 2016 · 3 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@awpr
Copy link

awpr commented Feb 2, 2016

When opening child processes with child_process.spawn using 'pipe' for its stdout or stderr, the resulting stream is already in the flowing state when it's returned, and some data can be lost when consuming the stream using the 'readable' event + read(). A minimal example:

const spawn = require('child_process').spawn;
const ls = spawn('echo', ['123']);

console.log("flowing:", ls.stdout._readableState.flowing);
ls.stdout.on('readable', function() {
console.log("read:", ls.stdout.read());
});

On recent node versions, no data is read; the 'readable' event only fires once, for end-of-stream. On older versions, the first chunk of data is read. I'm not sure which exact version introduced the bug, but:

% node --version && node /tmp/test.js
v0.10.36
flowing: false
read: null
read: \<Buffer 31 32 33 0a\>
read: null

% node --version && node /tmp/test.js
v5.1.0
flowing: null
read: null

% node --version && node /tmp/test.js
v5.5.0
flowing: null
read: null

Potentially related to #445. Seems to be the root cause of a few issues affecting GHCJS:

The interaction between https://github.com/nodejs/node/blob/master/lib/internal/child_process.js#L228 and https://github.com/nodejs/node/blob/master/lib/net.js#L165 looks suspicious to me, but I don't know enough about nodejs' internals to suggest an appropriate fix.
@brendanashworth brendanashworth added child_process Issues and PRs related to the child_process subsystem. net Issues and PRs related to the net subsystem. labels Feb 2, 2016
@awpr
Copy link
Author

awpr commented Feb 2, 2016

Also relevant, replacing the 'echo' with a read of a named pipe shows different behavior based on how quickly input becomes available on the pipe:

const spawn = require('child_process').spawn;
const ls = spawn('cat', ['/tmp/the_named_fifo']);

% cat /tmp/test.js
console.log("flowing:", ls.stdout._readableState.flowing);
ls.stdout.on('readable', function() {
console.log("read:", ls.stdout.read());
});

% mkfifo /tmp/the_named_fifo

% node --version && node /tmp/test.js & echo hi >> /tmp/the_named_fifo; wait
v5.1.0
[1] 13721
flowing: null
read: null
[1] + done node /tmp/test.js

% node --version && node /tmp/test.js & sleep 1; echo hi >> /tmp/the_named_fifo; wait
v5.1.0
[1] 13730
flowing: null
read: <Buffer 68 69 0a>
read: null
[1] + done node /tmp/test.js

@mscdex
Copy link
Contributor

mscdex commented Feb 2, 2016

I think I have a fix for this coming up ...

@mscdex mscdex mentioned this issue Feb 2, 2016
Closed
@mscdex
Copy link
Contributor

mscdex commented Feb 2, 2016

#5036 should fix this

@mscdex mscdex removed the net Issues and PRs related to the net subsystem. label Feb 2, 2016
@mscdex mscdex mentioned this issue Feb 2, 2016
Closed
mscdex added a commit to mscdex/io.js that referenced this issue Feb 4, 2016
@mscdex
child_process: fix data loss with readable event
e528bdc 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: nodejs#5034
mscdex added a commit to mscdex/io.js that referenced this issue Feb 4, 2016
@mscdex
child_process: fix data loss with readable event
3e7af8d 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: nodejs#5034
@mscdex mscdex closed this as completed in 12274a5 Feb 11, 2016
mscdex added a commit that referenced this issue Feb 11, 2016
@mscdex
child_process: fix data loss with readable event
18ab5d4 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5037
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 11, 2016
@mscdex
child_process: fix data loss with readable event
6c468df 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5037
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Feb 15, 2016
@mscdex @rvagg
child_process: fix data loss with readable event
9e6ad2d 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5036
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
stefanmb pushed a commit to stefanmb/node that referenced this issue Feb 23, 2016
@mscdex @stefanmb
child_process: fix data loss with readable event
6438449 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: nodejs#5034
PR-URL: nodejs#5036
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 1, 2016
@mscdex
child_process: fix data loss with readable event
d59d379 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5036
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 1, 2016
@mscdex
child_process: fix data loss with readable event
29951cf 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5036
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 2, 2016
@mscdex
child_process: fix data loss with readable event
ab787ad 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: #5034
PR-URL: #5036
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@3noch 3noch mentioned this issue Mar 15, 2016
Closed
jBarz pushed a commit to ibmruntimes/node that referenced this issue Nov 4, 2016
@mscdex @jBarz
child_process: fix data loss with readable event
4500518 
This commit prevents child process stdio streams from being
automatically flushed on child process exit/close if a 'readable'
event handler has been attached at the time of exit.

Without this, child process stdio data can be lost if the process
exits quickly and a `read()` (e.g. from a 'readable' handler)
hasn't had the chance to get called yet.

Fixes: nodejs/node#5034
PR-URL: nodejs/node#5037
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

child_process: fix data loss with readable event mscdex/io.js
3 participants
@mscdex @awpr @brendanashworth