🌐 AI搜索 & 代理 主页
Skip to content

Deferred: invoke processes in async groups instead of individually #3228

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jbedard wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Deferred: invoke processes in async groups instead of individually #3228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e8483a
Deferred: invoke processes in async groups instead of individually
jbedard Jul 7, 2016
4bc074b
Deferred: ignore errors thrown in progress callbacks
jbedard Jul 19, 2016
a15233f
Deferred: specify array-splice length argument required in ES5
jbedard Jul 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions src/deferred.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ function adoptValue( value, resolve, reject ) {
}
}

var processQueue = [];
var processQueueTimeout;

jQuery.extend( {

Deferred: function( func ) {
Expand Down Expand Up @@ -183,8 +186,13 @@ jQuery.extend( {
},

// Only normal processors (resolve) catch and reject exceptions
// Exceptions in other cases should not effect the promise result
process = special ?
mightThrow :
function() {
try {
mightThrow();
} catch ( e ) {}
Copy link
Member

@gibson042 gibson042 Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jbedard jbedard Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@gibson042 gibson042 Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} :
function() {
try {
mightThrow();
Expand Down Expand Up @@ -212,20 +220,26 @@ jQuery.extend( {
}
};

processQueue.push( process );

// Support: Promises/A+ section 2.3.3.3.1
// https://promisesaplus.com/#point-57
// Re-resolve promises immediately to dodge false rejection from
// subsequent errors
if ( depth ) {
process();
deferredTick();
Copy link
Contributor Author

@jbedard jbedard Jul 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@dmethvin dmethvin Jul 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jbedard jbedard Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

@dmethvin dmethvin Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jbedard jbedard Jul 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notify case is an issue, see 4bc074b

Copy link
Member

@gibson042 gibson042 Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jbedard jbedard Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to revert this one line?

Copy link
Member

@gibson042 gibson042 Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {

// Call an optional hook to record the stack, in case of exception
// since it's otherwise lost when execution goes async
if ( jQuery.Deferred.getStackHook ) {
process.stackTrace = jQuery.Deferred.getStackHook();
}
window.setTimeout( process );

// Ensure processing has been scheduled
if ( !processQueueTimeout ) {
processQueueTimeout = window.setTimeout( deferredTick );
}
}
};
}
Expand Down Expand Up @@ -385,5 +399,12 @@ jQuery.extend( {
}
} );

function deferredTick() {
processQueueTimeout = undefined;
jQuery.each( processQueue.splice( 0, processQueue.length ), function( _, process ) {
process();
} );
}

return jQuery;
} );
22 changes: 22 additions & 0 deletions test/unit/deferred.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,28 @@ QUnit.test( "jQuery.Deferred.then - deferred (progress)", function( assert ) {
} );
} );

QUnit.test( "jQuery.Deferred.then - deferred (progress) exceptions", function( assert ) {

assert.expect( 1 );

var piped1, piped2,
defer = jQuery.Deferred(),
done = assert.async();

piped1 = defer.then( null, null, function() {
throw new Error( "You cant see me" );
} );

piped2 = defer.then( null, null, function( n ) {
assert.strictEqual( n, 42, "Errors in previous notifiers don't stop other notify callbacks" );
} );

piped2.then( done );

defer.notify( 42 );
defer.resolve();
} );

QUnit.test( "[PIPE ONLY] jQuery.Deferred.pipe - deferred (progress)", function( assert ) {

assert.expect( 3 );
Expand Down