Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#10814 closed enhancement (fixed)

Improvements to dojo.Deferred

Reported by: kzyp Owned by: kzyp
Priority: high Milestone: 1.5
Component: General Version: 1.4.0
Keywords: Cc: elazutkin
Blocked by: Blocking:

Description (last modified by peller)

  • Provide side-effect free chaining
  • Provide consumable-only promises
  • Provide dojo.when for promise/normal value normalization

Attachments (9)

DeferredList.js (893 bytes) - added by kzyp 4 years ago.
fix to DeferredList to work with new Deferred
Deferred.js (3.7 KB) - added by kzyp 4 years ago.
Some bug fixes
Deferred.2.js (8.0 KB) - added by kzyp 4 years ago.
More unit tests
Deferred.js.patch (3.4 KB) - added by Mark Wubben 4 years ago.
Patches to Deferred
test_Deferred.js.patch (1.7 KB) - added by Mark Wubben 4 years ago.
Adds tests for new Deferred bugs
Deferred-Cancel-Leak.diff (935 bytes) - added by neonstalwart 4 years ago.
fixes leaking 'fired' and 'results' into global scope
Deferred-1.5-fixes.diff (1.5 KB) - added by markwubben 4 years ago.
dfd.html (2.5 KB) - added by neonstalwart 4 years ago.
Deferred-leak.patch (1.1 KB) - added by markwubben 4 years ago.

Download all attachments as: .zip

Change History (73)

comment:1 Changed 4 years ago by bill

Unfortunately the new Deferred hangs dijit/tests/test_Tree.html... the root nodes of each tree show with a loading icon, but none of the children show up.

Changed 4 years ago by kzyp

fix to DeferredList to work with new Deferred

comment:2 Changed 4 years ago by kzyp

I noticed the problem with the tree as well. This latest version fixes one problem, but I think there is still a problem with DeferredList (which has to be rewritten as it completely relied on the internals of dojo.Deferred).

Changed 4 years ago by kzyp

Some bug fixes

Changed 4 years ago by kzyp

More unit tests

comment:3 Changed 4 years ago by jburke

I would prefer not to see console.error used if dojo.config.deferredOnError was not defined. It means there could always be console.error logging showing up even for valid, caught error paths in production deployment. I prefer to see something like this in the reject function, but open to other ideas:

    (dojo.config.deferredOnError && dojo.config.deferredOnError(error));

and then at the bottom of the closure for the code, some block like so:

var cfg = dojo.config;
if (cfg.isDebug && !cfg.deferredOnError) {
  cfg.deferrredOnError = console.error;
}

might save some typing by setting up the var cfg and using that inside the reject function too.

comment:4 follow-up: Changed 4 years ago by kzyp

The point of this (or at least what Eugene as fighting for) was to make errors show up by default, and provide a means for suppressing them (for valid errors paths in prod code). This change would increase the code size and I am not sure I understand the advantage.

Anyway, I am going to commit the code, but leave this ticket open so we can continue to discuss the best path for error handling (or whatever other changes might be needed)

comment:5 Changed 4 years ago by kzyp

(In [21558]) dojo.Deferred rewrite, refs #10814 !strict

comment:6 in reply to: ↑ 4 Changed 4 years ago by elazutkin

Replying to kzyp:

The point of this (or at least what Eugene as fighting for) was to make errors show up by default, and provide a means for suppressing them (for valid errors paths in prod code). This change would increase the code size and I am not sure I understand the advantage.

How much more code?

Minified?

Gzipped?

What is your alternative? setTimeout()-based checks? Please explain how we should process errors in your view.

How is it better than everything else?

I put all questions on separate lines, so it will be easier for you to answer them, and harder to ignore. :-D

comment:7 Changed 4 years ago by kzyp

I was trying to defend your case for error visibility, Eugene, I thought you wanted us to default to console.error. James' change is probably a few dozen bytes. I don't really care how the errors are handled, all my stuff is checked in now, so please feel free to change Deferred.js to whatever you want.

comment:8 Changed 4 years ago by elazutkin

I see. I have no intentions to modify your code --- like I said before I like it. I think it has a right balance of the legacy support and new stuff.

But I will not let you go that easy :-) --- now you have to write a blog post or two explaining the change to users. Old stuff is still there, but now we have new exciting stuff, what are the benefits, and use cases. People should take advantage of the new stuff whenever is possible. Otherwise it'll be lost in obscurity.

