Confusion about `done`, unhandled rejections, and dev tools

[domenic]

Intro

Both in the current IDL:

// No matter what approach is pursued, integration with developer tooling is
// key. Developer consoles SHOULD log un-caught errors from future chains.
// That is to say, if future.done(onresponse); is the only handler given for
// a resolver which is eventually rejected, developer tools should log the
// unhandled error.

...

  // An end-of-chain callback. Equivalent to .then() except it does not generate
  // a new Future and therefore ignores the return values of the callbacks.

and in @DavidBruant's message:

.done to end the chain (and have the devtools report uncaught errors at the end of the chain)

I see some confusion over how done and unhandled exceptions are expected to work. Let me outline how I envision them, and how they work in current promise libraries, which I think is better than---or at least clearer than---the sentiments expressed above.

The Problem

See https://github.com/promises-aplus/unhandled-rejections-spec/issues/1 for a more in-depth explanation if the below doesn't immediately make sense to you.

If you do this:

let future = new Future({ accept, reject } => reject(new Error('boo!')));

you have created a promise with an unhandled rejection. This is equivalent to the synchronous code

let result = (() => throw new Error('boo!'))();

Of course, the synchronous code would hit window.onerror, since nobody's there to catch it. But we can't do that with promises, because they are first-class values who we can give to someone else who might handle them, say, after 100 ms.

This is the essential problem, in a nutshell. If nobody ever attaches a rejection handler, errors will go unreported and be silenced. Bad!!

The done solution

See https://github.com/promises-aplus/unhandled-rejections-spec/issues/5 for a more in-depth explanation if the below doesn't immediately make sense to you

One solution is to have a rule that you enforce on promise users and creators: always either

• return the promise to your caller, or;
• call done to signal that the buck stops here, and if there are any unhandled rejections, they should hit window.onerror.

That is, done is essentially:

Future.prototype.done = (onaccept, onreject) => {
  this.then(onaccept, onreject)
      .catch(reason => setImmediate(() => throw reason));
};

This rule is pretty good, and used in Q and in WinJS promises with some amount of success. @lukehoban has previously remarked that it ends up not being as much of a burden on developers as you'd initially think, and I agree.

But, it's still error-prone: if you slip up and don't follow the rule even once, you might silence an error forever.

The dev-tools solution

See https://github.com/promises-aplus/unhandled-rejections-spec/issues/2 for some speculations on how to implement this solution in current promise libraries.

This solution involves the dev tools having a hook into the promise library, such that any time an unhandled rejection is created, they are notified. But more importantly, whenever an as-yet-unhandled rejection becomes handled, it notifies the dev tools of that.

With this in hand, you can imagine a panel in the dev tools that shows outstanding unhandled rejections, dynamically adding them as they are created and removing them as they are handled. If they stick around for too long, you know there's a problem.

One way of thinking of this is as an "erasable console.log", i.e. if we could just console.log the unhandled rejections, but then when they get handled, "erase" that entry we wrote to the log. Indeed, Q implements something like this using the fact that when you log arrays in Chrome, they are "live" and auto-update as the array changes. So we log the array of unhandled rejections, and add/remove from it as appropriate. If you ever see a non-empty array in your console for too long, you know there's an issue.

The weak refs solution

This solution was mentioned to me by @erights. Using something like the weak references strawman, promise libraries could maintain a table mapping promises to unhandled rejections. Once the promise gets garbage collected, and the weak ref thus points to nothing, it logs those unhandled rejections.

Unfortunately this cannot be implemented in terms of weak maps, from what I understand. You need the garbage-collection observability which weak maps specifically do not provide.

Conclusion

So, hope that was clarifying. I think the current IDL doesn't express how done is meant to work very well, and seems to tie dev tools to done, whereas they are actually separate and somewhat-complementary solutions.

Hope this helps!

[DavidBruant]

With this in hand, you can imagine a panel in the dev tools that shows outstanding unhandled
rejections, dynamically adding them as they are created and removing them as they are handled.
If they stick around for too long, you know there's a problem.

