Support Node.js domains

[domenic]

Some helpful comments from @mikeal on a probable strategy for supporting domains. This would help, from what I understand, in two situations:

1. Putting exceptions thrown by .done() into the current domain.
2. When an event emitter or other domain-using callback library is used inside a fulfillment or rejection handler, and throws an error, it would escape Q but wouldn't escape the domain. E.g.

return Q.resolve().then(function () {
    doSomethingAsyncWithoutPromises(function (err, whatever) {
        // Q doesn't catch this, but domains would if we wired it up right.
        callANonexistantFunction();
    });
});

[domenic]

This already works without us doing anything, I think because process.nextTick is used. @mikeal or @isaacs feel free to confirm/deny.

This test passes:

describe("node domain support", function () {
    it("should work or something", function (done) {
        var error = new Error("should be caught by the domain's handler");
        var domain = require("domain").create();

        domain.run(function () {
            Q.resolve().done(function () {
                setTimeout(function () {
                    throw error;
                }, 10);
            });
        });

        var errorTimeout = setTimeout(function () {
            done(new Error("Wasn't caught"));
        }, 100);

        domain.on("error", function (theError) {
            expect(theError).toBe(error);
            clearTimeout(errorTimeout);
            done();
        });
    });
});

[ForbesLindesay]

What about code using Q.defer()...how does that work with arbitrary resolving code.

[domenic]

@ForbesLindesay I don't quite see what you're getting at? E.g. what would be a potential test case.

Also I just realized I totally forgot bullet point 1 in my OP. It probably works though. Lemme add another test.

[ForbesLindesay]

Something like the following, where request hasn't done anything specific to be domain aware?

var error = new Error("should be caught by the domain");
var d = domain.create();

d.run(function () {
    callAsync().done();
});

var errorTimeout = setTimeout(function () {
    done(new Error("Wasn't caught"));
}, 500);

d.on("error", function (theError) {
    expect(theError).toBe(error);
    clearTimeout(errorTimeout);
    done();
});

function callAsync() {
    var def = Q.defer();
    request('https://www.google.com', function (err, res) {
        def.reject(error);
    });
    return def.promise;
}

