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

Using "domains" to catch async errors in all the node helper functions #324

Closed
tyrion opened this issue Jun 23, 2013 · 4 comments
Closed

Comments

@tyrion
Copy link

tyrion commented Jun 23, 2013

var Q = require('q');

Q.nfcall(function() {
    setTimeout(function() {
        throw new Error("not catched");
    }, 100);
})
.then(function() {
    console.log("ok");
})
.fail(function() {
    console.log("fail");
});

and the output is:

tyrion@aws:~/src/uni$ node test.js 

/home/tyrion/src/uni/test.js:5
        throw new Error("not catched");
              ^
Error: not catched
    at null._onTimeout (/home/tyrion/src/uni/test.js:5:15)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

However it would be nice if the error could be catched. This can be accomplished by using the "domain" module.

var domain = require("domain");

function nfcall(callback) {
    var dom = domain.create();
    var nodeArgs = array_slice(arguments, 1);
    var deferred = defer();
    dom.on('error', deferred.reject);
    nodeArgs.push(deferred.makeNodeResolver());
    invoke(dom, 'run', function() {
        callback.apply(null, nodeArgs);
    }).fail(deferred.reject);
    return deferred.promise;
}

Now the output is

tyrion@aws:~/src/uni$ node test.js 
fail
@domenic
Copy link
Collaborator

domenic commented Jun 23, 2013

This is an interesting idea. @kriskowal, what do you think of these semantics? Would they be desirable? I'm not sure myself... leaning toward "no, too much magic."

Note that the following already works with Q:

"use strict";

var Q = require("q");
var domain = require("domain");

var d = domain.create();
d.on("error", function (err) {
    console.log("error caught", err);
});

d.run(function () {
    Q.nfcall(function() {
        setTimeout(function() {
            throw new Error("not catched");
        }, 100);
    })
    .then(function() {
        console.log("ok");
    })
    .fail(function() {
        console.log("fail");
    });
});

@kriskowal
Copy link
Owner

It is very interesting. However, I don’t think it’s practicable in Q. For one, we provide n* functions for browsers as well, where domains are not available. It becomes complicated, although not impossible, to have a conditional require that works both with or without Node.

It breaks my heart, because this is very clever and useful, but it would be wise to decline.

@domenic
Copy link
Collaborator

domenic commented Jul 2, 2013

FWIW I've really started to come around to this idea as a way of plugging a "hole" in the promises + Node.js love story. I was talking with @piscisaureus at NodeConf who wants to make some changes to domains to make them more robust and nice to work with, and part of his idea was vaguely promise-like, but a key part of that was this domain safety net.

It is something we may explore in a separate package or something in the future, creating super-promises augmented by domains that can wrap Node functions to make them ergonomic, yieldable, and bulletproof against both user-derived errors (e.g. throwing inside the callback) and bug-derived logic errors (e.g. double-callbacking or calling back with both an error and a result). These super-promises would likely be highly micro-optimized as well, e.g. sacrificing encapsulation and integrity in favor of prototype-based approaches or purposefully not-next-ticking when coming from a known-async operation (like promises-aplus/promises-spec#104 (comment)).

Stay tuned.

@CrabDude
Copy link

FWIW, this won't ever work with domains, because the reason you must restart with domains is the same reason you must restart without them: the core call stack is not allowed to unwind, creating an undefined state.

It's also coincidentally the same reason why promises catching all exceptions can actually be a liability, since exceptions from core, or exceptions with a slice of core frames sandwiched between userland frames in its call stack should not be caught.

There are two ways to solve this, wrap the userland boundary in try/catch blocks to guarantee core stacks always unwind and don't catch / rethrow TRESUR errors from core (which is what trycatch does), or a Generators + Asynchrony Implementation that separates errors from exceptions, in addition to catching JSON.parse exceptions and never ever throwing.

Either way, we're now using Q at LinkedIn, and eventually we'll be using Generators + Promises, so I'd like to raise awareness of this common misunderstanding of domains since without a solution like trycatch's it would actually be a bug.

@domenic If you remember, I explained all of this to you at nodeconf after your domains presentation included the same inaccurate information.

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

No branches or pull requests

4 participants