Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Open
davepacheco opened this Issue · 5 comments

3 participants

@davepacheco
Owner

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.

@langpavel

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)

@bnoordhuis

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.

@langpavel

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

@bnoordhuis

@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?).

@davepacheco
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.