Opened 11 years ago

Closed 7 years ago

#7750 closed defect (fixed)

Deferred squelches errors

Reported by: Mike Wilcox Owned by: Mark Wubben
Priority: high Milestone: 1.8
Component: Core Version: 1.2beta
Keywords: deferred, squelch, error Cc: James Burke, alex, dylan, Kris Zyp
Blocked By: Blocking:

Description

Test case (after including dojo):

window.onload = function(){
	var def = new dojo.Deferred();
	def.addCallback(function(){
		console.log("deferred fired")
		a = b // should be undefined error
	})
	def.callback();	
}

a = b should throw an error, but the deferred keeps this from happening. This wasn't as noticeable before because there was a console.debug inside the try/catch of the _fire method.

This previous debug did lead to the questioning of why some important errors were not showing up styled as an error to bring attention to it. The error swallowing can happen well after the deferred had fired.

Note that adding an addErrback is a workaround, not a fix. My example is simple, but not inclusive. A successful callback can fire another function that has the error, which would also be squelched.

I recommend inserting:

throw new Error(err);

on line 401.

Note even my fix may be considered a workaround. I did not look through the code deeply enough to find if there is a more concrete solution.

Attachments (1)

Deferred_error_visibility.patch (978 bytes) - added by Kris Zyp 10 years ago.
Fix for making errors visible by default when no error handler is provided

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by Mike Wilcox

Update: As I went back to the code I was working on with the fix above implemented locally, I did in fact find my syntax error. However the error message was "already fired!", as deferred errors fall back on the last accessible error object. My fix may not be sufficient.

comment:2 Changed 11 years ago by James Burke

I'm not sure what the bug is here? The purpose of using a Deferred is to avoid errors propagating out and stopping JS execution?

If you use djConfig.isDebug = true, it will throw an error and stop JS execution, but not for djConfig.isDebug = false.

Also, not sure if this is interfering, but I suggest using dojo.addOnLoad instead of window.onload.

I put up two tests using r15345 from SVN (latest svn as of the creation of this test).

First one with isDebug = false (the default): http://jburke.dojotoolkit.org/temp/15345/deferred.html

with isDebug=true: http://jburke.dojotoolkit.org/temp/15345/deferredDebug.html

Interestingly, with Firefox 3.0.2 with Firebug 1.2.1, I actually get a console error in box cases. In Safari 3, I only get the error in the console for the isDebug=true (the expected behavior: the Firefox behavior is a bonus).

mwilcox, can you clarify what you expect to be happening? As far as I can tell, this is working as designed? Maybe the isDebug behavior switching not what you want? If so, what would you want or expect?

If you can respond asap, that would be appreciated if you consider this a blocker for 1.2 since we hope to do that release very soon (could even be today). I will likely mark this as invalid otherwise.

comment:3 Changed 11 years ago by Mike Wilcox

You're right. I happened to not have djConfig in either my working file, nor the test case file. isDebug=true made the error appear.

I almost set this ticket to invalid myself, but I do want to question squelching an error that has nothing to do with the deferred - remember, it was squelching an error in a function called from the deferred. If this is expected behavior, so be it but... it was certainly unexpected to me.

The window.onload was simply to use as few dojo methods as possible to isolate the case.

comment:4 Changed 11 years ago by James Burke

Milestone: 1.2future
Owner: changed from anonymous to James Burke
Priority: highestnormal
severity: criticalnormal

OK, great.

On the Deferred behavior of calling the error callbacks being called if the deferred.callback() is called: maybe Alex can chime in here: I was under the impression that dojo.Deferred is a port from the mochikit/twisted deferred. I assume this behavior would be in their code too?

I am not sure how much we need to follow their model vs. maybe allowing errors in the callback to exception out instead of catching them.

I'll put this as a future milestone, in case someone more versed in Deferred and its origins/design goals can comment on it.

Changed 10 years ago by Kris Zyp

Fix for making errors visible by default when no error handler is provided

comment:5 Changed 10 years ago by Kris Zyp

Cc: Kris Zyp added

The Deferred mechanism is designed to wait for error callbacks to be added for errors handling, but squelching errors by default (when no error handler is provided) is atrocious for usability, and very frequently frustrates users that can't find error messages for the errors. Attached is a patch to provide some sanity for such situations.

comment:6 Changed 8 years ago by Adam Peller

Owner: changed from James Burke to Kris Zyp
Status: newassigned

comment:7 Changed 8 years ago by Kris Zyp

I believe we completely rewrote Deferred.js since this bug report. However, I am not completely happy with the error handling of the current implementation. We are definitely reporting errors (so technically we have address this bug), but I'd kind of like to have some non-default setting that would prevent errors from being caught at all so that you can easily break on them with a debugger set to break on uncaught errors. Anyone have any opinions on that?

comment:8 Changed 8 years ago by ben hockey

+1 for something that better facilitates break on error

comment:9 Changed 8 years ago by bill

Milestone: future1.8
Owner: changed from Kris Zyp to Mark Wubben

+1 from me too. I thought we had a long mailing list discussion about this at one point, but can't find it now.

Probably this ticket should go to Mark Wubben since he's doing the new Deferred implementation?

comment:10 Changed 8 years ago by Mark Wubben

I'm not sure what the error should be here. I frequently throw errors in my promised-based code since it's a separate communication channel, these shouldn't be logged as Dojo does currently. Then again if they're not logged they remain hidden.

Q has a .end() on promise-chains that will rethrow asynchronously if an error value reaches it. That's probably the best solution but still requires you to think about your promise chains quite carefully.

comment:11 Changed 8 years ago by Mark Wubben

Perhaps tracing will be a solution? See the diff at <https://github.com/promised-io/core/commit/bf219d1982110571cb933aaa82dd3ad3e6f03f4c>.

Rather than rethrowing errors you can set up more advanced tracing logic.

comment:12 Changed 7 years ago by Mark Wubben

Resolution: fixed
Status: assignedclosed

Tracing has been implemented with the new Promise API in [28265], see also #14615.

Note: See TracTickets for help on using tickets.