[patch] [cla] Successful result from Deferred errback doesn't move to resolve chain

[mbulman]

If an errback handler returns a successful result, the process chain is supposed to move to the resolved portion of the callback chain. This does not happen.

            var def = new dojo.Deferred();
            var retval = 'fail';
                
            def.addErrback(function() {
                return 'ignore error and throw this good string';
            }).addCallback(function() {
                throw new Error('error1');
            }).addErrback(function() {
                return 'ignore second error and make it good again';
            }).addCallback(function() {
                retval = 'succeed';
            });
           
            def.errback('');
            
            t.assertEqual('succeed', retval);

Browsers: all OS: all

[mbulman]

Fix + test

[mbulman]

Patch with fix and test added. Email is [email protected]… in case it isn't obvious from the cc field

[bill]

Hi, thanks for the patch+test, can you file a ​CLA?

Also, not that Deferred was rewriitten in 1.5, so I assume that your version field is accurate and this problem is still happening in 1.5?

[mbulman]

CLA filed. As for the version, I have only tested in 1.5, but yeah, it's still happening.

[bill]

Thanks!

[Kris Zyp]

(In [22982]) Correct issue with error deferred going to back to success, fixes #11840 !strict

[bill]

Unfortunately after [22982] dijit/tests/layout/ContentPane.html is getting 1 failure (tested on FF3.6/win), in onDownloadError.

[mbulman]

Yeah I found another case as well yesterday, and have what I think is a fix, just need to run all tests to verify. Should have something back today.

[mbulman]

Here is what appears to be a proper fix for handling error objects being returned instead of thrown

[mbulman]

I've added another patch that should fix the issues (and adds a test to verify). I ran all of the test suites, and the deferred changes don't seem to be making anything break. Although it looks like there are tests in src that always fail in some browsers, so the only way I could verify was to make a note of those and revert my changes and make sure they still broke. Is there something we can do about having broken tests in */trunk/ so that true regression testing can be done?

[Mark Wubben]

There are two different behaviors in the Deferred implementation:

• deprecated (pre-1.5) with addCallback/addErrback and friends
• CommonJS-style promise behavior

I don't know enough about the deprecated behavior so I'll defer to mbulman on that. I've compared promise behavior with the promised-io library by Kris. For the sake of this discussion, assume the original patch was not committed.

For promise behavior, the only time you can get into an isError state:

• A. When rejecting a deferred (with no or any value)
• B. When a resolvedCallback or errorCallback throws
• C. If an errorCallback does not return a result (i.e. you stay in the error state)

Conversely, you get out of of the error state:

• D. When an errorCallback returns any value (and doesn't throw)

This works as intended, and I've added test cases in my patch.

For the deprecated behavior – and again, this is based on mbulman's assumptions, not research into previous Dojo versions – you get into an error state:

• E. When errback'ing a deferred (here errback is in fact the same method as reject, so see above for promise behavior)
• F. When a callback or errback throws an error
• G. When a callback or errback returns an error

Conversely, you get out of the error state:

• H. When an errback returns any value (and doesn't throw)

Points G and H are not working and are what this ticket is about.

Again, I don't know the actual desired behavior here. However, when debugging a problem I ran into using Dojo code from around May, but with the latest Deferred, it does appear to be the case that returned Error objects from an errback keep the deferred in the error state (confirming G).

The initial commit partially fixes G, but not H. It introduces a bug in the following scenario, which may occur in the XHR code:

def.addCallbacks(function(){}, function(err){
	return err;
});
def.promise.then(
		function(){ retval = "fail"; },
		function(){ retval = "succeed"; });
def.errback(new Error);

The errback returns the error, which the Deferred implementation interprets as case D, so it calls resolve with the new result (which is still the same error object). Then, due to the patch, isError is set to false, and in the promise the resolvedCallback is invoked.

I've attached a patch that has test cases for all these scenarios and a better fix in Deferred. I've also taken the liberty to clean up mdulman's test function formatting a little for consistency with the rest of the tests.

[mbulman]

Is there a specific reason why returning an error in resolvedCallback would not put the chain into an error state? I am basing most of my assumptions on python twisted's deferred system (and what seems to be a fairly common standard across other systems), not on past dojo versions.

In the case of error state, why should there be any difference between the two Deferred implementations mentioned by markwubben?

Also, kzyp, I'd recommend reverting [22982] for the time being as it does introduce bugs.

[Kris Zyp]

Is there a reason why returning an error should put the deferred in an error state? Promises are intended to provide an async equivalent of sync flow, where normal return (regardless of value) -> success state and throwing (regardless of value) -> error state.

[mbulman]

Ok, I'm pretty sure I'm +1 for markwubben's patch, but here's the timeline of my thoughts:

1. Returning a value from an error handler was not bringing Deferred back into the success chain, so I submitted the first patch
2. I noticed that 404's were staying in the success chain because of what I _thought_ was an instance of Error being returned instead of thrown (see "return errHandler(..." in dojo._ioSetArgs), so I submitted patch 2.

As for returning instances of Error's being equivalent to throwing, I was basing that on #2 above combined with the way deferreds are handled in python twisted (allows for returning or throwing). I'm 100% content with throwing being the only mechanism to get into the error chain, but if returning an Error instance was ever supported, I see no reason to diverge from that in the rewrite (it's not a brand new idea, and having things work in different ways depending on the interface used would just cause confusion).

So assuming returning Error instances are either support nowhere, or everywhere, I'm good with it

[Mark Wubben]

If returning Errors worked with the old addCallback/addErrback behavior, it should still work in the implementation of that old behavior. My patch achieves that, without adding it for the new Promise behavior.

[mbulman]

Like I said above, I'm pretty sure it was a bad assumption on my part that returning Error's ever worked. Running returnErrorObject() from your patch verifies my bad assumption.

[Mark Wubben]

In Deferred 1.4.3, returning Error changes to the error state:

var func = function(){
	var ret = f(res);
	//If no response, then use previous response.
	if(typeof ret != "undefined"){
		res = ret;
	}
	fired = ((res instanceof Error) ? 1 : 0);
	// …
};

// later:
try{
	func.call(this);
}catch(err){
	fired = 1;
	res = err;
}

Returning the error sets res to the error and fired to 1. Same when the error (or rather, any object) is thrown: res is set to the error and fired to 1.

[mbulman]

Got it. I was not even thinking about older versions. Pardon my ignorance.

[Mark Wubben]

Kris, could we resolve this? AFAIK my patch restores 1.4 behavior without affecting 1.5 behavior. It also fixes the bug that was introduced in [22982].

[Kris Zyp]

(In [23081]) Applied mark wubben's fix for errback moving to resolve chain, fixes #11840 !strict