Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Promise returned by then() doesn't chain progress #58

Closed
nonplus opened this Issue Oct 9, 2012 · 18 comments

Comments

Projects
None yet
2 participants
Contributor

nonplus commented Oct 9, 2012

Currently the promise returned by then() doesn't propagate progress() notifications. In the following example, I would expect both progback1 and progback2 to be called with "update" but only the first one is called:

var dfd = when.defer();
var p1 = dfd.promise;
var p2 = p1.then(null, null, progback1);
p2.then(null, null, progback2);
dfd.progress("update");

I don't think the spec addresses this, but it looks like a bug to me. Is it the intended behavior? If so, that really limits the usefulness of progress reporting. If not, I can add some unit tests and create a pull request.

The behavior can be fixed in _then = function (callback, errback, progback) by replacing:

progback && progressHandlers.push(progback);

with:

progressHandlers.push(function (update) {
    progback && progback(update);
    deferred.progress(update);
});

FWIW, jQuery's promises propagate progress. So does node-promise (at least from looking at the code).

Owner

briancavalier commented Oct 10, 2012

Hey @nonplus, I think you're right that it's a bug in the current implementation. Thanks for the example code snippet. I'll look deeper into it tomorrow.

You're right that the spec doesn't really address progress propagation. To be honest, I don't use progress events all that much, and they have started to seem rather strange to me. They don't propagate in the same way as resolutions, i.e. there's no change for handlers to transform progress events, and it's not clear to me that progress events are meaningful to observers that are far down the promise chain.

For example:

var a, b, c;
a = functionThatReturnsAPromise();
b = a.then(transformResult);
c = b.then(transformItAgain, null, observeProgress);

In that case, transformItAgain has (hopefully!) been implemented in a way that it can consume b's result, but may not know anything about a's result at all, and doesn't need to. But if both a and b issue progress events, and they propagate thru to observeProgress, will observeProgress know what to do with them? It will probably understand b's progress events, but what if a's progress events are some radically different format (e.g. a string or number instead of an object). In that case, it seems like observeProgress has to inspect every progress event to see if it can understand it or not.'

Obviously, that's a trivial example, but a, b, and c could be spread out across multiple functions, or even multiple promise-aware libraries in an app, which seems like it could make it kinda tricky to implement a progress handler, since it might receive progress events that it knows absolutely nothing about.

I wonder if it makes sense to allow progress handlers to return a value in a similar way as resolution handlers, and their return value is propagated to the next promise's progress handlers.

Anyway, it sounds like you've been using progress events a lot more than I have, though, so if you have some use cases that you could share, or any thoughts or ideas, I'd really appreciate it. That might help me get my head around a better long term approach.

Thanks!

Contributor

nonplus commented Oct 10, 2012

We've been using progress handlers to display updates from asynchronous operations in the UI. The progress is often reported from a fairly deep promise chain, so the chaining is crucial for us. I noticed the missing progress behavior when converting our app from jQuery's promises to when.js (spending a couple hours trying to fix my unit tests until I figured out what the problem was).

We actually do have some situation where multiple chained handlers issue progress events, but we're very careful about consistency (using a string intended for UI display) and about low verbosity (otherwise it can get expensive).

Although we don't use them that way, I think it would make sense for progress resolution handlers to be able to return a different value to make them mirror the behavior of success and error callbacks. Returning undefined should probably stop propagation. This would allow code to deal with mapping and silencing progress from heterogeneous libraries.

BTW, as much as I dislike some aspects of jQuery deferred promises, their progress implementation is pretty convenient. Specifically, the last progress value is remembered and a progress handler is invoked with it even if it subscribes after the promise had been resolved. I.e. in the following code, progback will be called with "Update", which is different from the when.js or node-when behavior.

var dfd = $.Deferred();
dfd.notify("Update"); // jQuery calls the progress method "notify"
dfd.resolve("Done!");
dfd.promise().then(null, null, progback);
Owner

briancavalier commented Oct 10, 2012

