Search jQuery

jQuery Forum

Deferred and resolve

in Developing jQuery Core • 12 years ago

Sorry guys, but there are problems how errors are handled while resolving an deferred.
So I will try to explain those, and to propose some possible solutions.

My English is not specially good, so I will try to be as concise as possible.

Running this test, you will get "A ! B D" as result.
The last line is not evaluated because resolve() throws an error (do not catch the thrown one).
For that reason, "C" is never displayed in the result.
Maybe this is expected, so I will not necessarily consider this as a problem.

"D" is the one called asynchronously. Removing it, you will get "A !". See the test.
So the "B" is missing!

I you look at the resolveWith() code, you will see that the thrown "!" is not caught, so the "B" callback is not called at all. The reason why it appears in the first test, is because the async "D" call (10ms after) forced the rest of callbacks (B and D) to be called.

resolveWith: function( context, args ) {
    if ( !cancelled && !fired && !firing ) {
        firing = 1;
        try {
            while( callbacks[ 0 ] ) {
                callbacks.shift().apply( context, args );
            }
        }
        finally {
            fired = [ context, args ];
            firing = 0;
        }
    }
    return this;
},

Also, you will remark that there is an try..finally, and without the catch block the finally block will not be evaluated in IE6 and IE7.

Solution A

The first fix would be to add a catch block (IE6, IE7) and to ensure that fireing continues if interrupted:

resolveWith: function( context, args ) {
    if ( !cancelled && !fired && !firing ) {
        firing = 1;
        try {
            while( callbacks[ 0 ] ) {
                callbacks.shift().apply( context, args );
            }
        }
        catch ( e ) {
            jQuery.error( e );
        }
        finally {
            firing = 0;

            if ( callbacks[ 0 ] ) {
                deferred.resolveWith( context, args );
            } else {
                fired = [ context, args ];
            }
        }
    }
    return this;
},

In this way, the result of the second test would be "A ! B".

Solution B

It could be important to make resolving an safe operation.
In that way you could safely chain the resolve() and resolveWith(), but also safely resolve more deferreds:

dfd0.resolve();
dfd1.resolve();

In that case the result of the second test would be "A ! B C" and "A ! B C D" in the first one.

So to accomplish that we could simply replace the jQuery.error(e) of the solution A with a jQuery.reportError(e) that would simply output the error to console (if exists).

Solution C

Ignoring errors like jQuery does for events.

resolveWith: function( context, args ) {
    if ( !cancelled && !fired && !firing ) {
        firing = 1;

        while( callbacks[ 0 ] ) {
            callbacks.shift().apply( context, args );
        }

        fired = [ context, args ];
        firing = 0;
    }
    return this;
},

Replies(6)

jaubourg

Re: Deferred and resolve

12 years ago

This behaviour is known and the implementation was carefully designed to ensure it acts like it does.

An exception is the context of a resolve/then/done/fail call can only have two meanings:

1) It's unexpected (if it was, there would be a try/catch in the callback itself) in which case the caller has to be made aware of it and, thus, the exception has to be propagated,