(I haven't actually tried this)

[mikeal]

actually, after a small refactor several months ago, the calling of the request callback always happens in an event handler on the Request event emitter so it's already bound to the current domain (assuming there is one) by nature of being an event emitter. in other words it's better to think of request's callback API as an eventemitter event handler. a better illustration might be redis' callback API which works on top of a connection pool and therefor isn't automatically scoped to the current domain.

[domenic]

@mikeal is there a simple async function we can call (or setup we can create) to exhibit this behavior, without dragging in a redis library? E.g. some way I could modify the test case above to use something other than setTimeout and somehow escape the domain.

[mikeal]

you could create a little callback api around a tcp server and hit it a few times, popping the callbacks off of an array.

setTimeout and nextTick will both already get trapped.

On Nov 28, 2012, at November 28, 20129:04 PM, Domenic Denicola [email protected] wrote:

@mikeal is there a simple async function we can call (or setup we can create) to exhibit this behavior, without dragging in a redis library? E.g. some way I could modify the test case above to use something other than setTimeout and somehow escape the domain.

—
Reply to this email directly or view it on GitHub.

[domenic]

Would love some sample code there when you get the time; I still can't quite imagine how that works.

Since nextTick is already trapped and all Q callbacks get called in nextTick, it seems likely we're already OK. But better safe than sorry.

[isaacs]

@domenic Here's some code that will not behave properly.

// The "pool"
var EE = require('events').EventEmitter
var e = new EE
e._interval = setInterval(function() {
  e.emit('beep', Math.random())
}, 100)

// the API
function beep(cb) {
  e.once('beep', cb)
}

// the client
var domain = require('domain')
var d = domain.create()
d.on('error', function(er) {
  console.error('got an error', er)
})
d.run(function() {
  beep(function(n) {
    // we've escaped the domain!
    throw new Error('oops i accidentaly the javascript')
  })
})

[isaacs]

So, the Bad Thing only happens because the "async" call is not using any new async mechanism, but instead hopping onto the event emitted by something that was set up before entering the domain.

As a result, because callbacks are not implicitly bound (though any way to get them "into the future" IS implicitly bound, if it's created right now), the callback gets called outside the domain.

In this case, it can be solved by using an explicitly bound callback:

// The "pool"
var EE = require('events').EventEmitter
var e = new EE
e._interval = setInterval(function() {
  e.emit('beep', Math.random())
}, 100)

// the API
function beep(cb) {

  // -=-=-=--=-=-=-=-=-=-=-=-=-
  // THE FIX:
  if (process.domain)
    cb = process.domain.bind(cb)
  // -=-=-=--=-=-=-=-=-=-=-=-=-

  e.once('beep', cb)
}

// the client
var domain = require('domain')
var d = domain.create()
d.on('error', function(er) {
  console.error('got an error', er)
})
d.run(function() {
  beep(function(n) {
    // there is no escape!
    throw new Error('oops i accidentaly the javascript')
  })
})

There are two ways to look at this.

Since a lot of people use flow control libs, you can solve a lot of un-found problems by putting explicit binding in the flow control libs. (Q, async, etc.)

Otoh, that's potentially adding a cost which is unnecessary most of the time. There are not many client-pooling libs out there that re-use event emitters, and provide a callback API. It's probably easier to just patch it in the few places where it's actually broken, especially they're still broken even if the flow control libs are patched, that won't help people who aren't using Q or async.

[domenic]

@isaacs would this also be a fix?

function beep(cb) {
  e.once('beep', function () {
    var args = arguments;
    process.nextTick(function () {
      cb.apply(null, args)
    })
  })
}

If so I imagine Q is already fine as we do something similar.

In either case thanks so much for taking the time to give a nice example and talk us through the problem. I think it'll be worthwhile adding to Q (if we don't have it working already); it'd be far from the first unnecessary cost we've taken on ^_^

[isaacs]

@domenic No, it would not fix it. When you create the nextTick, you're out of the domain. nextTick will be implicitly bound to the currently active domain, but when you're calling process.nextTick, there is no currently active domain, because you're in the context of the e object, which does not have a domain.

[ForbesLindesay]

So the test case (which we suspect fails) could look like:

var error = new Error("should be caught by the domain");
var d = domain.create();

var e = new require('events').EventEmitter

d.run(function () {
    callAsync().done();
});
e.emit('beep');

var errorTimeout = setTimeout(function () {
    done(new Error("Wasn't caught"));
}, 500);

d.on("error", function (theError) {
    expect(theError).toBe(error);
    clearTimeout(errorTimeout);
    done();
});

function callAsync() {
    var def = Q.defer();
    e.once('beep', function () {
        def.reject(error);
    });
    return def.promise;
}

P.S. I hadn't been referring to @mikeal's request library, but rather a generic, badly behaved request method that retrieved something from a web-server.

[domenic]

OK I fixed @ForbesLindesay's test (had to modify it slightly to get it to fail):

it("should take care of re-used event emitters with `done`",
   function (done) {
    var error = new Error("should be caught by the domain");

    var e = new EventEmitter();
    setTimeout(function () {
        e.emit("beep");
    }, 100);

    d.run(function () {
        callAsync().done();
    });

    var errorTimeout = setTimeout(function () {
        done(new Error("Wasn't caught"));
    }, 500);

    d.on("error", function (theError) {
        expect(theError).toBe(error);
        clearTimeout(errorTimeout);
        done();
    });

    function callAsync() {
        var def = Q.defer();
        e.once("beep", function () {
            def.reject(error);
        });
        return def.promise;
    }
});

But I am trying to think if there is a case involving just then, analogous to this passing test, that we should be handling. @ForbesLindesay ideas?

[ForbesLindesay]

The only thing I can think of is assimilating poorly behaved promises (where poorly behaved means they don't work with domains)

[domenic]

Do you think it's possible to get this to pass? I couldn't, on first try:

it("should take care of re-used event emitters with handlers",
   function (done) {
    var error = new Error("should be caught by the domain");

    var e = new EventEmitter();
    setTimeout(function () {
        e.emit("beep");
    }, 100);

    d.run(function () {
        callAsync().done();
        Q.resolve().then(function () {
            e.once("beep", function () {
                throw error;
            });
        });
    });

    var errorTimeout = setTimeout(function () {
        done(new Error("Wasn't caught"));
    }, 500);

    d.on("error", function (theError) {
        expect(theError).toBe(error);
        clearTimeout(errorTimeout);
        done();
    });
});

[domenic]

Meh. If someone is really using domains and Q together and runs into an edge case where we didn't manage to make it work, they can bring it to our attention. I think we knocked out most of this.

[CrabDude]

If you want to guarantee all async errors are "caught" / callbacks are bound to the appropriate domain, there are only 2 options:

1. Shim EventEmitter to bind callbacks to the active domain at time of binding instead of ee.domain.

   This solves the long lived connection issue but fails for socket pooling (e.g., forever-agent used with http.request). trycatch does this.

2. Ensure callbacks are bound to the active domain.

   This is cumbersome at the application level, but expected at the library level (e.g., forever-agent should reset domains appropriately). Unfortunately, Q has no control over call stacks where Q is not used, which is why trycatch takes the first approach.

So, why am I mentioning this?

Because really the test that you're trying to get passing isn't related to promises or Q in any way; it's an issue with domains. My recommendation? Use trycatch.

[grahamrhay]

Hi,

Is there a reason why this wouldn't (/shouldn't) work?

it("wtf", function (done) {
    var domain1 = domain.create();
    domain1.run(function() {
        console.log('domain1');
        var promiseFactory = Q.fbind(function() {
            console.log('here1');
            return true;
        });
        var promise = promiseFactory();
        promise.done(function() {
            console.log('here2');
            throw new Error('something bad');
        }); 
    });
    domain1.on('error', function(err) {
        console.log('error', err.stack);
        domain1.exit();
        domain1.dispose();
        var domain2 = domain.create();
        domain2.run(function() {
            console.log('domain2');
            var promiseFactory = Q.fbind(function() {
                console.log('here3');
                return true;
            }); 
            var promise = promiseFactory();
            promise.done(function() {
                console.log('here4');
                done();
            }); 
        }); 
    }); 
});

Where by "doesn't work", I mean that the promise in the 2nd domain is not executed. If I remove the call to dispose the 1st domain, it works as expected.

[behrad]

and whats the best decision when composing a ROBUST ACK-based callback-based IOC container which call's user's callback and at the end should be notified with the passed done callback, providing that the container wants to guard against user's code probable un-handled errors (which will cause no done callback be called later, and container won't get notified) and also not to swallow un-handled errors in user's code context?

if should use domains (to guard against all type of errors), then

1. If user has not any active domains, we provide a domain, catch errors, re-emit them on an EE so that user can know about it!?
2. What if user had an active domain? How can we chain/propagate errors between different domains so that both container and user can do their handlings?

I've seen libs like https://github.com/epeli/qdomain and https://github.com/CrabDude/trycatch but can't yet get the benefits from mixing promises with domains, I found no other solutions for catching all types of errors with promises without domains.

[danawoodman]

Why is this the recommended approach if domains is being depricated? https://nodejs.org/api/domain.html

[CrabDude]

@danawoodman I'm not certain who you're asking or which recommendation you're referring to, but...

Using domains is no longer recommended since, as you pointed out, they're deprecated. The issue of catching all (async) errors remains an outstanding issue that trycatch alone solves. trycatch alone also properly resets the current stack in a consistent predictable manner.

Last I checked, trycatch works with q and other promise libraries as I use q myself in production.

[danawoodman]

Sorry, I was referring to a linked issue above in kue where they recommend using domains despite their deprecation: https://github.com/Automattic/kue#prevent-from-stuck-active-jobs

[behrad]

@danawoodman That section in Kue docs is older than domains deprecation, so please create an issue in Kue to update the docs.