Listener functions fail silently

[bjoerge]

The try/catch wrapper around the code that notifies listener functions will swallow up any error occurring in the call chain of a listener function (./when.js#L281). This is not very desirable as any errors inside a listener function will be suppressed.

Consider the following example:

var deferred = when.defer();
deferred.then(function() {
    i.will.produce.ReferenceError
});
deferred.resolve();

This will fail silently, which is clearly not the expected behavior (I would expect a message to appear in the error log of the browser/environment).

[briancavalier]

Hey @bjoerge,

The goal is for when.js to translate uncaught exceptions into promise rejections. So, on one hand, your example is kind of like a try/catch with an empty catch statement. If there are chances that a promise might be rejected, the application should always register a rejection handler with the promise, just as if there chances some code might throw an exception, the app should always try to ensure that it is caught.

So, in this case, the following will catch the error:

var deferred = when.defer();
deferred.then(function() {
    i.will.produce.ReferenceError
}).then(null, function(err) {
    // Catches the ReferenceError from above
    console.error(err);
});

deferred.resolve();

However, I certainly agree that it's not realistic to try to account for accidental developer mistakes like ReferenceErrors everywhere like this! Which is why your example is particularly interesting :) It's a developer error, and it would be nice if when.js helped in debugging it.

IMHO, if when.js did a console.error for every exception it translates, that would generate a lot of extra noise. So in most cases, I think when.js should silently translate exceptions into promise rejections. BTW, other promise implementations, like Q, etc. behave the same way as when.js. The reason is that the following is perfectly reasonable, and simply uses an exception to reject a promise chain:

function doStuff() {
    var deferred = when.defer();

    // Simulate something async, like an XHR
    setTimeout(deferred.resolve);

    return deferred.then(function(result) {
        if(something) {
            // Do awesome stuff and compute new result

            return awesomeResult;

        } else {
            // It's perfectly acceptable to use exceptions like this to reject subsequent
            // promises.  This allows for a familiar programming model, and allows existing
            // functions that aren't promise-aware, and thus may throw exceptions anyway
            // to be used as promise callbacks.

            throw new Error('this causes subsequent promises in the chain to reject');
        }
    });
}

The caller can handle the exception as a promise rejection:

when(doStuff(), handleAwesomeResult, handleFailure);

Ok, after all that, like I said, I do agree that it would be nice if when.js could help the developer figure out the problem in your example. I've been thinking of creating a debug mode for when.js where it logs things like this. It's a little tricky in that a promise library needs to be somewhat "trusted", and untrusted code probably shouldn't be able to turn debug mode off.

If we added a debug mode, do you think that would be a reasonable solution?

[briancavalier]

There's some related discussion over on #13 about providing a .fail() convenience function that may help in registering a handler that will catch some of the scenarios where rejections get swallowed silently.

Another option might be for when.js to identify rejected promises that don't have any errbacks registered with them, and then log a last-ditch error message in that case.

[briancavalier]

We've been focused on getting 1.0 out the door, but now that that's done, I have some ideas for how to handle this.

[bjoerge]

Hey Brian, this totally slipped out of my radar for a while... sorry about that!

Thanks for your answer, I see your perfectly good reasons for the try/catch block. However, are there any reason to not rethrow the error again after rejecting the promise? In that case the error can just propagate up the call chain and be handled as usual in the application code. At least thats what I would expect it to do :-)

In any case, I guess the important thing here is that this is a gotcha developers should be aware of when using when.js (I personally spendt quite a while trying to figure out why things didn't work the way I expected before figuring it out). Maybe adding a note about it to the README could be an idea?

Beyond that, thanks for a great library!

[KidkArolis]

This problem is really getting in the way. Every JS error, e.g. "ReferenceError" gets silently swallowed always everywhere, whenever when() is used which makes it impossible to develop. For example, tests fail silently instead of displaying what error happened, etc.
I'm now trying to catch those errors with reject handlers, but can't seem to get it to work.
I haven't thought too much about it, but it somehow feels like it's a bad idea swallowing errors that are visible in the regular JS..

[briancavalier]

@KidkArolis: when.js 1.1.0 has a new debug module that is starting to experiment with ways of dealing with this. We're definitely looking for feedback, so if you haven't tried it out yet, please give it a go.

One of the essential problems is that there are 2 classes of exceptions here: 1) exceptions that represent coding errors, e.g. ReferenceError that the application shouldn't handle, and 2) "legitimate" application exceptions that the application can and should handle.

