abort xmlhttprequest in Tile.WFS

[crschmidt]

xmlhttprequest should be aborted in Tile.WFS. Requires reworking OpenLayers.loadURL to return the request somehow, so the tile can have a handle on it.

[pgiraud]

I took a look at this and this seems pretty simple. Please note that the attached patch includes the patch proposed for #1170.

With that in mind, the only things that changed are :

• loadUrl returns the created request,
• Tile.WFS now has a "request" member,
• request transport is aborted when the tile is destroyed.

Patch includes a new test. The test_Tile_WFS_requestSuccess test is now incorporated in test_99_Tile_WFS_destroy as I think it doesn't need to be separated.

[tschaub]

abort wfs request on tile destroy

[tschaub]

I updated this patch and gave it a quick review. Will commit if someone else says it's good. I think this is a good idea in general (returning a reference to the request from loadURL). Tests pass in FF.

[crschmidt]

Looks good.

[tschaub]

(In [5539]) Abort XMLHttpRequest on tile.destroy for WFS. The loadURL function now returns a request object. Thanks pgiraud for the fix. r=crschmidt (closes #964)

[euzuro]

this is an excellent patch!

could we also perhaps extend the use of this aborting to the draw() function in the if (this.isLoading) conditional?

it might even make sense to rework this so that the "request" parameter doubles as the 'isLoading' variable -- we could just set it to null in the requestSuccess function.

my thinking here is that this would help to speed things up in the case where the user hits the zoom-in button four times in a row -- the first three zoom-in's requests can all be cancelled so that we can concentrate on the last set. no?

i'm still a little rusty from holidays, maybe i'm missing something. thoughts?

[elemoine]

Replying to euzuro:

this is an excellent patch! could we also perhaps extend the use of this aborting to the draw() function in the if (this.isLoading) conditional? it might even make sense to rework this so that the "request" parameter doubles as the 'isLoading' variable -- we could just set it to null in the requestSuccess function. my thinking here is that this would help to speed things up in the case where the user hits the zoom-in button four times in a row -- the first three zoom-in's requests can all be cancelled so that we can concentrate on the last set. no? i'm still a little rusty from holidays, maybe i'm missing something. thoughts?

Welcome back Erik ;-)

At a first glance, I'd agree with you. If you cook up a patch, I'll review it.

[euzuro]

patch to abort our requests on multiple reloads (see my previous comment for explanation). since isLoading is a variable defined at the generic Tile level, it doesn't make sense to replace it with the 'request' property (as I had mentioned in my comment). tests pass in ff/ie6

[elemoine]

Replying to euzuro:

Thanks for the patch Erik. Comment: this.request is never set to null in the code so the test added in loadFeaturesForRegion will always pass (except the very fist time loadFeaturesForRegion is entered).

[euzuro]

Replying to elemoine:

Replying to euzuro: Thanks for the patch Erik. Comment: this.request is never set to null in the code so the test added in loadFeaturesForRegion will always pass (except the very fist time loadFeaturesForRegion is entered).

.... so is there some other way you would recommend doing this? That just seemed like the most simple solution to me -- I'm totally open to suggestions :-)

[elemoine]

Replying to euzuro:

Replying to elemoine:

Replying to euzuro: Thanks for the patch Erik. Comment: this.request is never set to null in the code so the test added in loadFeaturesForRegion will always pass (except the very fist time loadFeaturesForRegion is entered).

.... so is there some other way you would recommend doing this? That just seemed like the most simple solution to me -- I'm totally open to suggestions :-)

How about: your patch + setting this.request to null in requestSuccess. You actually mentioned this in your first in comment:7.

[euzuro]

reworked to set request to null in requestSuccess() .... and also in the tile's destroy()

[euzuro]

Replying to elemoine:

Replying to euzuro:

Replying to elemoine:

Replying to euzuro: Thanks for the patch Erik. Comment: this.request is never set to null in the code so the test added in loadFeaturesForRegion will always pass (except the very fist time loadFeaturesForRegion is entered).

.... so is there some other way you would recommend doing this? That just seemed like the most simple solution to me -- I'm totally open to suggestions :-)

How about: your patch + setting this.request to null in requestSuccess. You actually mentioned this in your first in comment:7.

good call. i've updated that. and also updated the wfs tile's destroy() to set it to null as well. thx!

[elemoine]

I'm ok with the patch. One small question: why do you have this comment //this.request.destroy(); in the patch? Couldn't it be removed?

[euzuro]

Replying to elemoine:

I'm ok with the patch. One small question: why do you have this comment //this.request.destroy(); in the patch? Couldn't it be removed?

it could be removed... it's just a subtle reminder to us all that we should create a proper destroy() function for the request object, in an effort to have a more glorious, memory-safe application.

[elemoine]

this.request references an object that was created by XHR. So we'll never be able to create a destroy method for it. Am I wrong?

[euzuro]

Replying to elemoine:

this.request references an object that was created by XHR. So we'll never be able to create a destroy method for it. Am I wrong?

as i understand it, 'request' is an object of type 'OpenLayers.Ajax.Request'... which is perhaps only a thin wrapper around the XHR object, but it is an object of a class which we define. Therefore we could/should therefore define a destroy() method for it.

i'd be happy to cook up that patch, but it'll have to be in a couple of days, i just got some new work onto my plate.

[elemoine]

Ok Erik, sorry for the confusion. Anyway, your patch looks good to me, please go ahead and commit it.

[crschmidt]

Committed in r5777:

See #964 - not only should we cancel ajax requests when we destroy the tile, but

also when we initiate a new response. which is to say that when we instruct the tile to run a new request, we can discard the old one(s). that is what this patch does (as well as cleaning up memory in the destroy). Note that I have added this.request.destroy(); call, but commented out. this is a nod to future development/improvement of the OpenLayers.Ajax.Base and OpenLayers.Ajax.Request class to give it its own destroy() method. Just for fun I'll go ahead and open a separate ticket for that: #1277. Thanks elemoine for the reviews and the good dialogue to finishing up this patch.