Errors in sync xhr are squelched

[guest]

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;
},

[jburke]

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.

[jburke]

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.

[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?

[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
        ...
    }
}

[jburke]

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").

[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?)