I'll see if I can use new things in dojox.lang.async.

comment:9 Changed 4 years ago by kzyp

Absolutely! I wouldn't part all this work into dojo.Deferred and not blog/brag about it :).

comment:10 Changed 4 years ago by bill

Unfortunately [21558] is breaking dijit/tests/Tree.html and dijit/tests/layout/ContentPane.html. (The DOH tests return failure instead of returning success, as they do on [21557].) Can you take a look?

comment:11 follow-up: Changed 4 years ago by Mark Wubben

Kris, awesome code. Lots of clever tricks, especially using mutator for Dojo-style mutating Deferreds, and the subsequent code path in the callback handling.

Two issues I found though:

  • Line 325 (in SVN) doesn't check if promiseOrValue has an object value; this check is done on line 196, so this seems inconsistent.
  • dojo.Deferred().then().addCallback() will fail because the promise returned by .then() doesn't have addCallbacks(). This seems to be an oversight on line 241.

comment:12 Changed 4 years ago by kzyp

(In [21583]) Fixes dojo.when handling of falsy values, refs #10814 !strict

comment:13 in reply to: ↑ 11 Changed 4 years ago by kzyp

Replying to Mark Wubben:

Two issues I found though:

  • Line 325 (in SVN) doesn't check if promiseOrValue has an object value; this check is done on line 196, so this seems inconsistent.

Thank you, should be fixed now.

  • dojo.Deferred().then().addCallback() will fail because the promise returned by .then() doesn't have addCallbacks(). This seems to be an oversight on line 241.

The addCallback() method and friends are available on the Deferred objects for backwards compatibility, but are intentionally not implemented/available on plain consumer-only promises. This is to ensure that one can safely pass promises to consumers without worrying about their value being mutated (as addCallback does). The then() method and the "promise" property return these consumer-only promises, and these promises can be stripped down to only exposing the safe method (then()) without violating backwards compatibility because the previous API did not have a then() method and 'dojo.Deferred().then().addCallback()' wasn't possible before.

comment:14 Changed 4 years ago by Mark Wubben

OK, fair enough, but then why is then().addCallback a function, rather than undefined? If it's there I'd be inclined to use it.

comment:15 Changed 4 years ago by kzyp

(In [21589]) Make addCallback, addErrback, and addBoth only visible on Deferred (not promises), refs #10814 !strict

comment:16 Changed 4 years ago by Mark Wubben

There's a double resolve when dojo.xhr fails to successfully make the request. Easy to reproduce by running dojo.xhrGet({url:"http://google.com"}) (which will fail because it's cross-domain).

This gives:

Error: Unable to load http://google.com status:0
Uncaught Error: This deferred has already been resolved

comment:17 Changed 4 years ago by kzyp

There's a double resolve...

I can't seem to reproduce this.

comment:18 Changed 4 years ago by kzyp

(In [21591]) Provides the fired property for backwards compatibility
DeferredList handles empty array properly, refs #10814 !strict

comment:19 follow-ups: Changed 4 years ago by Mark Wubben

Okay, I did some more digging. The double resolve seems to be a Chrome issue, it does not occur in Firefox and Safari.

Here's my test page:

<script src="dojo/dojo/dojo.js"></script>
<script>
dojo.xhrGet({ url: "http://google.com" });
</script>

The double resolve only only occurs in Chrome, line 211 in Deferred.js throws TypeError: Illegal invocation:

(dojo.config.deferredOnError || console.error)(error);

This error is subsequently caught (_base/xhr.js line 679) after which the errback is invoked again, leading to a double resolve error.

This seems to boil down to how console.error is invoked as a standalone function, rather than bound to console. This also fails:

(undefined || console.log)("hi")

Whereas this works:

(undefined || function(){ console.log.apply(console, arguments); })("hi")

Another observation is that, with error logging enabled in Deferred by default, XHR requests now log errors twice.


There's also a bug with .fired. Handling an errback results in the .fired state being 0 because line 181 always calls listener.deferred.resolved, even if result or newResult is an error instance.

comment:20 in reply to: ↑ 19 Changed 4 years ago by elazutkin

Replying to Mark Wubben:

This seems to boil down to how console.error is invoked as a standalone function, rather than bound to console.

Heh, I expected this problem from IE8 and asked Kris twice if he tested it with IE. But the problem bit from the other side. :-)

