Duplicate EPIPE Errors

[skeggse]

I'm spawning a process using child_process. I assume that at some point the child process will fail for whatever reason, so I need to be listening for all applicable error events, to avoid the entire process shutting down thanks to the EventEmitter throwing the uncaught error event.

For some odd reason, I get two EPIPE errors with the following code.

var child_process = require('child_process');

var child = child_process.spawn('/something/that/doesnt/exist', []);

child.on('error', console.log.bind(console, 'child'));
child.stdin.on('error', console.log.bind(console, 'child.stdin'));
child.stdout.on('error', console.log.bind(console, 'child.stdout'));
child.stderr.on('error', console.log.bind(console, 'child.stderr'));

child.stdin.write('oh no');

That'd be fine, except that I want to remove the old child and spawn a new one. To avoid handling events for an ostensibly dead process, I removeAllListeners for the child and its stdio. Then, when the second EPIPE error hits, the application fails. I could, of course, removeAllListeners, then add noop listeners for the error events, but that's more of a workaround.

Is this expected behavior, and if so, why?

[isaacs]

Nice find! If you change the console.log to console.trace, the reason becomes clear:

Trace: child.stdin { [Error: write EPIPE] code: 'EPIPE', errno: 'EPIPE', syscall: 'write' }
    at Socket.EventEmitter.emit (events.js:95:17)
    at onwriteError (_stream_writable.js:231:10)
    at onwrite (_stream_writable.js:249:5)
    at WritableState.onwrite (_stream_writable.js:97:5)
    at fireErrorCallbacks (net.js:438:13)
    at Socket._destroy (net.js:472:3)
    at Object.afterWrite (net.js:718:10)
Trace: child.stdin { [Error: write EPIPE] code: 'EPIPE', errno: 'EPIPE', syscall: 'write' }
    at Socket.EventEmitter.emit (events.js:95:17)
    at net.js:441:14
    at process._tickCallback (node.js:415:13)

The first error is from the stream.Writable class, and then it re-emits on the net.Socket object.

I think this is a bug.

[skeggse]

Cool! Which EPIPE error should occur?

[isaacs]

@skeggse I think the Stream one is better, since it happens first. It'd be important to make sure we're not losing any error events by removing the emit from net.js, but I'm pretty sure that every write error will end up getting raised in _stream_writable.js.

Patch (with a test!) welcome.

[kadishmal]

I was bugged by this very issue for couple hours now just to find out it's been reporter. Have the same escalation of the error.

Trace: [Error: This socket is closed.]
at EventEmitter.emit (events.js:95:17)
at onwriteError (_stream_writable.js:231:10)
at onwrite (_stream_writable.js:249:5)
at WritableState.onwrite (_stream_writable.js:97:5)

Trace: [Error: This socket is closed.]
at EventEmitter.emit (events.js:95:17)
at net.js:441:14
at process._tickCallback (node.js:415:13)

@isaacs, what's the best way to handle such scenario considering that I should catch only one of the errors? I would prefer to be able to catch onwriteError on the stream level.

[roamm]

Also bit by this today. Spent couple of hours digging in the node source code and found that 'error' event is emitted both in net.js and _stream_writable.js so I guess EPIPE is just one case for duplicate 'error' event. @isaacs Also found that _destroy() is not completely reentrance-safe, causing server connection count wrong. See comments below.

Socket.prototype._destroy = function(exception, cb) {
  debug('destroy');

  var self = this;

  function fireErrorCallbacks() {
    // cb() can call into onwrite() in _stream_writable.js emitting an error,
    // which can be handled in socketErrorListener() in http.js and calls destroy() again
    if (cb) cb(exception);
    if (exception && !self.errorEmitted) {
      process.nextTick(function() {
        self.emit('error', exception);
      });
      self.errorEmitted = true;
    }
  };

  if (this.destroyed) { // when the above case happens, this is not set yet
    debug('already destroyed, fire error callbacks');
    fireErrorCallbacks();
    return;
  }

  self._connecting = false;

  this.readable = this.writable = false;

  timers.unenroll(this);

  debug('close');
  if (this._handle) {
    if (this !== process.stderr)
      debug('close handle');
    var isException = exception ? true : false;
    this._handle.close(function() {
      debug('emit close');
      self.emit('close', isException);
    });
    this._handle.onread = noop;
    this._handle = null;
  }

  fireErrorCallbacks();
  this.destroyed = true; // it's set after fireErrorCallbacks();

  if (this.server) {
    COUNTER_NET_SERVER_CONNECTION_CLOSE(this);
    debug('has server');
    this.server._connections--;
    if (this.server._emitCloseIfDrained) {
      this.server._emitCloseIfDrained();
    }
  }
};

[skeggse]

@roamm I'm assuming this issue is now closed because #6968 was merged. Correct me if I'm wrong.