Domains inhibit uncaughtException error handling

[piscisaureus]

In the test case below, I would expect the uncaughtException handler to run (since the domain doesn't have an error event handler), but it doesn't.

Verified with node v0.10.39 and v4.2.1.

var domain = require('domain');

var d = domain.create();
d.enter();

process.on('uncaughtException', function(e) {
  console.log('Uncaught exception: ', e);
});

setTimeout(function() {
  throw new Error('Bla!');
}, 1);

[evanlucas]

It is actually caught with v0.12

[evanlucas]

This worked for me in v4.0.0 as well. Looks like https://github.com/nodejs/node/pull/3036/files re-introduced the regression. /cc @misterdjules

[misterdjules]

@evanlucas The 'uncaughtException' event is emitted with v0.12.7, but not with a build of the tip of node's v0.12 branch.

It seems that the problem has been there for a long time on v0.10, and is fixed by the following patch:

diff --git a/src/node.js b/src/node.js
index ab56040..5aec260 100644
--- a/src/node.js
+++ b/src/node.js
@@ -229,7 +229,7 @@
     // and exit if there are no listeners.
     process._fatalException = function(er) {
       var caught = false;
-      if (process.domain) {
+      if (process.domain && process.domain.listeners('error').length > 0) {
         var domain = process.domain;
         var domainModule = NativeModule.require('domain');
         var domainStack = domainModule._stack;

On v0.12 and v4.x, this is a regression that was introduced by me with nodejs/node-v0.x-archive#25835 and #3036 respectively.

Here's a patch for v0.12 and v4.x that fixes the issue:

diff --git a/lib/domain.js b/lib/domain.js
index 87a4707..b5fc392 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -80,7 +80,13 @@ Domain.prototype._errorHandler = function errorHandler(er) {
   var self = this;

   function emitError() {
-    var handled = self.emit('error', er);
+    var handled = false;
+
+    if (self.listeners('error').length === 0) {
+      return false;
+    }
+
+    handled = self.emit('error', er);

     // Exit all domains on the stack.  Uncaught exceptions end the
     // current tick and no domains should be left on the stack

These patches were put together very quickly, and somehow feel a bit hacky. I haven't had the time to investigate their impact on other use cases.

Also, while the issue with v0.10 hasn't been introduced by my change to support --abort-on-uncaught-exception when domains are used, the same changes are responsible for this regression on v0.12 and v4.x.

So it's probably worth it to take a bit more time to see if these changes could be implemented in a different way that would not introduce this regression.

I'll try to find more time this week-end to come up with the best fixes possible and I'll write something up here.

Thanks for reporting the issue!

[evanlucas]

Thanks for the in-depth explanation @misterdjules. From playing around with it, it looks like process._fatalException currently is being called twice. Not sure if that helps or not, but couldn't really make any other headway right now.

[misterdjules]

@nodejs/tsc @nodejs/collaborators I've added the following labels: v0.10, v0.12, v4.x, v5.x and master to identify issues that affect these versions. I find it's handy to determine what versions are affected by a given problem without having to read through the whole thread again.

In this case, all these labels where set on this issue because after careful examination, it affects all these versions.

[misterdjules]

From playing around with it, it looks like process._fatalException currently is being called twice

@evanlucas Indeed, the reason is that we enter a domain twice: once with the explicit call to d.enter() and a second time when calling the timer's callback.

Then, when handling the error thrown in the domain at the top of the stack, we call EventEmitter.prototype.emit('error') on an instance that doesn't have an 'error' event handler. So it throws an error, thus calling process._fatalException again.

For v0.10 though the problem is slightly different: we we either have domains on the stack and handle errors through these domains, or emit 'uncaughtException'.

Anyway, we need to think a bit more about this to come up with a fix that hopefully won't cause more regressions, and the first step is probably to add more tests, beginning with the code in the description of this issue.

[priyavivek1]

All, Do we have any ETA for this as we are heading towards production soon..

[misterdjules]

@priyavivek1 I should be able to come up with a good candidate for a fix early next week. Then it'll have to go through the usual review/update cycle, and then it'll have to make it into a release. In other words, it's a bit early to give an ETA, but I'll update this issue early next week with next steps, unless someone else takes care of it before me.

[priyavivek1]

@misterdjules : We are on node version 0.10.39 and would be requiring a fix on that top of that. Thanks

[misterdjules]

@piscisaureus @priyavivek1 See #3637.

[misterdjules]

Also created #3638 for node v0.12.

[misterdjules]

And finally submitted a PR that fixes the same issue on nodejs/node's master branch: #3640.

[misterdjules]

Fixed in v4.x-staging with 25cded4, v0.12-staging with 16417cc, v0.10-staging with 9cae9b2 and master with 425a354.

[rvagg]

great work @misterdjules! thanks for pushing so hard on this

[priyavivek1]

@misterdjules : Which version of 0.10.x is this fixed in..

[misterdjules]

@priyavivek1 v0.10.43.