The functional wrapper that you proposed should be tested in all browsers to see if there is any problem with apply(). Or we can use a simplified wrapper, which will work 100%:

(dojo.config.deferredOnError || function(x){ console.error(x); })(error);

Or something to this effect, e.g., assign the wrapper to a variable once, and use it without creating an inadvertent closure.

comment:21 Changed 4 years ago by kzyp

(In [21593]) Fixes console target |this| on invocation, refs #10814 !strict

comment:22 in reply to: ↑ 19 Changed 4 years ago by kzyp

Replying to Mark Wubben:

Another observation is that, with error logging enabled in Deferred by default, XHR requests now log errors twice.

Any objections to removing the error logging from XHR then?

There's also a bug with .fired. Handling an errback results in the .fired state being 0 because line 181 always calls listener.deferred.resolved, even if result or newResult is an error instance.

This seems correct to me. Line 181 affects the internal deferred for the returned promise, which should have .fired equal to 0 if the callback/errback successfully completed. Putting a deferred in error state should have fired equal to 1 to indicate the error, which it seems to:

var d = dojo.Deferred();
d.errback(new Error("test"))
d.fired

1

comment:23 follow-up: Changed 4 years ago by Mark Wubben

Regarding all these tricks to call console.error, including declaring an on the spot function, what's wrong with if(console.error){ console.error(error); }?

Regarding .fired, yes, in your example. Handling the errback changes the value though:

var d = dojo.Deferred(); d.addErrback(function(){}); d.errback(new Error); d.fired;
0

After firing a listener, listener.deferred.resolve(result) is always called. This is the success handler (even if isError == true) so .fired is set to 0.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 4 years ago by kzyp

Replying to Mark Wubben:

Regarding all these tricks to call console.error, including declaring an on the spot function, what's wrong with if(console.error){ console.error(error); }?

