Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add loop for re-trying failed tiles [Take 3] #2238

Merged
merged 5 commits into from Jan 24, 2023

Conversation

Ughuuu
Copy link
Contributor

@Ughuuu Ughuuu commented Nov 20, 2022

Based on #1627
From @paaddyy who originally wrote it:

I faced some issues with OpenSeadragon in my current project when tiles failed loading when the server is too busy. I missed a functionality to reload the failed tiles and after some research I only found an answer that it isn't possible yet to reload specific tiles.
So I extended the ImageLoader and ImageJob class a bit. If the ImageLoader notices within completeJob that a request failed and the job didn't run more than 3 times, it will re-run the job after a timeout of 2500ms. After 3 times failing it will be handled the normal way.
Do you think it's a good solution?

I also know the "tile-load-failed"-event and the xhr object within the event but i wasn't able to write a suitable solution with that object 馃槃

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for taking this on! The cleanup looks good. I think maybe that's all that's necessary... What do you think? Have you had a chance to play with it in action?

src/openseadragon.js Show resolved Hide resolved
@iangilman
Copy link
Member

@Ughuuu Any word on finishing this up?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 23, 2023

Sorry for the delay. Will try it soon and let you know of my findings. I wanted to use this as was using Cloudflare to store images and it would error rather a lot, then changed CDN to bunny and now it doesnt error a lot. As that worked now didn't give it that much importance, but now came back to this to test it properly.
Will update here once I test it better.

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 23, 2023

Tried it now locally and it doesn't quite work. Added logging and the issue is that job.image comes as undefined not null what the code looks for. I could either make the if !job.image, or add the extra check || job.image === undefined (which is what I currently did and will update this PR with, but can also update with other solution if you let me know what).

@Ughuuu Ughuuu marked this pull request as ready for review January 23, 2023 21:10
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 23, 2023

With that fix tried it locally and it works. Set tileRetryDelay to 3 and used a tileset where all tiles were missing from my computer and returning 404(and started a webserver locally).
Tile 0,0 was loaded, failed, after 2.5 seconds(that is default for wait before retry) was tried again, then again(4 times). Then, I zoomed in a little bit, and about 10 requests started, then after 2.5 seconds 10 more, and so on 4 times again.
I have not tried it with any real case example, and my only fear is that the request would get cached, even though if it fails I would assume it does not.

@Ughuuu Ughuuu requested a review from iangilman January 23, 2023 21:13
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jan 23, 2023

Ready to be merged from my side.

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you for testing it, cleaning it up, and documenting it. I'm glad to have this in!

@iangilman iangilman merged commit 6f7e8c7 into openseadragon:master Jan 24, 2023
iangilman added a commit that referenced this pull request Jan 26, 2023
msalsbery added a commit that referenced this pull request Feb 1, 2023
* master: (276 commits)
  Changelog for #2280 and #2238
  remove trailing space
  fix problem with click precision on ReferenceStrip
  Changelog for #2273
  Also add documentation for tileRetryDelay
  try fix with check for null and undefined
  fix build error
  Add tileRetryMax documentation.
  Revert async support and event breaking support in EventSource.
  Changelog for #2276
  add box-sizing property to the navigator display region
  Implement support for async function and promise type recognition with $.type. Add $.Promise proxy. Implement support for promises in EventSource. Implement ability to abort events as well as prioritize events.
  Changelog for #2270
  issues/2192 fix.
  Starting 4.0.1
  Version 4.0.0
  JSDoc fixes
  Changelog for #2256
  Changelog for #2249
  removed polling vs resizeviewer option from demo
  ...
@Titan21
Copy link

Titan21 commented Feb 1, 2023

Hi, I'm very interested in this change and am excited to see retry-functionality coming to OpenSeadragon.
Testing with the changes in this PR, I was able to see the tile loader retry, however my viewport wasn't updating and despite fetching the tile successfully after the 1st retry it kept on re-trying.

image

In the screenshot you can see tile 2-1-0.jpg getting a 404 (via 302 redirect) on the first attempt, but then loading successfully on retries 1 - 5.
However, despite successfully loading the file, the viewport is never updated (even after scrolling in- and out).
I'm hoping something's just wrong with my setup, but I wasn't able to make it work.

@iangilman
Copy link
Member

iangilman commented Feb 2, 2023

@Titan21 Thank you for the report! What settings are you using to engage this feature? And which branch are you building from? The master branch here?

@Ughuuu What do you think? Any thoughts on why this might be happening?

@Aiosa
Copy link
Contributor

Aiosa commented Feb 3, 2023

Just a note, this could be implemented outside the ImageJob class by re-defining the download procedure in TileSource class, without changing OSD. Just fyi, not sure if it would be a good design decision.

@iangilman
Copy link
Member

iangilman commented Feb 6, 2023

@Aiosa Good point. Now that this has been implemented this way, though, I figure we might as well run with it for now.

@Aiosa
Copy link
Contributor

Aiosa commented Feb 7, 2023

Actually the ImageJob has a timeout so that would get in the way... unless you want to have shared time span for all the tries (which could be a good idea since 30 secs that are available I think should be enough). Just wondering what does this patch mean wrt. timeous now, retry count 3 means potential timeout 90secons?

@iangilman
Copy link
Member

Yes, I believe that's what it means. I think that makes sense; I would think it would be odd to have retries and then force them all into the same timeout. Also, the way it's implemented, it doesn't try again immediately, but waits until all of the other tiles have been loaded (at least I think so).

@becklocey-churchofjesuschrist

Thank you for adding this. Just installed v4.1.0 and the tile retry is working great.

openseadragon-retry-works-great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants