Opened 10 years ago

Closed 6 years ago

#8467 closed defect (wontfix)

dojo.xhr() is not atomic

Reported by: Mark Wubben Owned by: Bryan Forbes
Priority: high Milestone: future
Component: IO Version: 1.2.3
Keywords: Cc: alex
Blocked By: Blocking:


I found a bug in dojo._base.xhr(). Before the method returns, it calls dojo._ioWatch(), which at the end of its method invokes _watchInFlight(). If at this moment there are transports in flight that are finished, their callbacks will be executed.

I had difficulties reproducing this issue outside our system, but the scenario that triggered the bug for me was a username box that, after each key press, did a server call to check the uniqueness of the username. (And yes, that is a bit aggressive, but hey.)

I've attached a patch which changes dojo._ioWatch() to only call _watchInFlight() directly if its actually handling a synchronous request. In fact, the comment accompanying _watchInFlight() mentions that it is used for handling synchronous requests, it's just that it also handles asynchronous requests.

Attachments (1)

xhr.js.diff (973 bytes) - added by Mark Wubben 10 years ago.
Patch for XHR

Download all attachments as: .zip

Change History (9)

Changed 10 years ago by Mark Wubben

Attachment: xhr.js.diff added

Patch for XHR

comment:1 Changed 10 years ago by James Burke

Milestone: 1.3future
Priority: highlow

I don't think this fixes the core issue: some async callbacks may be fired if they were in flight and the sync xhr path triggers the immediate watchInFlight. To fix this more completely, sync xhrs should not be put in the _inFlight list and just have the results checked immediately.

This is a side effect of the inFlight list that has been in use by Dojo for a long time. Pushing this to future since we're trying to wrap up 1.3 changes and this will be a larger undertaking. I was hoping to revisit for Dojo 2.0, to see if we still need the queue.

The queue helps for having a common timeout check and to avoid having to attach callbacks on individual XHR objects, but has caused issue with allowing specific xhr callback errors stopping the whole queue without a try/catch block, which complicates debugging. And as this bug indicates, sync xhr handling needs some tweaking. Would be good to talk over with Alex about the pros/cons of the queue, and other reasons it is useful.

comment:2 Changed 10 years ago by James Burke

Priority: lownormal

comment:3 Changed 10 years ago by Mark Wubben

Fair points. But, as it seems the core issue won't be fixed before 2.0 and a rethink, perhaps the patch could be applied to alleviate the symptoms I described? It won't fix synchronous XHR, but does prevent asynchronous XHR from being handled while making a new request.

comment:4 Changed 10 years ago by James Burke

(In [16473]) Refs #8467: minimize xhr callbacks happening in the middle of an execution thread when starting a new XHR call. Still needs more work for sync requests, so leaving ticket open for a future milestone to address sync case.

comment:5 Changed 10 years ago by James Burke

Mark: good point, we should minimize side effects as best we can. I committed a code change, using a slightly different patch than the patch you used. Please give a holler if you have problems with it.

comment:6 Changed 10 years ago by Mark Wubben

Looks great, thanks.

comment:7 Changed 7 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:8 Changed 6 years ago by Kitson Kelly

Resolution: wontfix
Status: newclosed

I suspect dojo/request deals with most of these scenarios, ultimately we should close this ticket and open a new one if there is a kernel of something to be addressed.

Note: See TracTickets for help on using tickets.