This is a much better idea than anything else I had considered. I was picturing devtools showing nothing by default and show only something if proven unhandled (because end-of-chain via done or future being GC'ed).

[domenic]

Yay! Glad to have brought that out. My intuition is that if something is proven unhandled, it should be thrown as a normal error and hit window.onerror/show up in the console just like any other error would. It's the not-proven ones that cause a problem.

[DavidBruant]

This is maybe where I'd differ in opinion. When a library has to show the error somewhere, it has to throw it (which gets reported to window.onerror, etc.). A built-in API could decide to just log in the devtools and not bother the runtime with that.

[domenic]

Hmm, I can see that making sense. No strong feelings one way or another. Except maybe you'd still want to expose a global error handler for e.g. logging-back-to-the-server purposes, whether it be window.onerror or Future.onerror or...

[DavidBruant]

I had no strong feeling in one way or another, but the logging-back-to-the-server use case makes a lot of sense. I don't really like the everything-sink window.onerror, so I would be more in favor of an unhandledError event on Futures. But... not very strong on this.

[slightlyoff]

Sorry for not responding to this sooner. I don't see any implications with any resolution to this for the API, only the non-normative text. Is that correct, or have I missed something?

FWIW, I find the WinJS style of clobbering apps with unhandled promise errors to be terribly un-webby.

[domenic]

Indeed, no API changes, just changes to the text. (I hestitate to say "non-normative text," as if everything that's a comment in the .idl file is non-normative, then this repo specifies very little at all :P.)

[slightlyoff]

I'm gonna close this ticket if there aren't objections.

[domenic]

It might be good to leave it open until we can fix the text; I can do that over the weekend.

[domenic]

I was about to fix the wording but then I realized that the done in the polyfill does not actually deal with unhandled exceptions in any way, but instead seems to be just a crippled version of then. Is that intentional?

Put another way, does DOMFuture have any strategy for unhandled rejections at all?

[slightlyoff]

I've been thinking about this and talking with @wycats about error handling, and I think that we're not anywhere close to having a good solution right now.

Automatic swallowing of exceptions (where we are today) is fatal to usability. Delayed exception handling is the oddball situation. .done() and .catch() currently set the default the other way, and this sucks. We need to revisit entirely.

[domenic]

I think tooling is the correct solution (see "the dev tools solution") above.

[domenic]

Delayed exception handling is the oddball situation.

I disagree. When dealing with promises as proxies for remote objects, delayed handling is the norm. When passing promises to promise-accepting functions (e.g. fs.writeFile('dest.txt', fs.readFile('src.txt')).then(() => ...)), it's quite common to delay handling.

In general, the more promise-based your system becomes, the more you delay handling your promises until multiple ticks from their creation. @kriskowal and @erights may be able to confirm.

[erights]

Absolutely -- what @domenic says is correct (including the need for tooling). Prior to E, Original-E had a different exception handling regime that tried to be more "conventional", in that the error reports took a separate control flow path. Prior to that, Joule had yet another design. Before that, Actors and Flat Concurrent Prolog used yet other conventions. Of all these, it was only when we transitioned to E's broken promise contagion, as explained at http://erights.org/talks/promises/paper/tgc05.pdf and part III of http://erights.org/talks/thesis/markm-thesis.pdf , did things just start to work, even for cases we hadn't thought of. Q's rejected promise contagion and DOMFuture's rejected futures contagion have all the same virtues as E's broken promise contagion.

A great case in point is the escrow exchange agent code at http://static.googleusercontent.com/external_content/untrusted_dlcp/research.google.com/en/us/pubs/archive/40673.pdf . To write this correctly, I had to think carefully through many error scenarios, because there are many failure cases which this code logically must deal with. What I found is that one I got the code to where it successfully dealt with the first few cases I examined, I found it dealt with the remaining cases without need for further refactoring.

I suggest this use case as a good test case for any alternative mechanism. I offer it as the "factorial" example of delayed exception handling.

[erights]

@domenic I remember you have a slideshow whose focus is on this issue specifically. I also remember that it is quite good and makes some points I did not make in my papers. Could you post a link to that? Thanks.

[domenic]

Sure, it's on SlideShare. The error stuff starts on slide 62.

[domenic]

@erights: to be fair, I think what @slightlyoff was getting at was not completely destroying the broken promise contagion mechanisms, but instead neutering them. In particular (and, @slightlyoff, please correct me if I'm wrong) I think the idea would be that if a promise is rejected but has no rejection handlers attached by the end of the tick, it throws-on-next-tick.

Thus code like this:

let promise = new DOMFuture(({ reject }) => reject(new Error("boo!"))).then(onFulfilled);

promise.catch(onRejected);

would still work, whereas code like this

let promise = new DOMFuture(({ reject }) => reject(new Error("boo!"))).then(onFulfilled);

process.nextTick(() => promise.catch(onRejected));

would throw an uncatchable error, in addition to triggering onRejected. (Thus, in browsers onRejected would be called, whereas in Node.js it would not be, because the program crashed. :-/)

My reading of @slightlyoff's post was that the second case is "the oddball situation." Thus my arguments were mainly countering that claim, i.e. saying that as your system becomes more promise based, the second case is actually the norm.

[erights]

@domenic I see. @slightlyoff If Domenic has that right, my apologies for misunderstanding you. This is indeed a more subtle issue than I thought at first. But for this subtler issue, I think Domenic again has it right -- there's nothing oddball about that delayed registration. I can probably find some examples in real code if you wish.

Note that weak-refs, hopefully in ES7, will provide us one of the diagnostic tools we need to bridge this gap. Using weak-refs, if a rejected promise gets collected without having notified any handlers, we can arrange that this generates a diagnostic. The promise implementation would have to keep the reason in the promise's executor (post-mortem gc handler), so that it has the diagnostic to report after discovery that the promise has been rejected.

Since E has both promises and weak-refs, we can experiment with this in E. This would probably add value to E in any case, for the same reason that it would add value to JS.

[cowwoc]

Guys,

The specification says done() should thrown unhandled errors to window.onerror. As mentioned above, the current implementation does not. Instead it swallows errors silently.

Can you please make the implementation to follow the specification while you figure this out? Dropping exceptions silently is a big no-no :)

The following (modified) code worked for me:

  var reject = function(reason) {
    // console.log("queuing reject with:", reason);
    async(function() {
      setState("rejected");
      setError(reason);
      // console.log("rejecting with:", reason);
      if (rejectCallbacks.length === 0)
        throw reason;
      else
        rejectCallbacks.pump(reason);
    });
  };

[slightlyoff]

We're removing .done(). Error propagation/notification is going to be left to tools.

[cowwoc]

Alex,

What does that mean?

1. Are you going to remove done() from the DOM Futures specification? I'm looking at http://dom.spec.whatwg.org/#futures-api
2. What does "leaving it to tools" actually entail? Can you provide an example of who will do what in the new specification?

[medikoo]

No done means that there won't be a natural way to expose unhandled errors, and that's very very bad. It should be part of a spec, done is a must for any promise implementation.

[cowwoc]

@slightlyoff I saw the latest commit replaces done() with finally(). While it's true that we are all used to try-catch-finally I would say that the new design is incomplete and that in its current form the old design is actually superior. In a real try-catch-finally design, an exception is thrown even without finally. In your implementation, omitting finally prevents exceptions from being thrown. Fix that, and I would support the new finally approach.

[kriskowal]

In Q, finally is a nuanced method. Roughly:

function finally(callback) {
    return this.then(function (value) {
        return Promise.of(callback()).then(function () {
            return value;
        });
    }, function (exception) {
        return Promise.of(callback()).then(function () {
            throw exception;
        });
    });
}

It is important for convenience that the callback receives no arguments, and important that it be able to delay fulfillment in the fulfillment case or replace the exception in the rejection case, just as a finally block receives no argument, can supersede a thrown exception, but by default passes a fulfillment value through, just as a return value passes through a finally block. A finally clause is typically used for teardown and resource release like promise.finally(server.shutdown).

[cowwoc]

So... is it safe to say this issue needs to be reopened? :)

[kriskowal]

@cowwoc I would advocate a separate issue.

[cowwoc]

A separate issue indicates that we are discussing a separate topic. Aren't we still discussing done, unhandled rejections and dev tools? Is there a good reason to open a separate issue and lose the discussion history that comes with this one?

[slightlyoff]

Sorry for the delay:

• .done() has been removed from Promise.idl, and I'll be removing it from the polyfill today or tomorrow when I do the s/Future/Promise/g renaming.
• .finally() is dropped for the moment as well. It's a nice convenience, and I agree with @lukehoban that we need it, but until other libraries iterate on it some, I'd prefer to keep semantic churn out of the design.
• As for unhandled exceptions, I'm still going to argue that this is a job for devtools. The sooner we get Promise into the platform, the sooner we can start to exert some pressure to get that done on browsers.

[medikoo]

@slightlyoff Thanks for clarification.

Speaking from my experience, promise implementation which doesn't provide natural way to expose unhandled exceptions cannot be perceived as complete.

It's also important to acknowledge that if there's no done (or its counterpart) in promise implementation natively, then done cannot be added effectively custom way, as to escape then's try/catch behavior we need to escape the flow within callback with setImmediate or setTimeout and that means we cannot provide done that throws immediately in case of an error.

It also raises serious performance concerns as just left with then we are forced to create promise objects we don't need, whenever we just want to access resolved value.

Above means that any promise implementation that would rely on this spec and won't provide done on it's own, will have big issues. I work a lot with promises, and I would never decide to use such implementation.

[cowwoc]

1. Shouldn't this issue be re-opened until a satisfactory resolution is reached?
2. The purpose of this library is to act as a polyfill. Removing functionality based on the premise that platforms will add the missing functionality defeats the entire point of a polyfill, does it not? :) You're supposed to fill that gap until the platform adds the functionality.

[cowwoc]

@slightlyoff

The latest spec removes done() and says "The exception is not re-thrown and does not reach window.onerror."

The spec doesn't specify how tools are supposed to handle uncaught exceptions. It just says how they're not supposed to handle them. So where does that leave us ...? Clearly, this issue is not resolved. Please reopen it.