Opened 12 years ago

Closed 6 years ago

#3814 closed defect (invalid)

Errors in sync xhr are squelched

Reported by: guest Owned by: Bryan Forbes
Priority: high Milestone: 2.0
Component: IO Version: 0.9
Keywords: needsreview Cc: sjmiles
Blocked By: Blocking:

Description (last modified by Adam Peller)

If the xhrPost test in dojo ests\_basexhr.html is modified to a sync and an error happens while handling the request, the error should not be squelched. I should be able to catch the error in my code and progress accordingly.

function xhrPost(t){
	var d = new doh.Deferred();
	var td = dojo.xhrPost({
		sync: true,
		url: "xhr.html?foo=bar", // self
		content: { color: "blue"},
		handle: function(res, ioArgs){
			throw "test";
			if((dojo._isDocumentOk(ioArgs.xhr))||
				(ioArgs.xhr.status == 405)
			){
				d.callback(true);
			}else{
				d.errback(false);
			}
		}
	});
	alert("should not see this!");
	// t.t(td instanceof dojo.Deferred);
	return d;
},

Change History (16)

comment:1 Changed 12 years ago by James Burke

Milestone: 0.9
Priority: highestnormal
severity: blockernormal

comment:2 Changed 12 years ago by James Burke

Milestone: 0.91.0

I don't think we should have a separate error path for sync vs. sync calls. That is more for the user of the Dojo IO calls to try to learn. I would rather give them one story that fits all cases (define an error handler or detect errors in your handle function).

If we can work out a decent way to throw errors for async calls, then we can reconsider this. One of the other committers, Scott Miles has asked for this type of behavior (a throw instead of routing through the Deferred error path).

The problem in the async and sync paths is that throwing an error will stop the Dojo code from checking other inflight IO calls. Scott suggested maybe throwing the exception in a setTimeout, but I believe that we might lose the stack trace in that case, so I'm not sure if that would work out.

I'm moving this for consideration in 1.0, if we can get a workable solution for all cases that will not break the inflight checks. Feel free to post suggestions to this ticket.

comment:3 Changed 12 years ago by James Burke

Cc: sjmiles added
Milestone: 1.00.9
Owner: changed from James Burke to alex

For reference, Scott suggested using a timeout to call the success callback. So in _base/xhr.js, in the _watchInFlight function, instead of doing this:

tif.resHandle(dfd);

do something like this:

window.setTimeout(function(){ tif.resHandle(dfd);}, 1);

Using a setTimeout might work for the async case, but I would like to have a unified behavior for async and sync. Since we call the sync callbacks as part of the inflight checks, that would be a problem.

I suppose we could not add the deferred object to the inflight queue by checking if the io args had a sync: true property, and then call the normal sort of checks we do in the inflight array immediately on the sync deferred before we leave the current function.

Hmm, and using a setTimeout for each async success callback seems to negate one of the benefits for the inflight queue: avoiding a bunch of individual timers. At least the setTimeout doesn't fire until the IO operation is deemed to be "finished", so that saves a little bit of work for the timers.

Ugh, but if we go with the setTimeout path, then I still don't think you will get the error thrown because the Deferred object will want to catch any exception that occurs in your success callback. So we would have to reconsider the usage of Deferred.

I'm fine with removing the dependency on Deferreds for the IO operations, but allow a way to wrap an IO call in a deferred (but not as part of base). That would allow us to remove Deferred from base, saving a little bit on the size of base. But that is going to be a little bit of work to adjust all the IO transports to that approach.

I'm going to pass this to Alex for consideration. If we are going to do any work in this area, then I think we should do it for 0.9, so flipping the milestone back to 0.9.

comment:4 Changed 12 years ago by alex

I guess I don't understand why this is a bug. There's an "error" handler which you can set. That we squelch in order to be able to service it is not something that needs "fixing", IMO.

Or do I completely misunderstand the issue?

comment:5 Changed 12 years ago by guest

The benefit of a sync request is that the code below the request waits till the request is complete before it executes. Right? Below is some pseudo code describing the situation I am trying to solve. If the error is not propagated, how will this work?

function foo() {
    try {
        //perform sync xhr
        ...
        //use response to fill screen with information
        ...
    } catch (e) {
        //handle error
        ...
    }
}

comment:6 Changed 12 years ago by James Burke

I think the larger issue is that it can be confusing that an exceptional case in your callback function logic triggers a call to your error handler. I don't think the contract was very clear: people may be assuming the error callback is only for handling actual transport errors, and not application-level errors with an actual callback. Maybe stating that the Deferred error path will be followed for application callback errors might help that situation.

