Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

crypto module does not bind callbacks to the active domain #3965

Closed
amccollum opened this Issue Sep 5, 2012 · 3 comments

Comments

Projects
None yet
3 participants

The crypto module doesn't bind its callbacks to the active domain:

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

d = domain.create();

d.on('error', function (err) {
  console.log('An error: ', err);
});

d.run(function () {
  crypto.randomBytes(16, function () { throw new Error('this is an error'); });
});

Produces the following output (in Node v0.8.9-pre):


domain.js:66
    throw er;
          ^
Error: this is an error
    at Object.ondone (/Users/andrew/Sites/mingl-soma/build/test.js:11:46)

The behavior should match other builtin modules, such as fs, which do domain binding automatically.

@ghost ghost assigned bnoordhuis Sep 5, 2012

langpavel added a commit to langpavel/node that referenced this issue Sep 5, 2012

I've worked around this by writing a wrapper that captures the domain on invoke and wraps the callback with domain.enter(); cb(...); domain.exit() but it would be nice if crypto played nice with domains out of the box.

I'm willing to work on a patch if you have an idea of how it should be done. A wrapper function in crypto.js would work, but it's not very pretty.

bnoordhuis added a commit to bnoordhuis/node that referenced this issue Aug 27, 2013

crypto: make randomBytes/pbkdf2 cbs domain aware
Make the crypto.randomBytes() and crypto.pbkdf2() callback functions
run inside the current domain (if any.)

Fixes #3965.
Member

bnoordhuis commented Aug 27, 2013

See #6135 - I already had a patch for this, just needed to dust it off. It makes crypto.randomBytes() and crypto.pbkdf2() domains aware. Are there any other crypto functions that I'm forgetting?

The patch is targeted at master. I don't really want to back-port it to v0.10 because the delta is pretty big by now unless someone can give me a compelling reason (e.g. "this breaks my code".)

Thanks @bnoordhuis, I can continue with my workaround until this makes it to a stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment