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

Exceptions in progress handlers are not caught #295

Closed
eden opened this issue May 17, 2013 · 5 comments
Closed

Exceptions in progress handlers are not caught #295

eden opened this issue May 17, 2013 · 5 comments

Comments

@eden
Copy link

eden commented May 17, 2013

In this example node program, I expect the "Oops..." exception to be caught by the final fail handler. Instead it's not caught by anything and causes node to crash. Placing the throw in the then handler behaves as expected (it's caught by the fail handler).

var Q = require('q');

var deferred = Q.defer();
deferred.promise.then(
    function() {
        console.log('Complete');
    },
    function(err) {
        console.log('Caught at then 2nd arg', err.stack);
    },
    function(n) {
        throw new Error('Oops...');
        console.log('Got', n);
    }
).fail(function(err) {
    console.log('Caught at fail', err.stack);
});

process.nextTick(function() {
    deferred.notify(42);
    deferred.resolve();
})
@domenic
Copy link
Collaborator

domenic commented May 18, 2013

Hmm, I thought I had good arguments against this, but when I start typing them out they seem less strong. Tagging in @briancavalier in case he has any thoughts; I know it's an area he's considered a lot in when.js.

@briancavalier
Copy link

This comment has most of my thoughts on why this is tricky. Honestly, I had to re-read them, since I haven't throught about this issue in a while. The biggest problems that I see (see the above comment for more detail) are: 1) allowing promise p's progress handlers to cause p to reject grants too much authority, and 2) allowing p's progress handlers to reject children of p creates a weird situation where the outcome of p can't be observed.

I hope there's a better alternative, but the only things that seem at all reasonable to me right are:

  1. pass errors through as progress and force progress handlers to deal with them. This is what when.js does.
  2. "crash", what Q does. This effectively forces devs to ensure progress handlers cannot throw, which honestly doesn't seem like a bad thing at all.

I just re-read this idea from over at Promises/A+, and now I'm very intrigued by it. I need to give it more thought, but would be interested to hear @domenic's and @ForbesLindesay's latest thinking on it as well.

@domenic
Copy link
Collaborator

domenic commented May 20, 2013

Thanks Brian; I knew that rejecting p was out of the question, but @eden's suggestion was allowing p's progress handlers to reject children of p, and your example from that thread was exactly the counterexample I'd forgotten about.

I agree that that other idea was pretty interesting. Although, the strangeness of it (i.e. the way that notify ends up behaving so differently from resolve or reject) makes me further dislike how we've mixed progress into the then signature.

@ForbesLindesay
Copy link
Collaborator

I still think I like that idea better than anything else I've tried. I've not had a chance to experiment with it but returning a promise from the triggerProgress method makes it easy to ignore, crash or reject.

I still think making progress handlers deal with the errors thrown by other progress handlers is downright weird. I think I prefer crashing to that option.

@domenic
Copy link
Collaborator

domenic commented Jul 4, 2013

OK, it's decided. notify will return a promise derived from the various onProgress handlers. See #340.

@domenic domenic closed this as completed Jul 4, 2013
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