NodeJS: http.ClientRequest fires error event twice in specific scenario

Question

Consider following javascript code executed in nodejs:

// create ClientRequest
// port 55555 is not opened
var req = require('http').request('http://localhost:55555', function() {
  console.log('should be never reached');
});

function cb() {
  throw new Error();
}

req.on('error', function(e) {
 console.log(e);
 cb();
});

// exceptions handler
process.on('uncaughtException', function() {
 console.log('exception caught. doing some async clean-up before exit...');
 setTimeout(function() {
   console.log('exiting');
   process.exit(1);
 }, 2000);
});

// send request
req.end();

Expected output:

{ Error: connect ECONNREFUSED 127.0.0.1:55555
    at Object.exports._errnoException (util.js:1026:11)
    at exports._exceptionWithHostPort (util.js:1049:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1081:14)
  code: 'ECONNREFUSED',
  errno: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 55555 }
exception caught. doing some async clean-up before exit...
exiting

Actual output:

{ Error: connect ECONNREFUSED 127.0.0.1:55555
    at Object.exports._errnoException (util.js:1026:11)
    at exports._exceptionWithHostPort (util.js:1049:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1081:14)
  code: 'ECONNREFUSED',
  errno: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 55555 }
exception caught. doing some async clean-up before exit...
{ Error: socket hang up
    at createHangUpError (_http_client.js:252:15)
    at Socket.socketCloseListener (_http_client.js:284:23)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at TCP._handle.close [as _onclose] (net.js:492:12) code: 'ECONNRESET' }
exception caught. doing some async clean-up before exit...
exiting

As you can see, http.ClientRequest (or perhaps stream.Writable?) fires error event twice, first with ECONNREFUSED and after exception is caught, ECONNRESET.

This does not happen if we execute callback asynchronously in http.ClientRequest error handler using nextTick or setTimeout, e.g. this change gives expected behavior:

req.on('error', function(e) {
 console.log(e);
 process.nextTick(cb);
});

Can anyone explain why this is happening and if this is a bug or works as expected? Behavior is same in latest node 4.x and node 6.x.

Thanks!

Answer 1

Synopsis

The trouble is that you are throwing an uncaught error inside the listener callback. Node intentionally does not handle unexpected Exceptions so the consequence is that the code that normally runs after this error has been skipped as control must go to the appropriate catch(, in this case all the way to your uncaught Exception handling.) This causes a few things not to happen, but the most noticeable is that HTTPRequest isn't aware that it successfully emitted an error when it gets called back for the socket closing.

Details and References

Node's general philosophy is not to trap unexpected throws and node treats this pattern as a programmer error that should be allowed to reach a failure. (The documentation is not explicit about the API for an event listener callback, but also provides no example where one throws an exception or instructions to consider the possibility when handling the emitter on the developer side of the stream API.)

When your exception propagates up to the emitter, and subsequent listeners and the rest of its cleanup and marking of the socket does not occur, causing ClientRequest to think it needs to provide an error again:

The emitter that your callback throws is followed by code to suppress the 2nd error:

req.emit('error', err);
// For Safety. Some additional errors might fire later on
// and we need to make sure we don't double-fire the error event.
req.socket._hadError = true;

Since your throw wasn't caught there, the Check for this variable then finds _hadError is still unset:

if (!req.res && !req.socket._hadError) {
  // If we don't have a response then we know that the socket
  // ended prematurely and we need to emit an error on the request.
  req.emit('error', createHangUpError());
  req.socket._hadError = true;
}

If you push your error throwing into another asynchronous block then you are not preventing the rest of the socket cleanup process to continue as the Exception will occur in some other function stack.

In some other cases node is careful about when it calls callbacks and what it sets beforehand. But this is largely done to allow callbacks to do some alternate cleanup, etc, as seen in this comment:

we set destroyed to true before firing error callbacks in order
to make it re-entrance safe in case Socket.prototype.destroy()
is called within callbacks

Swapping the order of setting _hadError and emitting would suppress this second error for you and seems safe given that this seems to be the only _hadError check. But:

• if this is used to suppresses more errors in the future then that would negatively impact error callbacks that try to probe the state of the connection during an error

• it would still be leaving the socket in a partially cleaned up state which is not nice for longer living programs.

So I'd generally say it is better not to directly throw exceptions in callbacks unless you have an unusual situation where the normal internal handling has to be prevented or overridden.