Opened 12 years ago
Closed 12 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: | [email protected]… |
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)
Change History (21)
Changed 12 years ago by
Attachment: | deferred.patch added |
---|
comment:1 Changed 12 years ago by
Patch with fix and test added. Email is [email protected]… in case it isn't obvious from the cc field
comment:2 Changed 12 years ago by
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 12 years ago by
CLA filed. As for the version, I have only tested in 1.5, but yeah, it's still happening.
comment:4 Changed 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:6 Changed 12 years ago by
Milestone: | tbd → 1.6 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Unfortunately after [22982] dijit/tests/layout/ContentPane.html is getting 1 failure (tested on FF3.6/win), in onDownloadError.
comment:7 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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
orerrorCallback
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 (hereerrback
is in fact the same method asreject
, so see above for promise behavior) - F. When a
callback
orerrback
throws an error - G. When a
callback
orerrback
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 12 years ago by
Attachment: | deferred-deprecated-behavior-fixes.patch added |
---|
comment:10 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
Ok, I'm pretty sure I'm +1 for markwubben's patch, but here's the timeline of my thoughts:
- Returning a value from an error handler was not bringing Deferred back into the success chain, so I submitted the first patch
- 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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Got it. I was not even thinking about older versions. Pardon my ignorance.
comment:17 Changed 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Fix + test