2) It's expected, meaning that, since it's not handled by the callback itself, it means the callback wants to pause the execution of further callbacks (and that's a feature)

Case 1 is a no brainer: there is a problem in the code and it needs to be notified.

Case 2 can easily be dealt with by adding a try/catch around calls the coder knows may want to halt execution: http://jsfiddle.net/uqkE6/1/

You can even loop and call done with no argument to be sure to fire all the callbacks and log in the process if you want to: http://jsfiddle.net/D4jvh/2/

Even more crazy, you can throw an actual promise and use it to determine when to continue to execute: http://jsfiddle.net/vnF3V/

As for the catch/finally bug in IE6-7 I wasn't aware of it but are you sure it hasn't been fixed in the latest versions (they made maintenance ones not so long ago iirc or am I completely off-base here?).

rkatic

Re: Re: Deferred and resolve

12 years ago

Please, let me do a simplistic comparison of solutions:

Solution B: "exception-proof" resolving.
Solution C: not "exception-proof" resolving.
Current solution: not "exception-proof" resolving, but it may act as it is, if there is (or will be) an empty done() or an done(fn) where fn will not throw exceptions...

I am wandering if this "crazy resume feature" is perhaps too crazy.

You can not simply relay on done() for (not) resuming resolving. In case of an lazy done(), it will be resumed anyway, even if you don't want it.

With the solution A, I was trying to make that "resume feature" less "creazy" and more predictable. However, it would require a fix: in case of an exception, resolving have to be resumed asynchronously, after the exception is (not) handled. Something like:

resolveWith: function( context, args ) {
   if ( !cancelled && !fired && !firing ) {
        firing = 1;
        try {
            while( callbacks[ 0 ] ) {
                callbacks.shift().apply( context, args );
            }
        }
        catch ( e ) {
            jQuery.error( e );
        }
        finally {
            if ( callbacks[ 0 ] ) {
                setTimeout(function() {
                    firing = 0;
                    deferred.resolveWith( context, args );
                }, 1);
            } else {
                fired = [ context, args ];
                firing = 0;
            }
        }
   }
   return this;
},

Now, resolving is always resumed, but how it is worse then "maybe wil resume" behavior?

Probably I am missing something here..

Regarding the try/finally bug, even if it was fixed with an update, I am not sure if we can ignore not fixed ones. If there is IE6, it is likely not updated.

jaubourg

Re: Re: Deferred and resolve

12 years ago

Robert, you're transforming a synchronous behaviour into an asynchronous one and make it impossible to deal with further exceptions and break the resolve contract.

With your solution:

try {

dfd.resolve();

} catch( e ) {

// I have the first exception, what am I supposed to do now?

// in 1 ms, the other callbacks will be fired so:

// 1) I won't be able to catch other exceptions

// 2) I can call resolveWith here and change the context and args values

dfd.resolveWith( otherContext, otherArgs ); // BOOM!

}

Like I said in my first answer to you: it's no accident that current implementation is acting as it does.

With current implementation, IF, and ONLY IF, the calling code expects the exception as a halt signal, can it catch it, do whatever it needs to do and call done() to continue.

Stopping execution and propagating the exception while providing a mean to resume it is the only approach that makes sense in all cases.

Regarding try/finally, I'll have to do some tests in IE6 and IE7 latest. I'd hate it if we had to add a catch block as this will break debugger trace in most browsers.

rkatic

Re: Re: Deferred and resolve

12 years ago

Robert, you're transforming a synchronous behaviour into an asynchronous one and make it impossible to deal with further exceptions and break the resolve contract.

I am not sure what "resolve contract" means for you, but the current solution is already transforming a synchronous behaviour into an asynchronous one (for example, take my first test where "B" is outputed asynchronously), so I suppose that a contract is something more, and if there is an explanation of your idea of such thing on the web, I would like to read it.

try {
   dfd.resolve();
   } catch( e ) {
   // I have the first exception, what am I supposed to do now?
   // in 1 ms, the other callbacks will be fired so:
   // 1) I won't be able to catch other exceptions
   // 2) I can call resolveWith here and change the context and args values
   dfd.resolveWith( otherContext, otherArgs ); // BOOM!
   }

1) Well, in my opinion, exceptions thrown by the consumer should be handled by callbacks in the first place. Wrapping done(fn) calls in try/catch blocks would catch exceptions of who-know-which callback anyway.

2) No, because in the meantime the deferred will be in the fiering state (I fixed it immediately after posting it). Perhaps I should implement the solution A in an more simple way:

resolveWith: function( context, args ) {
    if ( !cancelled && !fired && !firing ) {
        firing = 1;
        try {
            while( callbacks[ 0 ] ) {
                callbacks.shift().apply( context, args );
            }
        }
        catch ( e ) {
            jQuery.error( e );
        }
        finally {
            firing = 0;
            fired = [ context, args ];

            if ( callbacks[ 0 ] ) {
                setTimeout( deferred.done, 1 );
            }
        }
    }
    return this;
},

This one resumes using done() instead of staying in fiering state.

Stopping execution and propagating the exception while providing a mean to resume it is the only approach that makes sense in all cases.

What about dfd.cancel() to not resume?

You can even loop and call done with no argument to be sure to fire all the callbacks and log in the process if you want to: http://jsfiddle.net/D4jvh/2/

http://jsfiddle.net/D4jvh/3/

rkatic

Re: Re: Deferred and resolve

12 years ago

Ok, I can understand that it is important the ability to resume (or not) resolving after catching exceptions.
The problem with the current solution is that without handling exceptions, the resume feature by done() easily induces unpredictable behaviors, making debugging considerably harder.

You are not understating the problem or you do not consider it a big deal or I am missing something. If so, I would like to have an explanation.

I can understand (and agree) that my previous solutions are not ideal (A and B because resuming is performed even after not caught exceptions; C because it removes the resume feature).
Hoverer, it doesn't mean that there is not an better solution, and I think I have one (hm, lets call it Solution D).

To handle exceptions, the producer has to override/wrap the resolveWith() function (your examples was not enough because not handling next done() calls). See example: http://jsfiddle.net/D4jvh/6/.

Now, that works, having the precaution to always call cancel() or done(), because otherwise the behavior will be unpredictable (because of the "auto resume feature") but it is not a big deal. The real problem, is when the exception is not handled at all (as I said).

To resolve both, we can make resuming an explicit operation, introducing an resume() method, and remaining in the firing state until that method is not called.
But it is not all! We now can remove the try/finally construct completely, avoiding the need to insert the catch block to avoid the IE6/7 bug. So the solution D would be something like https://gist.github.com/828650 and the previous example would be replaced with this one: http://jsfiddle.net/D4jvh/7/.

Note how in this one we don't need to call cancel(), that can be important in cases where there is no such method (Deferred).

Now, with the new _Deferred, the first test would output "A !" (http://jsfiddle.net/2Mbfn/3/), where resuming is halted even in case of an asynchronous done().

If there is no objections, I would like to fill some related bug reports and to submit a patch + pull request.

TL DR; Resuming the resolving should be explicit.