Users need to be able to suppress the error messages (when they have provided error handlers and know they don't want the messages to be displayed).

Regarding .fired, yes, in your example. Handling the errback changes the value though:

var d = dojo.Deferred(); d.addErrback(function(){}); d.errback(new Error); d.fired;
0

After firing a listener, listener.deferred.resolve(result) is always called. This is the success handler (even if isError == true) so .fired is set to 0.

OK, I see, if the function returns undefined the it is supposed to stay in error state, I guess. I'll get that fixed. I can't wait to be able to deprecate and ditch the mess that is addCallback :/.

comment:25 in reply to: ↑ 24 Changed 4 years ago by Mark Wubben

Replying to kzyp:

Regarding all these tricks to call console.error, including declaring an on the spot function, what's wrong with if(console.error){ console.error(error); }?

Users need to be able to suppress the error messages (when they have provided error handlers and know they don't want the messages to be displayed).

Agree, but if(console.error){ console.error(error); } doesn't stop that from happening. In fact, the current code:

function(x){ console.error(x); }

does not even check if console.error exists, so you'd need:

function(x){ console.error && console.error(x); }

At which point you might as well write:

if(dojo.config.deferredOnError){
  dojo.config.deferredOnError(error);
}else if(console.error){
  console.error(error);
}

Which would be a bit more verbose, but functionally equivalent to (dojo.config.deferredOnError || console.error)(error);.

Regarding .fired, yes, in your example. Handling the errback changes the value though:

var d = dojo.Deferred(); d.addErrback(function(){}); d.errback(new Error); d.fired;
0

After firing a listener, listener.deferred.resolve(result) is always called. This is the success handler (even if isError == true) so .fired is set to 0.

OK, I see, if the function returns undefined the it is supposed to stay in error state, I guess. I'll get that fixed. I can't wait to be able to deprecate and ditch the mess that is addCallback :/.

No it also goes wrong for d.addErrback(function(err){ return err; }). Fired shouldn't go to 0 if it's already in the error state, or simplifying it, if it's not == -1.

comment:26 Changed 4 years ago by Mark Wubben

By the way, the one thing I miss about addCallback is the automatic hitching.

comment:27 Changed 4 years ago by kzyp

(In [21692]) Make instances of dojo.Deferred each be an instanceof dojo.Deferred, and same for dojo.DeferredList.
Leaves error state unchanged when no value returned, refs #10814 !strict

comment:28 Changed 4 years ago by kzyp

(In [21859]) Treat cancellations as unlogged errors, provide results property, refs #10814 !strict

comment:29 Changed 4 years ago by Mark Wubben

There's a bug with canceling a 'derivative' promise:

var dfd1 = new dojo.Deferred;
var dfd2 = dfd1.then();
dfd2.cancel();
// >> Error: This deferred has already been resolved

Canceling dfd2 simply invokes dfd1.cancel, which results in dfd1 being rejected, and consequently dfd2 being rejected as well. At this point, dfd2's internal cancel method rejects dfd2 once more.

Best solution seems to be to check for finished again after invoking the canceller.

this.cancel = promise.cancel = function () {
	// summary:
	//		Cancels the asynchronous operation
	if(!finished){
		var error = canceller && canceller(this);
		if(!finished){
			if (!(error instanceof Error)) {
				error = new Error(error);
			}
			error.log = false;
			reject(error);
		}
	}
}

comment:30 Changed 4 years ago by Mark Wubben

Also, I believe the old Deferred allowed for a way to not catch exceptions thrown by callbacks. It'd be great to have this back for debugging purposes.

comment:31 Changed 4 years ago by Mark Wubben

Another observation, the deferred's methods aren't bound to the deferred, but will seemingly work fine if you call them in a different scope:

var dfd = new dojo.Deferred;
dfd.then(function(value){ console.log(value); });
var resolve = dfd.resolve;
resolve("foo"); // logs "foo"
window.results; // is ["foo", null]

This could lead to some hard to debug bugs.

comment:32 Changed 4 years ago by Mark Wubben

And 42 minutes later, turns out I'm running into a bug cause by the this issue:

dojo.xhr("GET", { url: "/" }).promise.cancel()

This fails because the XHR canceller tries to read ioArgs of the deferred being cancelled. However the object passed to the canceller is the promise object.

resolve/reject/cancel must use a variable reference to the this object, rather than the keyword.

Changed 4 years ago by Mark Wubben

Patches to Deferred

Changed 4 years ago by Mark Wubben

Adds tests for new Deferred bugs

comment:33 follow-up: Changed 4 years ago by Mark Wubben

Hey Kris, I've added test cases and fixes for the following bugs:

  • Cancelling new dojo.Deferred().then().cancel() throws an error
  • Support for dojo.config.debugAtAllCosts had been removed
  • Unbound invocations of dfd.cancel, dfd.resolve, dfd.reject affect the global object
  • Rejecting a deferred didn't add the error object in dfd.results (backwards compat)

Would be great to get these fixes in before the 1.5 beta 3 is cut.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 4 years ago by kzyp

Replying to Mark Wubben:

Hey Kris, I've added test cases and fixes for the following bugs:

  • Cancelling new dojo.Deferred().then().cancel() throws an error

This one is already been applied and committed, I believe.

  • Support for dojo.config.debugAtAllCosts had been removed

The attached patch for debugAtAllCosts is pretty big, I don't think we can incur that size increase on dojo base (or at least I don't think I can authorize this type of size increase), and it doesn't seem that critical since we are logging all errors by default now.

  • Unbound invocations of dfd.cancel, dfd.resolve, dfd.reject affect the global object

Unbound invocations of methods can affect the global object throughout Dojo (and in previous versions of dojo.Deferred). It is silly to increase the size of dojo.Deferred to protect against unbound invocations when everything else is still is affected by this. The solution is es5's strict mode, not more code.

  • Rejecting a deferred didn't add the error object in dfd.results (backwards compat)

I'll add this.

comment:35 Changed 4 years ago by kzyp

(In [22239]) Adds error as results property, refs #10814, !strict

comment:36 follow-up: Changed 4 years ago by kzyp

BTW, is there a way we can mark the "results" property as deprecated so we can remove it in the future? I really want to get this library smaller in the future.

comment:37 follow-up: Changed 4 years ago by jburke

For the debugAtAllCosts support, I have not been completely happy with how that has turned out. I was hoping that having the dojo.config.deferredOnError function hook would allow people who wanted to throw a chance to throw that way, like so:

dojo.config.deferredOnError = function (e) { throw e };

although I have not personally tested it yet.

Mark, if you have feedback on the usefulness of dojo.config.deferredOnError for handling the old debugAtAllCosts behavior (which is actually fairly new), please give a holler.

Kris, as for marking the results property as deprecated, you can probably mention it in the docs/inline comments, but seems unlikely that we will be able to drop it before something like a Dojo 2.0.

comment:38 in reply to: ↑ 36 Changed 4 years ago by markwubben

Replying to kzyp:

BTW, is there a way we can mark the "results" property as deprecated so we can remove it in the future? I really want to get this library smaller in the future.

If the plan is to deprecate addCallback/addErrback and associated variants, the deprecation of results could be included in that warning message.

comment:39 in reply to: ↑ 37 ; follow-up: Changed 4 years ago by markwubben

Replying to jburke:

For the debugAtAllCosts support, I have not been completely happy with how that has turned out. I was hoping that having the dojo.config.deferredOnError function hook would allow people who wanted to throw a chance to throw that way, like so:

dojo.config.deferredOnError = function (e) { throw e };

although I have not personally tested it yet.

Mark, if you have feedback on the usefulness of dojo.config.deferredOnError for handling the old debugAtAllCosts behavior (which is actually fairly new), please give a holler.

It gives a stack trace to the deferredOnError call, not the origin of the exception.

Error logging is not enough for debugging. I do get the considerations about file size, but I see no other option that the approach I took in the patch—or maintaining a "debuggable" Deferred in DojoX that is the same code except it doesn't catch exceptions in the callbacks.

comment:40 in reply to: ↑ 34 Changed 4 years ago by markwubben

Replying to kzyp:

  • Cancelling new dojo.Deferred().then().cancel() throws an error

This one is already been applied and committed, I believe.

Please see the cancelThenDerivative test case. It's not resolved yet.

  • Unbound invocations of dfd.cancel, dfd.resolve, dfd.reject affect the global object

Unbound invocations of methods can affect the global object throughout Dojo (and in previous versions of dojo.Deferred). It is silly to increase the size of dojo.Deferred to protect against unbound invocations when everything else is still is affected by this. The solution is es5's strict mode, not more code.

The actual bug (which isn't in the test cases) is with this construct:

var dfd = new dojo.Deferred(function(_dfd){ console.log(dfd === _dfd); /* false */ });
dfd.promise.cancel();

This occurs, for example, when you use:

dojo.xhr(…).promise.cancel();

The canceller is called with the promise object, not the Deferred instance. XHR expects the Deferred instance, can't find ioArgs and throws an error.

Also, regarding file size, right now this is referenced 5 times, at a cost of 20 characters. Adding a single character variable (assuming proper minification) to reference it can be done at a total cost of _=this or 6 characters, pus 5 times a reference to _, or a total of 11 characters. That'd be 9 less than the current code, fixing the dfd.promise.cancel issue and making it easier to pass resolve/reject as callback methods to other functions without having to hitch them.

comment:41 Changed 4 years ago by elazutkin

One possible solution to the size issue is to create a separate module, which extends/modifies Deferred. Just put this module in the core, or dojox (depending on how good it is). This way it can be used for debugging when needed, do not affect the normal build size, and can be used without modifications of the code, which depends on Deferred.

comment:42 Changed 4 years ago by neonstalwart

based on revision 22283 you can save a few bytes by removing var resolve = on line 192 and you can also remove var reject = on line 202 if you call this.reject on line 280. this will also stop reject leaking fired and results into the global scope (see attached test case).

Changed 4 years ago by neonstalwart

fixes leaking 'fired' and 'results' into global scope

comment:43 Changed 4 years ago by kzyp

Checked in a fix for the global leak (don't know why the revision doesn't show up here)

comment:44 Changed 4 years ago by markwubben

Kris, two problems (still) remain:

		function cancelPromiseValue(t){
			var cancelledDef;
			var def = new dojo.Deferred(function(_def){ cancelledDef = _def; });
			def.promise.cancel();
			t.is(def, cancelledDef);
		},

Because I call promise.cancel(), _def is the promise object rather than the deferred. Subsequently there's an error because the cancel method calls this.reject which doesn't exist, as this is the promise object.

Then there's this test case:

		function cancelThenDerivative(t){
			var def = new dojo.Deferred();
			var def2 = def.then();
			try{
				def2.cancel();
				t.t(true); // Didn't throw an error
			}catch(e){
				t.t(false);
			}
		},

When def is cancelled, the rejection is propagated to def2. However, in the cancel handling of def2 it's rejected once more. You need to test if the deferred was finished by its canceller.

A fix, with a _this variable reference rather than the keyword:

		var _this = this;
		this.cancel = promise.cancel = function () {
			// summary:
			//		Cancels the asynchronous operation
			if(!finished){
				var error = canceller && canceller(_this);
				if(!finished){
					if (!(error instanceof Error)) {
						error = new Error(error);
					}
					error.log = false;
					_this.reject(error);
				}
			}
		};

Apologies for not putting this in a patch, but you seem to have overlooked these issues when I did put them in a patch earlier.

I'll leave finding a solution that still requires .cancel() to be called on the promise/deferred itself (rather than unbound) to you.

comment:45 Changed 4 years ago by kzyp

(In [22333]) Fixes issue with wrong |this| for cancellation of promises, refs #10814 !strict

comment:46 Changed 4 years ago by peller

  • Description modified (diff)
  • Type changed from defect to enhancement

comment:47 Changed 4 years ago by markwubben

There's still two issues:

  • new Deferred(function(dfd){ }).promise.cancel() doesn't pass the deferred instance to the canceller
  • new Deferred().then().cancel() gives a "deferred has already been resolved" error.

See attachment for patch and test cases.

Changed 4 years ago by markwubben

comment:48 Changed 4 years ago by elazutkin

  • Cc elazutkin added

comment:49 Changed 4 years ago by markwubben

(In [22409]) Fixes issue with new Deferred(function(dfd){ }).promise.cancel() not passing the deferred instance to the canceller. Fixed new Deferred().then().cancel() resulting in a "deferred has already been resolved" error.

Added test cases for both, and for having the error in the results property ([22239]).

Refs #10814 !strict

comment:50 Changed 4 years ago by markwubben

  • Resolution set to fixed
  • Status changed from new to closed

Committed as per phiggins' and kzyp's approval in #dojo-meeting.

comment:51 Changed 4 years ago by neonstalwart

  • Resolution fixed deleted
  • Status changed from closed to reopened

found some more global leaking - see updated dfd.html test

Changed 4 years ago by neonstalwart

comment:52 Changed 4 years ago by neonstalwart

i should maybe mention that the first test 'dfd cancel' passes. it's the 2nd test 'dfd chain' which fails and leaks results and fired.

Changed 4 years ago by markwubben

comment:53 Changed 4 years ago by markwubben

I've narrowed it down to:

def.then(function(){ return def; });
def.resolve(true);

This bit inside notify() is the culprit:

if (newResult && typeof newResult.then === "function") {
	newResult.then(listener.deferred.resolve, listener.deferred.reject);
	continue;
}

listener.deferred.resolve and listener.deferred.reject are called directly, so they're bound to the global.

I've attached a patch with hitches the resolve and reject, it also adds a test case. Should I commit this?

P.S. This is another reason I'd like to remove the references to this and use a closure variable instead.

comment:54 follow-up: Changed 4 years ago by peller

Unless this is deemed critical, I suggest we push this off to 1.5.1. Could we open this and future Deferred issues as separate tickets?

comment:55 Changed 4 years ago by jburke

I would like to have any global leakage fixed, but I am at a conference at the moment and I have not studied the issue closely yet. If the global leakage is a result of bad code practice, that might make it not so critical.

comment:56 in reply to: ↑ 54 Changed 4 years ago by neonstalwart

the fix is simple. also, i figured since this ticket was to provide "Provide side-effect free chaining" i thought it was appropriate to reopen the ticket and i also consulted with kris before doing so but in the future i'll open new tickets. not a problem.

before:

newResult.then(listener.deferred.resolve, listener.deferred.reject);

after:

newResult.then(dojo.hitch(listener.deferred, 'resolve'), dojo.hitch(listener.deferred, 'reject'));

the cause of the leak is due to a simple oversight. resolve and reject just needed to be hitched to the right context.

it seems that results and fired which are leaked are only there for backwards compatability with the previous implementation of Deferreds so they don't affect the underlying logic of the promise-based implementation but will affect existing code which might have looked at results and fired on a Deferred instance. given that we're at RC3 and nobody's complained about results and fired being missing or broken on their Deferreds then there's probably not many people testing 1.5 who actually refer to those properties in their code.

my conclusion would be that it would be low risk to push it to 1.5.1 but it would be just as low risk to commit it now. would we have to do another rc if we committed it now? if that was the case then it isn't worth committing it now.

comment:57 Changed 4 years ago by peller

in theory we should make a new rc for any changes and this would delay the release. It may be appropriate to do so, but we should do it immediately. Please consult with jburke.

While it was completely reasonable to reopen this ticket, it's probably best to make new tickets going forward simply because we have a policy of keeping tickets short, also once we finish 1.5.0 we'll have multiple releases to track for the changesets.

comment:58 Changed 4 years ago by jburke

The global leakage is bad, made worse by their names "results" and "fired", seems like it would be easy for those to overwrite something bad the developer wanted in the global space.

I want to do this change before 1.5 is released. I judge it to be very low risk given that we avoid the globals. The only risk is for something that before did not seem like it was fired or had results now would have them, but I believe it would be a more correct state reflection.

I will commit the patch from markwubben since it has a test, but if for some reason we do not want it in for the final release, I am happy to revert it. But I believe it is the more correct change and avoids us clobbering other variables that may be important to the developer. I am OK not spinning an RC for this change (just have us do some tests, I will check our unit tests), but I understand if we want to do one for it.

comment:59 Changed 4 years ago by jburke

  • Resolution set to fixed
  • Status changed from reopened to closed

Global leaks fixed in [22467]

comment:60 Changed 3 years ago by bill

(In [23310]) fix spelling + jslint warnings, refs #10814 !strict

comment:61 in reply to: ↑ 39 Changed 3 years ago by reesd

Replying to markwubben:

Replying to jburke:

For the debugAtAllCosts support, I have not been completely happy with how that has turned out. I was hoping that having the dojo.config.deferredOnError function hook would allow people who wanted to throw a chance to throw that way, like so:

dojo.config.deferredOnError = function (e) { throw e };

although I have not personally tested it yet.

Mark, if you have feedback on the usefulness of dojo.config.deferredOnError for handling the old debugAtAllCosts behavior (which is actually fairly new), please give a holler.

It gives a stack trace to the deferredOnError call, not the origin of the exception.

Error logging is not enough for debugging. I do get the considerations about file size, but I see no other option that the approach I took in the patch—or maintaining a "debuggable" Deferred in DojoX that is the same code except it doesn't catch exceptions in the callbacks.

Was there any additional follow-up on this? I don't see it in another ticket and there is lots of context here, so I will follow-up here and we can move to another ticket if required.

It took me a while yesterday to figure out my error was being swallowed. Even the error in my error callback is swallowed. And without Firebug on or isDebug enabled it is silently swallowed. Even if this is the correct behavior, I would suggest it need to be documented. For now I have added a (const enabled) debugger line in my code - not my favorite workaround.

The other behavior I did not expect was that errors in the load callback would be caught by the error callback. My assumption would be that one or the other is called and then it's up to me to handle it from there. If that is correct behavior it probably needs to be documented.

Here is about what I was doing:

dojo.xhrGet({
	url: url,
	error: function(error, ioArgs) {
		throw error;
	},
	load: function(data, ioArgs) {
		throw "error example";
	}
});

PS, xhrGet example 4 also shows using addCallback even though it seems then() is now preferred (http://www.sitepen.com/blog/2010/05/03/robust-promises-with-dojo-deferred-1-5/).

PSS, Adding the following line as suggested above results in a "This deferred has already been resolved" error being thrown. dojo.config.deferredOnError also probably needs to be documented under Deferred.

dojo.config.deferredOnError = function (e) { throw e }; 

This is all with 1.5 on Firefox 3.6.13. Thanks, dave

comment:62 Changed 3 years ago by markwubben

Hi reesd, apologies for not replying here sooner, I've had this tab "open" for too long.

I think there are some high-level issues with promises in Dojo:

  • Unnecessary console.error() calls when you reject a deferred. These are annoying because rejecting deferreds can be an essential part of the program's control flow.
  • Swallowing of errors without raising exceptions

I'm looking to doing more work with promises in the next few weeks (working on changes to Kris' promised-io code), both for Node.js and a browser/Dojo runtime. I'll keep these things in mind for changes to dojo.Deferred as well.

Regarding your XHR example, I'd agree with your interpretation that the error callback is only in regards to errors in the request itself, not the load callback. I'm unfamiliar with historical behavior though, please if you could open a new ticket for that that'd be great.

comment:63 Changed 3 years ago by bill

In [26323]:

fix typos in example, thanks ykami, refs #10814 !strict

comment:64 Changed 2 years ago by bill

In [28068]:

Upgrade to newer Deferred syntax, using then() instead of addCallback()/addErrback(). Refs #10814 !strict.

Note: See TracTickets for help on using tickets.