Break on uncaught exceptions does not work with 'error' events in domains

[bajtos]

Consider the following code (uncomment one of the lines (1) or (2)):

var domain = require('domain');
var http = require('http');
var EventEmitter = require('events').EventEmitter;

var d = domain.create();
d.on('error', function(err) {
  console.log('unhandled error', err);
  process.exit(1);
});

d.run(function() {
  http.createServer(function(req, res) {
      // throw new Error('uncaught throw'); // (1)
      // new EventEmitter().emit('error', new Error('uncaught event')); // (2)
      }).listen(3000);
});

When running in a debugger, the first statement (1) breaks on uncaught exception but the second (2) does not.

When the server is not running inside a domain, both (1) and (2) breaks on uncaught exception in V8 debugger.

See PR #5713 for the implementation of "break on uncaught exception" (v0.11 only).

[jasnell]

@bajtos ... I know it's been a while, but is this still an issue for you?

[bajtos]

@jasnell yes, it's still an issue, I can reproduce the problem on io.js v2.3.1.

[misterdjules]

@bajtos I would think this is expected behavior: (2) doesn't throw an exception directly, it emits an 'error' event. It only throws an exception if there's no listener for the 'error' event.

When the createServer callback runs in a domain, the event emitter's errors are forwarded to that domain's 'error' event handler without throwing an exception.

So my question would be: why is this an issue?

[bajtos]

@misterdjules In my opinion, the code snippets (1) and (2) should behave the same way in debugger regardless of whether they are wrapped in a domain or not. To prevent confusion, here is the snippet for "not wrapped in domain":

var http = require('http');
var EventEmitter = require('events').EventEmitter;

http.createServer(function(req, res) {
  // throw new Error('uncaught throw'); // (1)
  // new EventEmitter().emit('error', new Error('uncaught event')); // (2)
}).listen(3000);

Both (1) and (2) eventually throw an error, because there is no error event handler installed on the EventEmitter instance. When the request handler is running inside a domain, this error is caught by the domain-level error handler.

We can debate whether a domain-level error handler makes the exception conceptually handled. I personally prefer to treat such exception as unhandled, as it makes "break on unhandled exception" little more useful. But I don't care too much which option we pick.

If we agree that exceptions caught by domain-level error handler are considered handled, then we should fix the implementation so that (1) inside a domain does not trigger "break on uncaught exception", because the exception was handled.

If we agree that exceptions caught by domain-level error handler are considered unhandled, then we should fix the implementation so that (2) inside a domain triggers "break on uncaught exception", because the error/exception was not handled.

OTOH, considering that I am the only person that noticed this issue so far, I think it may be best to simply close this issue for now and wait until there are more people noticing and asking for a fix.

[misterdjules]

I would like to summarize the current status of this discussion because I'm not sure I understand properly what you consider being an issue after reading your latest comment.

Given the following code:

var http = require('http');
var EventEmitter = require('events').EventEmitter;

http.createServer(function(req, res) {
  // throw new Error('uncaught throw'); // (1)
  // new EventEmitter().emit('error', new Error('uncaught event')); // (2)
}).listen(3000);

both (1) and (2) trigger a "break on uncaught exception" even in a debugger if one is connected. That seems natural, since (1) is an uncaught exception, and (2) results in a call to throw since there's no 'error' event handler on that EventEmitter instance and there's no domain with an error handler in the picture. So in this case (1) and (2) are equivalent in the sense that they both translate to throwing an error that is not caught, and since their behavior is actually identical for the debugger (it catches an uncaught exception), then I don't see any inconsistency.

Now regarding the original code sample:

var domain = require('domain');
var http = require('http');
var EventEmitter = require('events').EventEmitter;

var d = domain.create();
d.on('error', function(err) {
  console.log('unhandled error', err);
  process.exit(1);
});

d.run(function() {
  http.createServer(function(req, res) {
      // throw new Error('uncaught throw'); // (1)
      // new EventEmitter().emit('error', new Error('uncaught event')); // (2)
      }).listen(3000);
});

The event emitter in (2) is bound to the domain, which has an error handler, and thus (2) doesn't result in throwing any error. So it seems expected that the debugger doesn't intercept any exception in that case, which is why I don't see any inconsistency in that case either.

Now, the fact that an uncaught error handled by a domain's error handler doesn't turn it into a caught exception, or at least it doesn't make it caught by a try/catch block setup by the user. It's caught by the external exception handler (the TryCatch instance in node::MakeCallback and node::AsyncWrap::MakeCallback), but does that mean that the user caught it? The stack is still unwound until the frame where the external exception handler is instantiated, which is different from having the user setup a try/catch block wrapping the code that throws the error. So triggering a "break on uncaught exception" event still seems to be consistent.

However, I can see some inconsistency with how --abort-on-uncaught-exception behaves: if an error is thrown and handled by a domain's error handler (but not by a try/catch block setup by the user), node will not abort, although in the same situation a "break on uncaught exception" event is triggered and handled by the debugger.

So I would think that:

1. No emit('error') should be treated as an uncaught exception, unless there's no error handler on the event emitter and there's no domain with an error handler attached to this event emitter.
2. All throw statements should be treated as uncaught exceptions, even if a domain with an error handler is active at that time.

This would mean that the only change needed to improve consistency would be making node abort on an uncaught exception when using --abort-on-uncaught-exception even when a domain with an error handler is active.

I think it would make sense, since the goal of --abort-on-uncaught-exception is to record as much of the state of the process as possible at the very moment an uncaught error is thrown, and at the time a domain error handler is called, that context is already lost.