Ideally we'd like 1 to propagate all the way to the host environment so you get the usual failure/stacktrace/etc that you're accustomed to seeing, and we'd like 2 to be transformed into a rejection that the application can handle via promise rejection handler.

In addition, for asynchronous operations, exceptions will be happening in a future event loop turn (and thus a different stack frame) than the call to when(), so there's no way for your application to catch them. That's one of the motivations for transforming exceptions into rejections.

Right now, when/debug doesn't do anything super-fancy. It allows you to "tag" promises with an identifier you'll recognize in the log, and logs some info, including the tag and rejection value, when promises reject.

Here are some questions:

1. Should it log stack traces (via ex.stack where supported) for some or all rejections?
2. Should it try to detect "obvious" coding errors like ReferenceError and rethrow those in a way that will propagate to the host environment?
3. Is it helpful to log information when promises resolve, in addition to when they reject?

There's probably a million different things it could do, so we wanted to get it out there for folks to try so that we can tweak it to make it truly useful.

[briancavalier]

I just pushed a change to the when-debug branch that forces certain built-in Error types (RangeError, ReferenceError, SyntaxError, and TypeError) to propagate to the host environment when they occur in a promise handler.

It may be way to heavy-handed, but I figured it was at least worth experimenting with to see if it helps us figure out a path to better ways to debug promises.

If you guys get a chance, please try it out. I'd love to hear your feedback.

Thanks!

[KidkArolis]

Could you give an example of a "legitimate" application exceptions that the application can and should handle?

Again, without having thought about it too much, it seems like if the developer decides not to handle errors, when.js shouldn't handle them itself. I mean, if you are expecting something to potentially fail within your when.js callbacks, then you have an should explicitly try/catch.

I'll give when-debug a shot. Great work otherwise, writing promise based code is quite revealing, especially in nodejs :)

I'll also 'move' to issue #28 now, as the discussion there got more elaborate. Should this one be closed as a duplicate? On the other hand, #28 is getting 'polluted' with other topics.. :)

[briancavalier]

@KidkArolis Thanks, I have some ideas for how to make when.js even more useful in node, such as promisify-ing node functions that use the "standard" node callback style.

Let me know what you think after trying when/debug. It's a little tedious to use in node right now, but I have a plan for how to make it easier to enable/disable globally.

tl;dr :)

In my experience, it's fairly common to use exceptions to indicate errors at one level of an application that can be handled at a higher level--not necessarily the immediate caller. For example, you might have a parser that you use to parse some user input, and it might throw exceptions that you let propagate to a higher layer of your application that coordinates validation and display of feedback to a user.

One important characteristic of exceptions is that only the code that generates the exception, and the code that catches the exception needs to have any special code: throw and try/catch respectively. None of the code in-between needs to use try/catch, or even be aware of the exception at all. In my experience, that allows error handling to reside at whatever level makes the most sense in an application architecture.

Interestingly enough, promises can be used in a similar way by returning them up the stack instead of passing them down the stack. A low level routine can return a promise, which can then be propagated (via return) up the stack until it reaches a level of the application where it is appropriate to handle the failure, at which point it can observe the promise (e.g. using when()). Like exceptions, the code in-between doesn't need to be aware of the promise--although it does need to propagate return values up the stack. With exceptions, the language/platform gives us that for free, but with promises we do have to be a little more diligent. The price of dealing with asynchronous operations :)

The typical model for handling asynchrony in JS is to pass callbacks down the stack, but I've come to believe that with promises, a better model is to return them up the stack. I wrote a bit about that here, and here

[KidkArolis]

I keep commenting try/catch blocks in when.js file everytime I use it. For example, just now, I was writing a test like this in Buster.JS:

test1: function (done) {
  doStuff().then(function (result) {
    assert.equals(result, 42);
    done();
  });
}

When the assertion fails, Buster would normally report this test as failed. However, when.js seems to have handled the exception and so Buster.js instead reports that the test timed out.
I'm still confused about whether I'm using when.js wrong, or whether it shouldn't touch exception stuff and I should manually try/catch things in my functions and reject deferreds if I want to.

[KidkArolis]

I just got bitten by commenting those try/catch blocks.

