Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix case where pre-domain nextTick executing post-domain callbacks resulted in domains being ignored #152

Closed
wants to merge 1 commit into from

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Mar 18, 2014

@benjamingr
Copy link
Collaborator

Nice trick! an additional check should be added for those who are using a browser, other than that - looks promising :)

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

oh yes, of course, silly me!

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

Hmm, do you all prefer / is there a reason for, the whole typeof n !== 'undefined' && n !== null thing? I can repush with that variation, though it's getting a tad wordy.

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

Ah, line too long for jshint

@petkaantonov
Copy link
Owner

The checks should be rearranged to optimize for the case where domains are not in use:

Async.prototype.invokeLater = function Async$invokeLater(fn, receiver, arg) {
    ASSERT(typeof fn === "function");
    ASSERT(arguments.length === 3);
    if (process !== void 0 &&
        process.domain != null &&
        !fn.domain) {
        fn = process.domain.bind(fn);
    }
    this._lateBuffer.push(fn, receiver, arg);
    this._queueTick();
};

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

I've refactored it for that and for more complete duck-typing.

@petkaantonov
Copy link
Owner

I cannot get this test to pass:

"use strict";

var assert = require("assert");

var Promise = require("../../js/debug/bluebird.js");
var isNodeJS = typeof process !== "undefined" &&
    typeof process.execPath === "string";

if (isNodeJS) {
    describe("domain", function(){
        specify("gh-148", function(done) {
            Promise.onPossiblyUnhandledRejection(function(error,promise) {
                throw error
            });
            var called = false;
            var e = new Error();
            Promise.resolve(23).then(function(){called = true});
            require('domain').create()
              .on('error', function(E) {
                assert.equals(e, E);
                assert(called);
                done();
              })
              .run(function() {
                  var P = new Promise(function(resolve,reject){ reject(e) });
              });

        });
    });
}

To run the test place it in a file like /test/mocha/domain.js and then do grunt test --run=domain --verbose

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

Hmm, interesting… seems to be some interaction with mocha, as the following prints two "oks":

"use strict";
var Promise = require('./js/debug/bluebird.js');
Promise.onPossiblyUnhandledRejection(function(error,promise) {            
    throw error            
});            
var called = false;            
var e = new Error();            
Promise.resolve(23).then(function(){called = true});            
require('domain').create()            
  .on('error', function(E) {            
      console.log( e == E ? "ok" : "not ok", "error objects match"  );            
      console.log( called ? "ok" : "not ok", "other promise chained out" );            
  })            
  .run(function() {            
      var P = new Promise(function(resolve,reject){ reject(e) });
  });

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

I'll dig further in a little bit

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2014

Ok, because the wrapper code that's rethrowing the error isn't in a domain, we need to do domain stuff ourselves instead of rethrowing if the function was bound to a domain.

Also, change assert.equals in your example to assert.equal

@petkaantonov
Copy link
Owner

Commited in 134c396 Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error domain's don't always capture exceptions thrown from onPossiblyUnhandledRejection
3 participants