Ticket #964 (closed task: fixed)

Opened 6 years ago

Last modified 5 years ago

abort xmlhttprequest in Tile.WFS

Reported by: crschmidt Owned by: 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 Download (20.8 KB) - added by pgiraud 5 years ago.
wfsAbort.patch Download (3.0 KB) - added by tschaub 5 years ago.
abort wfs request on tile destroy
requestAbort.patch Download (1.8 KB) - added by euzuro 5 years ago.
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 Download (3.2 KB) - added by euzuro 5 years ago.
reworked to set request to null in requestSuccess() .... and also in the tile's destroy()

Change History

  Changed 5 years ago 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.

Changed 5 years ago by pgiraud

Changed 5 years ago by tschaub

abort wfs request on tile destroy

  Changed 5 years ago 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.

  Changed 5 years ago by tschaub

  • component changed from general to Tile.WFS

  Changed 5 years ago by crschmidt

  • state changed from Review to Commit

Looks good.

  Changed 5 years ago 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   Changed 5 years ago 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   Changed 5 years ago 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.

Changed 5 years ago by 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

follow-up: ↓ 9   Changed 5 years ago by euzuro

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

in reply to: ↑ 8 ; follow-up: ↓ 10   Changed 5 years ago 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   Changed 5 years ago 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   Changed 5 years ago 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.

Changed 5 years ago by euzuro

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

in reply to: ↑ 11   Changed 5 years ago 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   Changed 5 years ago 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   Changed 5 years ago 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   Changed 5 years ago 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   Changed 5 years ago 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.

  Changed 5 years ago 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.

  Changed 5 years ago 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.

  Changed 5 years ago by crschmidt

  • status changed from reopened to closed
  • state changed from Commit to Complete
  • resolution set to fixed
Note: See TracTickets for help on using tickets.