This repository has been archived by the owner. It is now read-only.

child_process.spawn with stdio blows up with invalid argument #4030

Open
davepacheco opened this Issue Sep 18, 2012 · 8 comments

Comments

Projects
None yet
5 participants

I expected the following program to be equivalent to "ls > foo.txt":

var child = require('child_process');
var fs = require('fs');

child.spawn('ls', [], {
    'stdio': [ null, fs.createWriteStream('foo.txt') ]
});

but instead, it blew up:

child_process.js:768
      throw new TypeError('Incorrect value for stdio stream: ' + stdio);
            ^
TypeError: Incorrect value for stdio stream: [object Object]
    at child_process.js:768:13
    at Array.reduce (native)
    at ChildProcess.spawn (child_process.js:723:17)
    at Object.exports.spawn (child_process.js:618:9)
    at Object.<anonymous> (/Users/jclulow/dap.js:4:7)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:492:10)

This is on node v0.8.9 on OS X, and I've also seen it on 0.8.4 on OS X and SmartOS.

I imagine this doesn't work because the file descriptor for the underlying stream hasn't been allocated yet. But the docs for child_process.spawn say this about the "stdio" argument to "spawn":

Stream object - Share a readable or writable stream that refers to a tty, file, socket, or a
pipe with the child process. The stream's underlying file descriptor is duplicated in the
child process to the fd that corresponds to the index in the stdio array.

It doesn't say anything about the stream having to be open already. That's probably not an unreasonable constraint, but the docs should probably mention this.

If you tweak your code:

var child = require('child_process');
var fs = require('fs');
var out = fs.createWriteStream('foo.txt');
out.on('open', function(){
  child.spawn('ls', [], {
    'stdio': [ null, out ]
  });
});

But it's strange that child_process.spawn does not recognize invalid (not opened) stream object. (out.fd === null at creation time)

Member

bnoordhuis commented Sep 19, 2012

I imagine this doesn't work because the file descriptor for the underlying stream hasn't been allocated yet.

That's indeed what happens.

It would be nice to make it work but there is no file descriptor until the next tick of the event loop (or whenever the open() syscall finishes). This needs some thought.

@bnoordhuis Do you have any idea? #4032 does not fix anything, only gives more friendly error reporting

Member

bnoordhuis commented Sep 28, 2012

@langpavel I haven't really fleshed it out yet but it would probably look something like this:

function spawn() {
  if (--spawn.nevents !== 0) return;
  // continue spawning the child process
}
spawn.nevents = 1;

for (var i = 0; i < stdio.length; ++i) {
  var s = stdio[i];
  if (!(s instanceof fs.ReadStream || s instanceof fs.WriteStream)) continue;
  if (s.fd !== null) continue;
  s.once('open', spawn);
  ++spawn.nevents;
}

spawn();

Special-case code like that is nasty, though, and there remain some edge cases to be worked out (e.g. how to deal with a stream that's destroyed before the 'open' event is emitted?).

I've discovered a nasty consequence of this: if you're doing this with two file descriptors (e.g., stdin and stderr), you have to wait for both of them to open, but if the input stream opens first, it can start emitting data before you're ready to spawn, and that data will not be read by the child process. Based on a quick look at the code, I think it would be safe to pause the input stream in the 'open' handler as long as it's a file input stream, but as documented I don't think that's guaranteed to work. Isaac suggested using an fd for this case instead of a stream, which will be my next approach.

Owner

jasnell commented May 18, 2015

@davepacheco ... a quick test shows that the reported behavior is still in v0.12. Is this still an issue for you? Do we need to keep this open?

If the behavior and documentation have not changed, it sounds like this is still an open issue.

Any chance of this being fixed? I think there's also a race if you try to register handlers for the I/O streams after invoking spawn, so basically anyone trying to run a short-lived process and capture output is SOL.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.