Sdwebimage: Load image with option `SDWebImageRefreshCached` get error

[ufosky]

Issue Info

Info | Value |
-------------------------|-------------------------------------|
Platform Name | ios
Platform Version | all
SDWebImage Version | 4.1.0
Integration Method | cocoapods
Xcode Version | Xcode 8.3
Repro rate | all the time (100%)
Repro with our demo prj | Yes, just add little code.
Demo project link | none

Issue Description and Steps

Replace this line as the link below with code options:SDWebImageProgressiveDownload | SDWebImageRefreshCached, and run the demo. When first navigate to the detail view of an image, it will show as expect, but go back and do it again, no image show now.

https://github.com/rs/SDWebImage/blob/master/Examples/SDWebImage%20Demo/DetailViewController.m#L44

[ufosky]

Before I upgrade this library to 4.1.0, it's no problem.

[bpoplauschi]

@ufosky I tried your scenario, but I still get all the images. What is the error you see? Can you debug a little?

[ufosky]

When a image is display first time, the completed callback will be invoked twice.
The first invoke has no error and the image is set to the view.
The second invoke get an error like below and the image is nil, but the view is not changed.

Error Domain=NSURLErrorDomain Code=-1008 "resource unavailable" UserInfo={NSUnderlyingError=0x6080002481f0 {Error Domain=kCFErrorDomainCFNetwork Code=-1008 "(null)"}, NSErrorFailingURLStringKey=https://raw.githubusercontent.com/liyong03/YLGIFImage/master/YLGIFImageDemo/YLGIFImageDemo/joy.gif, NSErrorFailingURLKey=https://raw.githubusercontent.com/liyong03/YLGIFImage/master/YLGIFImageDemo/YLGIFImageDemo/joy.gif, NSLocalizedDescription=resource unavailable}

After first displaying, when show the same image again, the callback will get an error like this:

Error Domain=NSURLErrorDomain Code=-1100 "(null)"

[ufosky]

By the way, the progress callback block is invoked in a background thread, but do some UI operation in like this is not safe.
https://github.com/rs/SDWebImage/blob/master/Examples/SDWebImage%20Demo/DetailViewController.m#L47

[dreampiggy]

Reproduced. +1

[dreampiggy]

Maybe I found the reason. I don't know why but in SD 4.1, we change the NSURLRequestCachePolicy for download operation from NSURLRequestReloadIgnoringLocalCacheData to NSURLRequestReturnCacheDataDontLoad when the SDWebImageRefreshCached. This means if the NSURLCache can not find local cache, it will do not perform any HTTP request and directly failed. So finally NSURLSessionDelegate may get that "resource unavailable" error and call the top level block.

The code is here:

4.1: https://github.com/rs/SDWebImage/blob/4.1.0/SDWebImage/SDWebImageDownloader.m#L172
4.0: https://github.com/rs/SDWebImage/blob/4.0.0/SDWebImage/SDWebImageDownloader.m#L156

The related PR is #1737

This is actually a break change and I think the meaning what SDWebImageRefreshCached specify is not NSURLRequestReturnCacheDataDontLoad. Maybe we should change back to NSURLRequestReloadIgnoringLocalCacheData and follow that Call completion block with nil image/imageData if the image was read from NSURLCache, we need to filter that UIImage and NSData if we retrive the cache from NSURLCache.

[bpoplauschi]

@ufosky @dreampiggy thanks for looking into this
I managed to reproduce it myself (I somehow understood that it happens in the list not in the details screen, so it took me a while).

Indeed, this is a change introduced in 4.1.0, in particular #1737
I am looking at this PR and now cannot find the logic for that change:

if (options & SDWebImageDownloaderIgnoreCachedResponse) {
    cachePolicy = NSURLRequestReturnCacheDataDontLoad;
...

It's due to our old code from SDWebImageManager:

if (cachedImage && options & SDWebImageRefreshCached) {
    // force progressive off if image already cached but forced refreshing
    downloaderOptions &= ~SDWebImageDownloaderProgressiveDownload;
    // ignore image read from NSURLCache if image if cached but force refreshing
    downloaderOptions |= SDWebImageDownloaderIgnoreCachedResponse;
}

So I think we need to revert.

CC @zaczh the author of #1737 - do you remember the reason for using NSURLRequestReturnCacheDataDontLoad?
Because our existing code, if there is a cachedImage and SDWebImageRefreshCached is set, then we want to ignore the NSURLCache - which makes sense.
With your changes, we always end up with NSURLRequestReturnCacheDataDontLoad cache policy if there is an image in our cache so it will never be downloaded.

[dreampiggy]

@bpoplauschi Sorry to bother because I do not use that SDWebImageDownloaderIgnoreCachedResponse options in my iOS application. After I check the commit history but still do not understand what's this option means. If I choose to use NSURLCache instead of SDImageCache, I means to set NSURLRequestUseProtocolCachePolicy right ? But this option seems to set back that NSURLRequestReloadIgnoringLocalCacheData. It's this really useful ?(You can just do not set any and this is the default behavior). Or may this means the description Call completion block with nil image/imageData if the image was read from NSURLCache

So maybe the code below can specify that behavior(but I think it's really strange and please do not use this if I'm something wrong):

// In order to prevent from potential duplicate caching (NSURLCache + SDImageCache) we disable the cache for image requests if told otherwise
NSURLRequestCachePolicy cachePolicy = NSURLRequestReloadIgnoringLocalCacheData;
if (options & SDWebImageDownloaderUseNSURLCache) {
    cachePolicy = NSURLRequestUseProtocolCachePolicy;
}

NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:url cachePolicy:cachePolicy timeoutInterval:timeoutInterval];

if (options & SDWebImageDownloaderUseNSURLCache && options & SDWebImageDownloaderIgnoreCachedResponse) {
    NSCachedURLResponse *response = [sself.sessionConfiguration.URLCache cachedResponseForRequest:request];
    // Check if the response can be read from NSURLCache, call completion block with nil
    if (response) {
        completedBlock(nil, nil, nil ,YES);
    }
}

[dreampiggy]

Ok again. I found that hack in here in 4.0: https://github.com/rs/SDWebImage/blob/4.0.0/SDWebImage/SDWebImageDownloaderOperation.m#L426

4.1 remove that code, which means even you set that SDWebImageDownloaderIgnoreCachedResponse, you will not get called with nil image and imageData. Maybe we should check like the above code and directly call completion with nil. (But in fact, what's this behavior means?)

Or we can deprecate that strange option if it's really no usage?

[bpoplauschi]

@dreampiggy man, this is a tough one and I've been banging against it for a while. I don't know very well the idea behind those flags, but here is what I know:

SDWebImageRefreshCached

• I think we all agree SDWebImageRefreshCached means to everyone: refresh the image even if you have it cached so we need to do that. But the docs say:

Even if the image is cached, respect the HTTP response cache control, and refresh the image from remote location if needed.

• This means we have to use NSURLRequestUseProtocolCachePolicy and trigger a request even if we have the image cached locally (in SDImageCache).
• We used to have a mechanism relying on [[NSURLCache sharedURLCache] cachedResponseForRequest:self.request] but that was causing some race condition crashes inside NSURLCache. This was removed by #1737 which at that time I thought was right and I understood it, but now I have my doubts :)
• Nevertheless, I don't know why, but we used to call the completionBlock 2 times and one call would be with a nil image.

SDWebImageDownloaderIgnoreCachedResponse

This is one used behind the scenes by SDWebImageDownloader, so I'm not surprised you're not using it directly - very few people do

[bpoplauschi]

@dreampiggy back to the basics. I don't expect us to figure out all this hidden details.
Let's think this from an end-user perspective.
For those using SDWebImageRefreshCached, we must make sure NSURLRequestUseProtocolCachePolicy is set via the request and it is after #1994. In this case, they will first get the completion called with the cached image and after the download, again with the new image, if any. I think that is fine.
Otherwise, it's the default behaviour of using SDImageCache and fallback to SDWebImageDownloader if there is no cached image.

[bpoplauschi]

Also I agree we can just remove SDWebImageDownloaderIgnoreCachedResponse since it will do nothing. I don't see any reason to ignore the cached response if you have one, until you can download a new image.

[dreampiggy]

@bpoplauschi Agreed with that strange SDWebImageDownloaderIgnoreCachedResponse. I don't know what that can be used for. If you like to detect whether the URL has been cached with NSURLCache but not SDImageCache, directly use -[NSURLCache cachedResponseForRequest] is enough and this do not need any SD part to provide. Maybe we should mark this options as deprecated and do not use that (I dislike that hack and according the comments there "Note: responseFromCached is set to NO inside willCacheResponse:. This method doesn't get called for large images or images behind authentication" there maybe other bugs)...

By the way, If you still want the backward compatibility, I think the code I showed may be correct acoording to the document(which only specify Call completion block with nil image/imageData if the image was read from NSURLCache but not any HTTP cache-control information. The previous implementation(check the HTTP Response cache-control and then call nil at block) is too far away from this). And that change does not affect what SDWebImageRefreshCached work. If SDWebImageRefreshCached set, do not set that SDWebImageDownloaderIgnoreCachedResponse and it will behave the same as before.

Moreover, even even you need HTTP Response cache-control to call that nil block. You just need to place this logic into URLSession:dataTask:didReceiveResponse:completionHandler:. You have the HTTP response now and check the cache-control header or status code. Then call the nil block and return NSURLSessionResponseCancel. Do not waste that user's cellular data to download the actual image data and hack...Itn's it?

There may be 3 different solution. So we can choose one and use that. My personal is that deprecated one because I think there are quite few user and it should be the best one for further maintain.

[parth51]

When we get the solution for this issue? I want to upload the application on app store let me know if any quick fix available.

[SuperDannyBoy]

i have the same problem.

[frootloops]

@dreampiggy I'm facing this problem too. Waiting for #1994

[dreampiggy]

@bpoplauschi I reviewed and approved that changes. Should it be merged now ?

[vandutran]

Do you have an release ETA for this? We're supposed to release a new app update for iOS 11 but this is holding us back. Wonder if we should revert to older version of SDWebImage or wait for a fix?

[ufosky]

I have move to using kingfisher，which has a good code quality

[feiilin]

When can i fix this bug by Carthage?I use "Carthage update --platform iOS" there is nothing happend...

[bpoplauschi]

I have merged #2032 so I think this issue can be closed. Will be releasing 4.1.1 soon.

[bpoplauschi]

4.1.1 is now released.