I think this was actually worse in 0.4.3 where the error was wrapped in another error mentioning something like "inflight error: " + error.toString() (effectively). This mean losing some of the original error. I think the Deferred path in 0.9 is better though, in that I believe it preserves the original error better.

To speak to guest's question, you handle the error in a "error" callback, a function that is passed to the XHR call (or inside the "handle" callback function). You can see examples in the xhr test page (or look at the original code quote in this ticket to see an example using "handle").

comment:7 Changed 12 years ago by guest

Maybe I'm the one that is confused, but in a sync case, I would expect the above code to work. We have many code paths in my current project that look like my above example. I had created my own wrapper for io.bind, so now updating it to xhr will not change any other code. The workaround to get what I want follows using a closure, but anyone else using a sycn xhr request need to know that they will not receive errors in their calling code if this is not changed.

judo.remoting.execute = function (method, params, onSuccess, options) {
	if (onSuccess !== null && typeof onSuccess == "object") {
		options = onSuccess;
		onSuccess = null;
	}
	var requestArgs = {
			methodName: method,
			params: dojo.toJson(params)
		};
	var sync = typeof(onSuccess) != 'function';
	var returnValue = null;
	var syncError;
	var bindArgs = {
		sync: sync,
		url: options.url,
		content: requestArgs,
		error: function(err, ioArgs){
				console.error(err);
				syncError = err;
				return err;
		},
		load: function(data, ioArgs){
			var response = dojo.fromJson(data);
			if (response.ResponseCode == 200) {
				console.debug("remoting data: " + data);
				if (sync) {
					returnValue = response.ReturnValue;
				} else {
					onSuccess(response.ReturnValue);
				}
				if (options.deferred) {
					options.deferred.callback();
				}
			} else {
				if (options.raiseError) { 
					throw response.ErrorDescription;
				}
			}
		}
	};
	dojo.xhrPost(bindArgs);
	if (sync) {
		if (syncError) {
			throw syncError;
		} else {
			return returnValue;
		}
	}
};

schallm

(How can I have my comments and tickets under my name? I sign in under the guest account, is this not the correct way anymore?)

comment:8 Changed 12 years ago by dylan

Milestone: 1.11.2

comment:9 Changed 11 years ago by Adam Peller

Description: modified (diff)
Owner: changed from alex to James Burke

comment:10 Changed 11 years ago by James Burke

Milestone: 1.22.0

This seems to be another aspect of the xhr module catching exceptions in order to continue the inflight queue inspection. While this is nice for production robustness, it seems generally not to be the expected behavior.

For Dojo 2.0, we can consider breaking API behavior to get this to work. It seems like the more useful case is if by default the exception catching does not happen, but have a config option to turn it on (vs. today's behavior where exception catching is the default, and only option). Also perhaps it can be a per-xhr call option.

So moving this to consideration in 2.0 when we can really clean this up.

comment:11 Changed 8 years ago by ben hockey

Keywords: needsreview added

comment:12 Changed 8 years ago by bill

Owner: changed from James Burke to Bryan Forbes

Bulk change to reassign IO tickets to Bryan, since he is working on new dojo/request module. Some of these tickets should probably be closed as already fixed, invalid, or wontfix.

comment:13 Changed 7 years ago by dylan

Bryan, is this an issue with dojo/request, or can it be closed out?

comment:14 Changed 6 years ago by jmorrin

When using dojo/request if you specify a sync xhr request and there is an error the deferred is properly rejected. I think this issue should be closed as won't fix.

comment:15 Changed 6 years ago by bpayton

@jmorrin, I think I see what you mean but disagree. When making a synchronous call, it is natural to expect that any errors that occur will be thrown or returned explicitly, not wrapped in an asynchronous package. It is true that with Dojo promises, resolve and reject handlers can be called in the same turn as the call to then, but with Promises/A+, the handlers must be called in a turn following the call to then. If Dojo promises are made Promises/A+-compliant, an error from a synchronous request would only be accessible asynchronously.

That said, maybe this should just be a caveat to using dojo/request/xhr's sync option: dojo/request always returns a promise regardless. I'm not sure how common or desirable it is to make synchronous XHR requests anyway.

What do you think?

comment:16 Changed 6 years ago by Bryan Forbes

Resolution: invalid
Status: newclosed

Since dojo/request always returns a promise, all error handling is done within the promise mechanism which handles try/catch with .then()/.otherwise().

Note: See TracTickets for help on using tickets.