How to handle exceptions thrown in progress handlers?

[domenic]

They should presumably stop progress propagation, but how should they be reported?

[domenic]

Some options:

• Ignore them, or log them, but don't let them have substantial impact. (Maybe pass to Q.onerror if it exists.)
• Throw in nextTick. Not great since an error in the progress handler doesn't seem fatal, conceptually, but this will bring down your app in Node.js or Windows 8.
• Transform the returned promise into a rejection. This is what I favor, simply because the other options kind of suck. @briancavalier thinks that progress handlers should not be allowed to effect the state of the returned promise though (i.e. they're not as important as fulfillment or rejection handlers); I can see that logic.

Previous discussion in the when.js group

[briancavalier]

Here's a bit more about why rejecting the returned promise seems weird/dangerous to me.

var p2 = p1.then(null, handleRejected, handleProgress);

I think most people interpret Promises/A to mean that handleProgress receives updates on progress toward fulfillment/rejection of p1. By extension, that also means progress toward p2, but I don't think most people perceive it that way. On the other hand, handleRejection is related only to the time at/after p1 is rejected. So, handleProgress and handleRejection deal with completely disjoint times in p1's lifecycle.

One might think that since handleProgress deals with progress leading (most directly) to p1's fulfillment/rejection, it should reject p1 if it throws, but that's obviously wrong. It grants too much authority to anyone who has a reference to p1--authority that is reserved for the resolver. That leaves the option to reject p2.

Let's assume it works that way. I don't necessarily think it's wrong, but I think it creates some weird situations. For example:

var pending = createPending();
var p1 = pending.promise;

var p2 = p1.then(handleFulfilled1, handleRejected1, function(update) {
    throw new Error();
});

p2.then(handleFulfilled2, handleRejected2)

pending.progress('wat');

At this point, p2 would become rejected, while p1 is still pending. That's kind of a weird state: The tail of the promise chain is unwinding, but the head is still pending!

That leads to the obvious next question of what happens when the head does start to unwind? IOW, what should happen when p1 fulfills or rejects? Promises/A+ says that handleFulfilled1 or handleRejected1, respectively, must be called. But what do we do with the result (be it a return value or a thrown exception), since p2 has already been rejected?

This seems like a huge problem: No one can observe the result of handleFulfilled1 or handleRejected1

I don't have any better options to offer right now, but just wanted to write down some of the potential pitfalls.

[ForbesLindesay]

Personally I'm not really a fan of adding progress as a third argument to .then(). If we instead treated it as a totally separate .progress(progback) that might open up completely different options with regards to error handling. In the once case where I've had to deal with progress so far (when using promises) I did so by having my promise also be an event emitter and emit a progress event, which just let exceptions throw.

[domenic]

That leads to the obvious next question of what happens when the head does start to unwind? IOW, what should happen when p1 fulfills or rejects? Promises/A+ says that handleFulfilled1 or handleRejected1, respectively, must be called. But what do we do with the result (be it a return value or a thrown exception), since p2 has already been rejected?

This seems like a huge problem: No one can observe the result of handleFulfilled1 or handleRejected1

This convinces me: the progress handler cannot control the state of the promise.

Throw in nextTick is possibly the best option now. Or remove progress entirely, but, I dunno, it's so convenient...

[domenic]

WinJS swallows them, for the record.

When.js propagates thrown exceptions as progress values.

Dojo, from what I can tell by inspection, rejects p2.

[briancavalier]

Yeah, propagating the error was my short-term "solution", until we could figure out something better. We listed progress propagation as an experimental feature in a recent release.

I'm beginning to think separating progress from then might be the path to the best solution.

[domenic]

I mean, it's a shame to have less features than any other promise library, even if those features are problematic. Q is, after all, the grandaddy of promise-libraries when it comes to features.

Put another way, the reason I originally got off my butt and added promise support was because WinJS had it, and I needed to assimilate some WinJS promises with progress. I can imagine someone coming from jQuery and having similar sadness.

@kriskowal any thoughts?

[domenic]

BTW this is the last item on the 0.8.11 release list :). Once we figure it out we can move on to 0.9, i.e. the "kill all the deprecated methods!" release.

[kriskowal]

I really don’t. I’ll defer to your judgement, @domenic. In the end, we will just need more practice with progress to get a better idea. I reserve the right to ax it outright in the future if it turns out that the feature is useless or hazardous.

[domenic]

I'll try running it by the continuum then but my natural instinct is to throw in nextTick, as dangerous as that can be.

[kriskowal]

On Sat, Nov 24, 2012 at 5:05 PM, Domenic Denicola
notifications@git.luolix.topwrote:

I'll try running it by the continuum then but my natural instinct is to
throw in nextTick, as dangerous as that can be.

Sounds sane to me.

[ForbesLindesay]

My two cents:

separate propagation from handling. The propagation API is then .then(callback, errback, progressPropagator) and is just there to transform the progress. I'm not sure what the best bet is here, but I'm not totally against it being thrown in nextTick.

We should then discourage actually handling progress in the propagation handler, and instead have a separate .progress(cb) which should just throw in next tick and return undefined.

As an alternative, axe propagation handling from promises altogether (except the most basic case). Instead, just forward progress evens if the progressPropogator is true and drop them if it's false. That takes care of the case:

var json = request('foo.com/file.json')
  .then(function (res) {
    return JSON.parse(res);
  }, null, true);

But ignores the case:

var res = asyncOpA()
  .then(function (res) {
    return asyncOpB(res);
  }, null, false);

At that point we just say "You're on your own". But we let people do the propagation manually like so:

var opA = asyncOpA();
opA.handleProgress(progressA);
var res = opA
  .then(function (res) {
    var opB = asyncOpB(res);
    opB.handleProgress(progressB);
    return opB;
  }, null, false);

function progressA(val) {
  res.emitProgress(val / 2);
}
function progressB(val) {
  res.emitProgress(0.5 + val/2);
}

The idea here is to do exactly what event emitters do, let an outside party add progress events into a promise, so that the consumer of a promise can still choose to make the promise emit progress events even if it wouldn't do so naturally.

This has other nice use cases. Consider the case that you're given an API that returns promises, which don't support progress, but where you know approximately how long each async operation takes, you could add progress with something like:

function addProgress(promise, expectedTime) {
  var start = Date.getTime();
  var int = setInterval(function () {
    promise.emitProgress(Math.min((Date.getTime() - start) / expectedTime, 1));
  }, 100);
  promise.then(function () {
    clearInterval(int);
  }, function () {
    clearInterval(int);
  });
}