Opened 11 years ago

Closed 11 years ago

#7981 closed defect (fixed)

isDebug: true breaks deferred callbacks

Reported by: Matt Sgarlata Owned by: James Burke
Priority: high Milestone: 1.3
Component: General Version: 1.2.0
Keywords: Cc: development-staff@…
Blocked By: Blocking:

Description

Setting isDebug: true breaks the dojo.Deferred errback functionality. When isDebug is set to true, it changes the behavior of dojo.Deferreds such that they raise exceptions rather than execute the errback. In my opinion, setting a debug flag like this should not change the behavior of a method, especially one that is specifically designed with errors in mind. When djConfig is setup such that isDebug: true, instead of raising an exception the error should be logged and the errback should be executed.

Attachments (1)

dojoBug7981Test.html (666 bytes) - added by Matt Sgarlata 11 years ago.
Test case

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by Matt Sgarlata

Attachment: dojoBug7981Test.html added

Test case

comment:1 Changed 11 years ago by Matt Sgarlata

I forgot to mention, this may or may not be related to bug 7750, but I have trouble following the conversation in that report.

In this test case I am using dojo.Deferred through the xhrPost method. The URL I am retrieving returns a response that has some bogus HTML code in it. If you click on the text test button, the test correctly alerts "callback called". If you click on the javascript test there is a JavaScript? error. Instead, the errback function should be called (that's the whole point of having errbacks in the first place!)

Setting isDebug: false causes the errback to be executed correctly, so this is only a problem when isDebug: true.

comment:2 Changed 11 years ago by Adam Peller

Owner: changed from anonymous to James Burke

comment:3 Changed 11 years ago by James Burke

Resolution: wontfix
Status: newclosed

I made this change in Dojo 1.2 because we have gotten quite a few complaints that it is hard to trace errors when they happen via a debugger. This stemmed from the use of Deferred calls where the errbacks were being called on dojo.xhr errors. Most of the time during development, you are hitting lots of error conditions and you want quick access to the failure point usually in the debugger.

So that was the motivation for the change: throw the error if isDebug = true. I would prefer to have the same behavior for both isDebug true and false, but in the greater interest of allowing easier development overall, the change was made.

I am open to alternative ways to solve this though: making sure you can get access to the original error, with stack traces for the debugger. This is complicated since Deferreds are not a concept you necessarily need to know to use the dojo.xhr methods.

I considered using a timer to later throw the error out of the current call flow, but that seemed to lose the stack trace/call sequence for debugging the issue.

Any ideas would be greatly appreciated. It was not one of my favorite things to change. I am going to close this as wontfix for now, but please feel free to reopen if you have additional insight into the issue.

comment:4 Changed 11 years ago by Matt Sgarlata

I understand your points and they are good ones. However, I have given this a lot of thought and I must respectfully disagree with your conclusions.

After a thorough review of the error handling code in my application, I've determined I'm not going to be able to use callbacks and errbacks at all because of this. I understand the motivation is to increase usability, but from my perspective the usability is decreased to zero. The feature does not function according to its specification with the debug flag turned on. Again, I strongly feel logging the error is the appropriate course of action.

A solution like this just wouldn't fly in other contexts. Imagine if you had logging turned on in Tomcat that when an error occurred the Tomcat server just shut down because that would give you a better error message. I guess that could be useful for someone, but if you're trying to test and make sure you display an error message correctly to the user, you are up a creek.

There are lots of places where dojo could report errors better and I think it's a great idea to improve error handling. This is one particular case that's different, because this code is specifically designed to work with errors. When an error occurs, the errback is supposed to be called.

comment:5 Changed 11 years ago by Juho Manninen

I second Matt's comments - this is somewhat unexpected behavior. James, you wrote in #6863: 'I can see using isDebug for this purpose to be a reasonable flag to use.' How about separating this into another one, like 'debugExceptions'?

comment:6 Changed 11 years ago by James Burke

I can see where using isDebug is probably not a good flag to key from -- isDebug really means "enableConsole". What if I moved the behavior change to use debugAtAllCosts instead? That flag has a dire sounding name and it already changes the behavior of the running code (by using the xdomain loader), so changing other behavior like throwing from a deferred should be OK in that case too. How does that sound?

I appreciate Matt's point of view. I think if the basic XHR code did not use Deferreds, but allowed you to bind a deferred to the xhr action as an option, this would be easier to solve/a non-issue: the xhr code would always throw, and if you decided to bind a deferred to the call, then the exception would go through the normal Deferred flow, and since you consciously chose to use a Deferred you would have to manage debugging errors.

However, it is possible to use Dojo Base now and never know about Deferreds, and in those use cases, particularly with the XHR/dojo.io.script/dojo.io.iframe calls, errors are silently eaten by the Deferreds. It make the toolkit very unapproachable.

I would like to look at the non/Deferred approach for basic IO operations for Dojo 2.0, but in the meantime, perhaps moving the behavior switch to debugAtAllCosts is more palatable?

comment:7 Changed 11 years ago by Matt Sgarlata

That sounds great. Thanks!

comment:8 Changed 11 years ago by Matt Sgarlata

Resolution: wontfix
Status: closedreopened

The status here got set to WONTFIX and it looks like we all forgot to reopen it. Per jburke's previous comment, the plan is to move the behavior switch to debugAtAllCosts

comment:9 Changed 11 years ago by James Burke

Milestone: tbd1.3

comment:10 Changed 11 years ago by James Burke

Resolution: fixed
Status: reopenedclosed

(In [16049]) Fixes #7981: move the throw-instead-of-catch behavior for deferreds and related to trigger off of debugAtAllCosts. \!strict

Note: See TracTickets for help on using tickets.