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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Multiple requests on same URL would fail #2356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MadsBogeskov
Copy link
Contributor

If you have multiple simultaneous requests using Kingfisher that hits the same endpoint the task management system would fail before this. Simply put, because the tasks map, uses the URL as the key it wouldn't be able to store the different requests.

However Kingfisher does seem to handle this case in task(for task: URLSessionTask) -> SessionDataTask? by looking at both the URL and the task id. So I changed the key of the tasks map to include both for lookup.

Before this change Kingfisher would leak the continuation it creates in downloadImage(with:options) in ÌmageDownloader.

@onevcat
Copy link
Owner

onevcat commented Feb 4, 2025

Hi, @MadsBogeskov

Thank you for this PR.

I believe Kingfisher is handling this correctly by storing the callbacks from different requests in the callbacksStore (code). This behavior, where multiple requests share the same URL, is also tested here. By doing this, Kingfisher ensures that only one data download task is initiated for the same URL request, optimizing data transfer. Once the download is complete, it triggers all the registered callbacks.

Can you verify this behavior? Feel free to let me know if my understanding is wrong and this PR is actually necessary!

@MadsBogeskov
Copy link
Contributor Author

Hi @onevcat - Thanks for the quick response. I'll take a look. There is an issue, as I have a set of unit tests that are blocking, as in never returning to the callsite, because the tasks have disappeared, which my above PR fixed for our app at least. Plus we were often getting continuation leaks because of the same issue, so something is up (at least in the app I am working on ;) )

I will take a further look into what's going on!

@onevcat
Copy link
Owner

onevcat commented Feb 5, 2025

Thanks for the declaration!

You remind me a fix in 8.1.2 (https://github.com/onevcat/Kingfisher/releases/tag/8.1.2). If you are using an older version, maybe it worths to upgrade and see if that fixes.

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.

2 participants