Add an AJAX concurrency limiter for tile source requests #2242
Conversation
There was a problem hiding this comment.
Good idea! And yes, I don't think we have anything like that yet.
What you've implemented so far isn't really a viewer's tile source loader limit as it is an OSD global AJAX limit. This will affect other things besides tile sources (for instance, loading tiles via AJAX). I could see that being useful as well... I guess the question is, should we rename the feature and have it function as is? Or, should we keep the name and refactor it to actually be a limit on a viewer's tile source loading?
As you note in the code, we don't necessarily have a good mechanism for setting OSD global values... I suppose we could add some sort of OpenSeadragon.ajaxLoaderLimit or something, though I'm concerned that would be confusing to users as it would be the only one like that.
What do you think?
|
I think renaming the feature to be more generic makes a lot of sense! Something just like Yeah I'm also not sure what the best way to set this value would be, having it be a member of the viewer would be nice because it lines up with all of the existing configuration options. But having it at the viewer level could still get us into trouble with the browser limits in the case that there are multiple viewers on the same page; from the perspective of making OSD as a whole not run up against browser limits (regardless of how many viewers are instantiated) it does make sense to make a global option, but we might have to invent a new way to set it 🤔 I'm willing to put in the work to implement it if we decide on a good way forward! Although this is my first time approaching the source so I'm not too familiar with the style or structure yet |
|
I agree that having it global makes sense, and maybe it's about time we introduce global options somehow. I think I like moving towards having functions for getting and setting properties, so maybe it would be something like You seem to be doing a fine job of fitting into the source style/structure! |
|
@ChiliJohnson Are you interested in continuing work on this? |
|
Hello, yes I'm still interested! Sorry I was out on vacation for a couple weeks there but am back now and able to push this forward. I've been running this branch in production for the last few weeks without issue (churning through hundreds of thousands of AJAX requests) so I've at least convinced myself that the core logic works; now I just have to refactor it into something prettier |
|
@ChiliJohnson Wonderful! There's no rush... Just making sure you haven't walked away :) |
|
I've just pushed some updates based on the feedback! Still not sure if this is exactly the right approach to global settings but it's at least less of a draft now so I converted this to a real PR |
| } | ||
| }; | ||
|
|
||
| $.AjaxQueue.numIncompleteRequests++; |
There was a problem hiding this comment.
This value is manipulated but never used. Maybe we don't need it? I suppose it could be nice to have for debugging...
| $.AjaxQueue.numIncompleteRequests--; | ||
|
|
||
| if ( | ||
| $.AjaxQueue.numActiveRequests === $.settings.ajaxLoaderLimit && |
There was a problem hiding this comment.
I think my paranoia would be happier if this was <=, just in case.
|
|
||
| $.AjaxQueue.numIncompleteRequests++; | ||
|
|
||
| if ($.AjaxQueue.numActiveRequests === $.settings.ajaxLoaderLimit) { |
There was a problem hiding this comment.
I think my paranoia would be happier if this was >=, just in case.
This is a proof-of-concept for a new viewer option which attempts to address #2241
For very large sessions it's easy to hit a browser's concurrent network requests limit, causing it to cancel a bunch of tile source ajax requests, it would be nice to have an option similar to
imageLoaderLimitto avoid this case.I'm opening this as a draft now; I added this feature without much care for style or the existing project structure, but thought it would be a good jumping off point for discussion about how to properly implement this!
This is a quick-and-dirty addition based on this gist: https://gist.github.com/Terrance/158a4c436baa64c4324803467844b00f