Thanks for the additional info, it definitely helps! I created the progress-propagation branch to explore some ideas. It has a change that allows progress events to propagate, plus:

  1. It allows progress handlers to transform the update by returning a value. The transformed value will be seen by progress handlers on the next promise (not by subsequent handlers on the same promise--i.e. it works like resolutions/rejections). Actually, the current implementation requires progress handlers to return a value ... that might not be right, though ... does that still break your use cases?
  2. If a progress handler throws an exception, it stops progress propagation, i.e. the progress update is not forwarded to the next promise.

If you could take a look and let me know your thoughts, especially about 1, I'd appreciate it. We can continue to evolve the branch until we find something that feels right.

As for the jQuery behavior of always sending the last progress update, I'll need to think about that. Initially, it doesn't make sense to me, since conceptually, there simply cannot be progress after resolution. It also seems strange in that progress handlers registered before resolution would see the last progress event at the instant it is issued, but progress handlers registered after resolution will see it at the instant they are registered, which may be far removed from the time it was issued.

I'm not aware of any other promise implementation that does that. I'll give it more thought, tho. If you have a use case, that might help show how it can be useful.

Thanks again!

Contributor

nonplus commented Oct 10, 2012

Having the progress handler return a value doesn't break my case. However, your initial implementation does break propagation since you removed progress chaining on line 282.

Contributor

nonplus commented Oct 10, 2012

The reason why the jQuery behavior for sending the last progress update makes sense is that you often need to be able to specify the initial progress notification before a promise consumer has had a chance to add a progress callback.

The following is a common scenario for me:

getFilesToDownload()
    .then(downloadFiles)
    .then(displaySuccess, displayError, displayProgress);

function getFilesToDownload() {
    var dfd = when.defer();
    var promise = dfd.promise;
    dfd.progress("Getting files to download...");
    ...
    return promise;
}

function downloadFiles(urls) {
    var dfd = when.defer();
    var promise = dfd.promise;
    for(var i = 0; i < urls.length; i++) {
        dfd.progress("Downloading file " + (i+1) + " of " + urls.length));
        promise = when(promise, ...);
    }
    return promise;
}

With the current when.js implementation, the "Getting files to download", and the first "Downloading file 1 of x" progress messages will never be displayed because when they're issued no one is listening yet.

My workaround is to use setTimeout() to delay the initial progress message, but that's pretty hacky...

Owner

briancavalier commented Oct 10, 2012

Interesting. Actually, using a nextTick (setTimeout) in this case seems no worse to me, whereas allowing promise events to be potentially observed out of order seems more strange (e.g. some resolution handlers could be called before some progress handlers). Hmmm, in this case, tho, nextTick doesn't totally guarantee strict ordering, since jQuery doesn't force nextTick resolutions.

Some promise implementations introduce a nextTick on resolutions and progress events for this very reason ... by nextTick'ing everything, it's possible to avoid this situation while also still guaranteeing strict ordering.

I've resisted introducing nextTick in when.js, tho because the performance hit has not yet been worth the tradeoff, and I hadn't encountered a truly compelling use case. This could end up being a good reason to do it, tho.

You've given me even more to think about :)

Contributor

nonplus commented Oct 10, 2012

I think it might be legit to only call the progress handlers on unresolved promises - this would avoid the resolution handlers begin called before progress handlers. I'm actually not quite sure what jQuery does...

FWIW, this is what my current workaround for the notification looks like - note that it ensures that the initial progress message is fired before any resolution/rejection (or subsequent progress updates):

var dfd = defer();
var updateProgress = true;

// We need to display progress asynchronously
// in order for the caller to be able to subscribe
// since when.js only notifies already subscribed progress handlers
setTimeout(sendNotification, 0);

function sendNotification() {
    if (updateProgress) {
        dfd.progress(message);
        updateProgress = false;
    }
}

// Here's where the asynchronous work happens
promise = ...;

// Chain the promise to our deferred, but ensure the notification message comes first
when(promise, function (value) {
    sendNotification();
    dfd.resolve(value);
}, function (err) {
    sendNotification();
    dfd.reject(err);
}, function (msg) {
    sendNotification();
    dfd.progress(msg);
    return msg;
});

return dfd.promise;

@briancavalier briancavalier added a commit that referenced this issue Oct 10, 2012

