Deferred: invoke processes in async groups instead of individually

[jbedard]

This is a first shot at #3227.

This does not invoke deferredTick anywhere else yet.

[mention-bot]

@jbedard, thanks for your PR! By analyzing the annotation information on this pull request, we identified @gibson042, @mgol and @jaubourg to be potential reviewers

[jbedard]

I think this specific line could actually change the execution order of processs.

Previously this single process would be invoked before other scheduled async ones. Now it will invoke other scheduled ones first, then this one last, but it is still "immediately" (see comment a couple line above).

Can anyone think of a case where this is wrong?

[dmethvin]

So the only possible callback that might throw and not be caught is the last one, right?

However, would something like deferred.then(fn1).done(fn2) force fn1 to be synchronous in order to run fn2? If the queue were per-Deferred that wouldn't happen, but it is global/shared.

[jbedard]

So the only possible callback that might throw and not be caught is the last one, right?

I'm not sure actually. Can you think of any examples where a throwing function would be pushed onto processQueue?

However, would something like deferred.then(fn1).done(fn2) force fn1 to be synchronous in order to run fn2?

All the scenarios I've tried seem to be valid to. Some examples... (where fn1 always executes before fn2, fn3 before fn4 etc).

deferred.then(fn1).done(fn2)

var def = $.Deferred();
def.then(fn1).done(fn2);
def.resolve(-1);
$.when(5).then(fn3);

var def = $.Deferred();
def.then(fn2).done(fn3);
$.when(5).then(fn1);
def.resolve(-1);

var def = $.Deferred();
def.then(fn2).done(fn3);
def.resolve(-1);
$.when(5).done(fn1);

[dmethvin]

Sorry, I think the case I was looking for was deferred.then(fn1).notify(fn2) since I think that's the only time special comes into play? This definitely could use a @gibson042 look.

[jbedard]

The notify case is an issue, see 4bc074b

[gibson042]

This PR does introduce some sequence jumping. Note how callbacks follow an orderly progression with the current codebase, but the .thens get clustered and pulled forward by this PR. That's not a problem (and is in fact the whole point), but the other part is potentially a problem: that the mere introduction of a bound Deferred (i.e., constructed by a .then that returned a thenable) affects all Deferred callbacks, pulling forward to pseudo-synchronicity callbacks that previously waited for the next event loop in front of other callbacks.

I'm against this PR as-is, but in favor of the broader refactoring that was hoped for when the Deferred changes first landed, to abstract out Promises/A+ compatibility code for both Deferred#then and jQuery.when, and in particular to eliminate this synchronous call (which exists to catch bad then implementations that throw after invoking a callback) by incrementing maxDepth earlier in the resolution process.

[jbedard]

Would it make sense to revert this one line?

[gibson042]

No, because then things would probably fall out of spec. I'm instead advocating for broader changes that would eliminate the need for doing something different when depth > 0, at which point there won't be any queue jumping.

[gibson042]

Swallowing errors from progress callbacks is a problem. We just went through a big exercise with ready to prevent its analogous situation.

[jbedard]

This was mainly just to test this out. I'm thinking that exceptions from a progress callback should be handled the same as exceptions in a success/error callback (~20 lines down). If we're ok with that then I think these two methods wrapping mightThrow can be combined and the special check will go within that single wrapper. Basically special just means that mightThrow throwing does not reject the promise...

[gibson042]

So send them through exceptionHook? Yeah, that makes sense.

[mgol]

Closing & re-opening the PR to trigger the EasyCLA check...

[linux-foundation-easycla]

• ❌ - Jason Bedard The commit (3e8483a ,4bc074b3d98338743b87669a3fe6ad0ca01a09b9 ,a15233f2cfdb1c7e583cced4b5d47f1d57bd4486) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.