try {
  var result = JSON.parse(response);
  deferred.resolve(result);
} catch (e) {
  deferred.reject(e);
}

was executing both resolve and reject, because somewhere up the stack there was an exception, that bubbled up to this try/catch block. Normally, when.js would have handled that, but I obviously messed it up by commenting try/catches in when.js itself.

How do I solve the buster.js problem though?

[briancavalier]

Hey, I'm traveling and have to respond from my mobile right now, so
please excuse my brevity :) I'll answer in more detail when I get to a
real keyboard.

With async tests in buster, you need to ensure that done() is called
in the case of success and failure, or, as you saw, buster will report
a timeout.

Take a look at when.js's own unit tests for ways to do that. For
example, you can use .always(done) in many cases.

On Jun 20, 2012, at 6:26 PM, Karolis Narkevicius
reply@reply.github.com
wrote:

I just got bitten by commenting those try/catch blocks.

try {
 var result = JSON.parse(response);
 deferred.resolve(result);
} catch (e) {
 deferred.reject(e);
}

was executing both resolve and reject, because somewhere up the stack there was an exception, that bubbled up to this try/catch block. Normally, when.js would have handled that.

How do I solve the buster.js problem though?

---

Reply to this email directly or view it on GitHub:
#18 (comment)

[KidkArolis]

Ok. It's starting to make more sense :)
I'll stop editing when.js :-"
I'll start using when/debug more.

Buster.js documentation is discussing this exact problem about assertions throwing exceptions and how that should be handled in the async tests (http://busterjs.org/docs/test/test-case/#async-tests).

Basically I changed the code to something like

test1: function () {
  return doStuff().then(function (result) {
    assert.equals(result, 42);
  });
}

I return a promise instead of using the done() callback.

And some thinking out loud now..
Say I'm using when/debug when developing my application. When I run tests and they fail, when.js echos the stack trace of Buster's assertion exception which is not really needed. Is that because it's hard to know which exceptions when/debug should log and which one's it shouldn't?
I'm wondering, if I would always correctly setup my promise chains such that any rejected promise always bubbles up (or down?), then my error handling code or tests would always pick that error up. Even if it's say a "developer" error (e.g. ReferenceError), the fact that my promises don't get stuck, but are always returned would mean that somewhere I will notice the error (unless I don't have a single error handler which is also bad). I'm just saying, perhaps the reason I was complaining about when.js swallowing too many errors earlier is that I'm not using when.js correctly. Hm..

[KidkArolis]

I also like the idea you mentioned earlier of "identify[ing] rejected promises that don't have any errbacks registered with them, and then log a last-ditch error message in that case."

[briancavalier]

Finally back to a real keyboard, so I'll try to catch up on answering :)

Returning a promise from buster's unit tests is great. It's the ideal, imho, and I'm glad that worked out. I had not been doing it because at some point I was having trouble with it. I should revisit it for when.js's unit tests because it's so much cleaner, and matches the "one assertion per test" model.

Yeah, it's tough, if not impossible, for when/debug to know which exceptions to log, since where and how to handle can be very application and architecture specific (plus dev-time errors, like SyntaxError). I think the best it can do is try to guess, and err on the side of being overly verbose. For example, it currently guesses that SyntaxError, TypeError, and the like are programmer error, and rethrows them in a different stack frame so they are uncatchable and propagate to the VM.

The last paragraph in your previous comment is right on the mark: returning promises up the stack is the way to go. That guarantees that a rejected promise at a low level will propagate up to a higher level in the application where it should be handled ... i.e. promises behave like an async analog to exceptions. The big difference, of course, is that Javascript VMs have no native understanding of promises, so they can't automatically print stack traces for unhandled rejections like they can for unhandled exceptions.

Related to that, if you find yourself creating lots of deferreds, that can be a sign that your promise usage is "inverted", that is, you're passing too much down the stack. The purpose of a deferred really is to adapt to an environment whose basic building block is callbacks. Once lower-level code has generated promise, typically by using a deferred, code above it can usually just return the result of calling when() or .then() on that promise, adding it's own handlers (which may mutate the result, switch from resolved to rejected, or vice versa). And so on, up the call stack. That way, any promise that ultimately ends up in a rejected state and makes it up the stack can be handled.

Hope that helps, rather than confusing things!

[briancavalier]

Closing, as there have been lots of improvements to when/debug, and there hasn't been any activity here for a while