@briancavalier briancavalier Test case for callback returning a promise that issues progress event…
…s. Very tricky case due to promise resolution order. See #58
ab919c6
Owner

briancavalier commented Oct 10, 2012

I wonder if something like this will work for you and be more compact, since when.js deferreds can be resolved with another promise (effectively equivalent to chaining one promise to another):

// Second param to always() is a progress handler
when(promise).always(sendNotification, sendNotification);
dfd.resolve(promise);
Owner

briancavalier commented Oct 10, 2012

Still need the setTimeout, though (sorry if that wasn't clear).

Contributor

nonplus commented Oct 10, 2012

Yeah, that should work, although it'll also need to chain the promise to the deferred. To summarize:

setTimeout(sendNotification, 0);
when(promise, sendNotification, sendNotification, sendNotification);
when.chain(promise, dfd);
return dfd.promise;
Owner

briancavalier commented Oct 10, 2012

Yep, that'll work just fine, too. They are, in fact, equivalent: when(promise).always(f, f) == when(promise, f, f, f); and d.resolve(promise) == when.chain(promise, d);. The only advantage to using when.chain is that it allows you to trigger d's resolution with a different value, if you need to: when.chain(promise, d, valueToResolveDWith)

@nonplus nonplus added a commit to nonplus/when that referenced this issue Oct 11, 2012

@nonplus nonplus Improved test case for callback returning a promise that issues progr…
…ess events. See #58
230b7be

@briancavalier briancavalier added a commit that referenced this issue Oct 11, 2012

@briancavalier briancavalier Merge pull request #59 from nonplus/progress-propagation
Progress propagation - Fixed progress callback for unresolved promises. See #58.
1d6adb0
Owner

briancavalier commented Oct 11, 2012

I just merged #59 into the dev branch, as well. Thanks again for the help with this!

Owner

briancavalier commented Oct 12, 2012

Hey @nonplus, it'd be great if you could jump into this discussion about progress events over at the cujojs google group. I'd like to see what the community thinks is best, and I think your input would be very valuable.

Owner

briancavalier commented Oct 16, 2012

I just implemented this proposal in the dev branch. The only real change from what was in dev before is that exceptions are now propagated thru the progress chain. See the link above for why I think that's best.

I'm def still open to tweaking this, but I'd like to converge on something good enough for a 1.6.0 release soon. Please give it a go and let me know what you think.

Contributor

nonplus commented Oct 16, 2012

Works for me and the proposal makes sense.

  • Stepan

On Tue, Oct 16, 2012 at 9:10 AM, Brian Cavalier notifications@github.comwrote:

I just implemented this proposalhttps://groups.google.com/d/msg/cujojs/QurUiAeTa5E/P3kasS14NMwJin the dev branch. The only real change from what was in dev before is that
exceptions are now propagated thru the progress chain. See the link above
for why I think that's best.

I'm def still open to tweaking this, but I'd like to converge on something
good enough for a 1.6.0 release soon. Please give it a go and let me know
what you think.


Reply to this email directly or view it on GitHubhttps://github.com/cujojs/when/issues/58#issuecomment-9489341.

Stepan Riha
nonplus@gmail.com

Owner

briancavalier commented Oct 17, 2012

Thanks, really appreciate your trying it out. I'm traveling the next couple days, and have a few other little things I want to work out for a 1.6.0 release, but I don't think it will be too far off--probably within the next week or so.

@ghost ghost assigned briancavalier Oct 26, 2012

Owner

briancavalier commented Oct 27, 2012

There are still some issues to work out around exceptions, but we're going to release what is there now as 1.6.0 in the next few days, and iterate on it as needed.

Owner

briancavalier commented Oct 31, 2012

Ok, the same code that was in dev is now release in 1.6.0. There's more thinking to do longer term on tricky cases, like whether progress events are allowed to be promises, and what it would mean for a progress handler to return a promise, etc. There's also some potential interplay between progress events and when.js 2.0 going with fully async resolutions.

For now, @nonplus, I'd like to close this issue, as I think the immediate issue has been resolved. If you run into any problems, please feel free to reopen, or create a new issue. Thanks again for all the help and discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment