OpenLayers OpenLayers

Ticket #964 (closed task: fixed)

Opened 1 year ago

Last modified 9 months ago

abort xmlhttprequest in Tile.WFS

Reported by: crschmidt Assigned to: tschaub
Priority: minor Milestone: 2.6 Release
Component: Tile.WFS Version: 2.4
Keywords: Cc:
State: Complete

Description

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.

Attachments

wfs_request_abort.diff (20.8 kB) - added by pgiraud on 11/23/07 08:19:28.
wfsAbort.patch (3.0 kB) - added by tschaub on 12/20/07 04:48:00.
abort wfs request on tile destroy
requestAbort.patch (1.8 kB) - added by euzuro on 01/10/08 12:40:15.
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
requestAbort.2.patch (3.2 kB) - added by euzuro on 01/14/08 12:22:37.
reworked to set request to null in requestSuccess() .... and also in the tile's destroy()

Change History

11/23/07 08:11:00 changed by 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.

11/23/07 08:19:28 changed by pgiraud

  • attachment wfs_request_abort.diff added.

12/20/07 04:48:00 changed by tschaub

  • attachment wfsAbort.patch added.

abort wfs request on tile destroy

12/20/07 04:50:12 changed by tschaub

  • owner set to tschaub.
  • state set to Review.

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.

12/20/07 04:50:24 changed by tschaub

  • component changed from general to Tile.WFS.

12/20/07 08:02:15 changed by crschmidt

  • state changed from Review to Commit.

Looks good.

12/20/07 12:28:51 changed by tschaub

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(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)

(follow-up: ↓ 7 ) 01/09/08 13:24:39 changed by 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?

(in reply to: ↑ 6 ) 01/10/08 06:19:18 changed by 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.

01/10/08 12:40:15 changed by euzuro

  • attachment requestAbort.patch added.

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

(follow-up: ↓ 9 ) 01/10/08 12:58:03 changed by euzuro

  • status changed from closed to reopened.
  • state changed from Complete to Review.
  • resolution deleted.

(in reply to: ↑ 8 ; follow-up: ↓ 10 ) 01/11/08 03:51:31 changed by 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).

(in reply to: ↑ 9 ; follow-up: ↓ 11 ) 01/11/08 13:10:09 changed by 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 :-)

(in reply to: ↑ 10 ; follow-up: ↓ 12 ) 01/13/08 09:25:04 changed by 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.

01/14/08 12:22:37 changed by euzuro

  • attachment requestAbort.2.patch added.

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

(in reply to: ↑ 11 ) 01/14/08 12:23:57 changed by 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!

(follow-up: ↓ 14 ) 01/14/08 16:01:40 changed by 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?

(in reply to: ↑ 13 ) 01/14/08 16:21:24 changed by 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.

(follow-up: ↓ 16 ) 01/14/08 16:25:43 changed by 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?

(in reply to: ↑ 15 ) 01/14/08 18:46:54 changed by 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.

01/15/08 00:40:17 changed by elemoine

  • state changed from Review to Commit.

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

01/16/08 12:43:42 changed by 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.

01/16/08 12:43:48 changed by crschmidt

  • status changed from reopened to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.