Skip to content

Fix bug where requests with processors would not check disk cache for original image data #846

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

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

Conversation

JValldejuli
Copy link

TaskFetchOriginalData will now check the disk cache for the original image data before making a network request.

… original image data

`TaskFetchOriginalData` will now check the disk cache for the original image data before making a network request.
@JValldejuli
Copy link
Author

To fix #837. Please let me know if you'd like me to take a different approach. Thank you!

@JValldejuli
Copy link
Author

Closing because I think I found a better solution.

@JValldejuli JValldejuli reopened this Apr 10, 2025
@JValldejuli
Copy link
Author

Reopening for now, realized the other approach has some drawbacks.

@JValldejuli
Copy link
Author

JValldejuli commented Apr 12, 2025

The change that I've actually started using in my own app is to allow the pipeline delegate to provide unique custom keys for both the memory and disk caches. The setup I have is only disk cache the original image data (so key is just URL). The memory cache keys continue to be a function of both URL and thumbnail options to leverage already in-memory downsampled images. I also have a custom image decoder powered by UIImageReader that can generate thumbnail sized variants given the original image data, hence disk caching only the original image data. With this configuration, I don't need the change in this PR but I wonder if it's a good thing to keep anyways.

The overall thought is that disk caching should only have unique keys for steps that are post-decode. thumbnail options on the other hand are a decode-time operation in which case they can simply share disk cache entries with requests that have no thumbnail options.

@kean
Copy link
Owner

kean commented Apr 17, 2025

Hey, thanks for raising the issue and for the extended details. I'm working on redesigning and simplifying the tasks for Nuke 13 – I'll address this as part of the rework.

@@ -29,6 +29,10 @@ final class TaskFetchOriginalData: AsyncPipelineTask<(Data, URLResponse?)>, @unc
return
}

if let data = pipeline.cache.cachedData(for: makeSanitizedRequest()) {

Choose a reason for hiding this comment

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

Would this be better done in the TaskLoadData since the returnCacheDataDontLoad would block this.

Copy link
Author

Choose a reason for hiding this comment

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

It looks like TaskLoadData only backs loadData calls so I don't think so.

I don't think returnCacheDataDontLoad blocking this is necessarily bad since one could argue returnCacheDataDontLoad should return an exact match in the cache for a given request, not cached original data that still needs to go through processors/thumbnail decoding. I don't feel super strongly about it though.

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.

3 participants