Clear domain stack after uncaught exception

[dohse]

After the patch caeb677 the uncaught exception handler does not clear
the domain stack after an exception in the top level domain has been
handled. This leaves unexpected domains on the domain stack. Fix this by
clearing the domain stack also in this case.

The affects node version starting with v0.10.34, but not v0.11.

This script demonstrates the problem:

var domain = require('domain');

var d = domain.create();

d.on('error', function() {
    process.nextTick(function() {
      console.log('domain._stack.length = %d', domain._stack.length);
    }, 100);
});

d.run(function() {
  throw new Error('Inside');
});

It yields these results:

> nvm use 0.10.33
Now using node v0.10.33
> node domain-stack-length
domain._stack.length = 1
> nvm use 0.10.34
Now using node v0.10.34
> node domain-stack-length
domain._stack.length = 2
> nvm use 0.10.35
Now using node v0.10.35
> node domain-stack-length
domain._stack.length = 2
> node-with-this-fix domain-stack-length
domain._stack.length = 1
> nvm use 0.11.15
Now using node v0.11.15
> node domain-stack-length
domain._stack.length = 1

[trevnorris]

LGTM

/cc @tjfontaine @misterdjules @cjihrig

[cjihrig]

Can this be broken into two assignments on two separate lines?

[dohse]

Certainly. This is an exact copy of https://github.com/dohse/node/blob/clear-domain-stack/src/node.js#L279-L284 - it might be better to unify these two sites.

[misterdjules]

@dohse Thank you very much for catching that, and my apologies for introducing this regression!

This look goods to me, with a couple of suggestions to be able to land this.

We would need to add at least one test to this PR, and the code that you used to reproduce the issue and test your fix seems like a good candidate for that. We might be able to come up with more tests, but that's already a good start.

I would also like to avoid the duplication of

var domainModule = NativeModule.require('domain');
domainStack.length = 0;
domainModule.active = process.domain = null;

if possible.

I took a shot at upgrading your PR with the two suggestions above. There's a version that only adds your test and factor out the domains stack cleaning in a separate function.

I also uploaded another version that turns the two domains stack cleaning function calls into just one.

@dohse @joyent/node-coreteam Thoughts?

[misterdjules]

@dohse @joyent/node-coreteam Created #9364 to illustrate my comments.

[misterdjules]

Thank you very much @dohse, closing in favor of #9364 but commits in this PR will still have you as author.