Domain fails to catch when error thrown inside crypto.pbkdf2

[hueniverse]

var crypto = require('crypto');
var domain = require('domain');

var d = domain.create();
d.on('error', function () { console.log('done'); });
d.run(function () {
    crypto.pbkdf2('a', 'b', 1, 8, function () {
        throw new Error('x');
    });
});

[rustyconover]

Hi,

Try:

var crypto = require('crypto');
var domain = require('domain');

var d = domain.create();
d.on('error', function () { console.log('done'); });
d.run(function () {
    crypto.pbkdf2('a', 'b', 1, 8, d.intercept(function () {
        throw new Error('x');
    }));
});

Since the crypto object isn't an EventEmitter it will not be implicitly bound to the domain, but you can accomplish the same thing by using domain.intercept for the callback function. See:

http://nodejs.org/api/domain.html#domain_domain_intercept_callback

Rusty

[hueniverse]

@rustyconover thanks, but you are just wrapping the callback in another domain. This should work as originally written.

[rustyconover]

@hueniverse Actually no, I don't think so reading about domains. From the first paragraph of docs:

Domains provide a way to handle multiple different IO operations as a single group. If any of the event emitters or >>callbacks registered to a domain emit an error event, or throw an error, then the domain object will be notified, >>rather than losing the context of the error in the process.on('uncaughtException') handler, or causing the program >>to exit immediately with an error code.

Domain's simply aren't a replacement for try {} catch() {}, they won't catch every exception. They only handle errors from EventEmitters or some special functions in require('fs'). If you want them to handle other exceptions you need to register your callbacks.

In my example, there is only one domain and I simply registered your callback with it with the d.intercept() call.

[othiym23]

This is a valid bug, and probably a consequence of how OpenSSL is bound into Node.

@rustyconover, if domains can't be treated as an async equivalent of try/catch, they're not very useful. Any ordinary async callback executed by another function run within a domain should also execute within that domain. EventEmitters created inside the domain are implicitly bound to the domain, but regular functions should also be bound to the domain by MakeDomainCallback in the C++ side of Node.

[rustyconover]

@othiym23 I agree now that is is a bug, reading more of the code. The domain isn't being passed when the callback is being called. I'm confused if it is getting dropped because the this object isn't correctly containing a domain key, in MakeDomainCallback in crypto.js for pbkdf2. Or in EIO_PBKDF2After in node_crypto.cc.

[isaacs]

@hueniverse @othiym23 @rustyconover Yes, this is definitely a bug. The crypto lib should be capturing the active domain, and then passing it along in the MakeCallback return.

[hueniverse]

As an aside, I've got more of these but will take a while to isolate them. Basically, we're using domains to capture all failed assertions in our test tool. Sometimes it blows up on a failed test due to domains not capturing the error.

[rustyconover]

I've got a start on a patch by adding the current domain to:
Handle PBKDF2(const Arguments& args) then adding it to the req.

I'll post it shortly.

[rustyconover]

@hueniverse If there are more of these and @isaacs likes my patch, I'd be happy to take a look at them.

[hueniverse]

@rustyconover nice! probably worth looking at the other crypto methods while in there...

[hueniverse]

Can we get this resolved?

[trevnorris]

FWIW this would be causing AsyncListeners. Maybe there's a way to
supplement domain coverage with AL to catch these.

[hueniverse]

This is really sad. I just wasted 2 hours debugging an issue caused by this bug. It's been open for so long, I forgot I opened it. Can we please get this fixed and released in 0.10.32 ASAP? This is no longer some exotic test script that's failing. This is now crashing our production servers!

[tjfontaine]

Fixed in 6e689ec