child_process.exec TypeError: Cannot read property 'addListener' of undefined

[gcochard]

Here's a code sample that triggers the error condition:

var child_process = require('child_process'),
    exec = child_process.exec,
    children = [],
    exitHandler = function(i,err,stdout,stderr){
        console.log(i,err,stdout,stderr);
    },
    i = 0;
for(i=0;i<1000;i++){
        children.push(exec('sleep 10 && echo hello',exitHandler.bind(null,i)));
}

If I do not wrap with try/catch it will crash after ~258 iterations on io.js v1.6.3 on OSX 10.10.2

Here is the stack trace:

child_process.js:753
  child.stdout.addListener('data', function(chunk) {
              ^
TypeError: Cannot read property 'addListener' of undefined
    at Object.exports.execFile (child_process.js:753:15)
    at exports.exec (child_process.js:622:18)
    at Object.<anonymous> (/Users/gcochard/iojs/test.js:10:19)
    at Module._compile (module.js:410:26)
    at Object.Module._extensions..js (module.js:428:10)
    at Module.load (module.js:335:32)
    at Function.Module._load (module.js:290:12)
    at Function.Module.runMain (module.js:451:10)
    at startup (node.js:123:18)
    at node.js:867:3

It seems that if a callback is passed, the general async-ism is to call it with any errors. Therefore the following:

  child.stdout.addListener('data', function(chunk) {...});

Should change to something like this:

if(!child.stdout || !child.stderr){
  return errorhandler(new Error('Something went wrong'));
}
child.stdout.addListener('data', function(chunk) {...});

And the error handler should also include a null check on child.stdout and child.stderr.

---

Additionally, when I do wrap with try/catch, it looks like it is emitting a spawn error on line 1022 which is not handled, even when adding an error listener on the child. It appears to be emitted by the ChildProcess object on line 1042. Its stack trace is as follows:

events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: spawn /bin/sh EAGAIN
    at exports._errnoException (util.js:749:11)
    at Process.ChildProcess._handle.onexit (child_process.js:1022:32)
    at child_process.js:1114:20
    at process._tickCallback (node.js:339:13)
    at Function.Module.runMain (module.js:453:11)
    at startup (node.js:123:18)
    at node.js:867:3

I know this is a contrived example, but I've hit this edge case before.

Please let me know if I can clarify anything here. The expected behavior is that the script keeps running, rather than crashing io.js with a stack trace.

[bnoordhuis]

Confirmed. The test case hits the max process limit (ulimit -u). The problem is that the return value of this child.spawn() call isn't checked.

[Fishrock123]

note: if you run this in the REPL both errors fire:

TypeError: Cannot read property 'addListener' of undefined
    at Object.exports.execFile (child_process.js:753:15)
    at exports.exec (child_process.js:622:18)
    at repl:2:57
    at REPLServer.defaultEval (repl.js:124:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:277:12)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:166:7)
    at REPLServer.Interface._onLine (readline.js:195:10)
Error: spawn /bin/sh EAGAIN
    at exports._errnoException (util.js:749:11)
    at Process.ChildProcess._handle.onexit (child_process.js:1024:32)
    at child_process.js:1116:20
    at process._tickDomainCallback (node.js:367:13)

[Fishrock123]

@bnoordhuis would this patch be acceptable for now? It's going to error anyways.

diff --git a/lib/child_process.js b/lib/child_process.js
index a38de28..ae0912a 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -961,7 +961,7 @@ var spawn = exports.spawn = function(/*file, args, options*/) {

   debug('spawn', opts.args, options);

-  child.spawn({
+  const err = child.spawn({
     file: opts.file,
     args: opts.args,
     cwd: options.cwd,
@@ -973,6 +973,8 @@ var spawn = exports.spawn = function(/*file, args, options*/) {
     gid: options.gid
   });

+  if (err < 0) throw errnoException(err, 'spawn');
+
   return child;
 };

Discussed this a bit on irc with @cjihrig and it seems like it's generally a wonky case.

Output with that patch (Still throws the latter error also if you are in the repl because existing reasons.)

Error: spawn EAGAIN
    at exports._errnoException (util.js:749:11)
    at exports.spawn (child_process.js:976:22)
    at Object.exports.execFile (child_process.js:654:15)
    at exports.exec (child_process.js:622:18)
    at repl:2:57
    at REPLServer.defaultEval (repl.js:124:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:277:12)
    at emitOne (events.js:77:13)

[cjihrig]

This appears to be roughly the same as nodejs/node-v0.x-archive#8504. I started looking at this a while back and opened nodejs/node-v0.x-archive#8779. It doesn't solve the problem, but prevents the accesses of the streams that are causing the crashes. The file descriptors aren't leaked in the strictest sense of the word - it just takes some time for them to be freed (sockets must close, etc.). So, running this in a tight loop will always cause this problem, unless some changes are made around the code block that @Fishrock123 linked to.

EDIT: The code to link to is https://github.com/iojs/io.js/blob/f3e9da3e69393acb6f2cb90c4dc53ae5908ac9ef/lib/child_process.js#L1106-L1157

[bnoordhuis]

@Fishrock123 I think it should be an 'error' event. It's a run-time error.

[cjihrig]

I agree it should be an event. Assuming it is emitted in the next tick, won't you still potentially run into the case where stdout and stderr are undefined?

EDIT: After testing, I did still hit that case. Thoughts on combining these changes with nodejs/node-v0.x-archive#8779?

EDIT2: error events aren't emitted on next tick. Perhaps add the ability to pass in an error handler to spawn()?

[Fishrock123]

Unless you are checking for either an error or that the object is actually proper right there, you'll still end up with an error since stdout is undefined.

[yinhongyan]

Was this issue fixed or is there a workaround? I hit it today.

[Fiery]

I encountered the same issue, I tried to solve it with raising the maxproc limit on Mac and it did solve the problem in my case. However considering the maxproc is hard coded to 2500 on regular Mac OSX, this issue would show up when you have more than 2500 processes to run at the same time.

[evanlucas]

Closing as this seems to have been fixed by #3799