Opened 9 years ago

Closed 9 years ago

#11840 closed defect (fixed)

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

Reported by: mbulman Owned by: Kris Zyp
Priority: high Milestone: 1.6
Component: General Version: 1.5
Keywords: deferred errback Cc: mbulman@…
Blocked By: Blocking:

Description

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

Attachments (3)

deferred.patch (1.6 KB) - added by mbulman 9 years ago.
Fix + test
deferred--patch-fix.patch (1.3 KB) - added by mbulman 9 years ago.
Here is what appears to be a proper fix for handling error objects being returned instead of thrown
deferred-deprecated-behavior-fixes.patch (3.6 KB) - added by Mark Wubben 9 years ago.

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by mbulman

Attachment: deferred.patch added

Fix + test

comment:1 Changed 9 years ago by mbulman

Patch with fix and test added. Email is mbulman@… in case it isn't obvious from the cc field

comment:2 Changed 9 years ago by bill

Owner: changed from anonymous to Kris Zyp
Summary: Successful result from Deferred errback doesn't move to resolve chain[patch] [no cla] Successful result from Deferred errback doesn't move to resolve chain

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?

comment:3 Changed 9 years ago by mbulman

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

comment:4 Changed 9 years ago by bill

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

Thanks!

comment:5 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

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

comment:6 Changed 9 years ago by bill

Milestone: tbd1.6
Resolution: fixed
Status: closedreopened

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

comment:7 Changed 9 years ago by 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.

Changed 9 years ago by mbulman

Attachment: deferred--patch-fix.patch added

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

comment:8 Changed 9 years ago by 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?

comment:9 Changed 9 years ago by 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.

Changed 9 years ago by Mark Wubben

comment:10 Changed 9 years ago by 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.

comment:11 Changed 9 years ago by 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.

comment:12 Changed 9 years ago by 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

comment:13 Changed 9 years ago by 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.

comment:14 Changed 9 years ago by 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.

comment:15 Changed 9 years ago by 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.

comment:16 Changed 9 years ago by mbulman

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

comment:17 Changed 9 years ago by 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].

comment:18 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.