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

Question: SDWebImage appears to be using http GET method with .refreshcached instead of a HEAD method first #2904

Open
matthewweldon opened this issue Dec 9, 2019 · 29 comments
Labels
discussion help wanted Extra attention is needed NSURLCache URLSession built-in cache related issue NSURLSession URLSession network related issue
Milestone

Comments

@matthewweldon
Copy link

Platform Name iOS
Platform Version iOS 12+
SDWebImage Version 5.0.0
Integration Method Carthage/cocoapods
Xcode Version 11
Repro rate 100%
Repro with our demo prj unsure
Demo project link n/a

Issue Description and Steps

I filed a stack overflow question like the guide suggested but I didn't get a response.

Basically I'm wondering if I can configure SDWebImage to use a head method with .refreshCached enabled to prevent unnecessary download bandwidth usage on our backend.

Copy/pasted the text from the stack overflow question here:

I'm using SDWebImage for caching images in a Swift/iOS app, and we are using it in some places with .refreshCached option enabled.

We're using Amazon s3 for images which has support for the "Last-Modified" header on the request.

My understanding is that with .refreshCached enabled SDWebImage should check the http header with a HEAD method to check to see the last modified date in the header, THEN if the Last-Modified date has changed, it can perform a GET method on the URL to get the image data, saving bandwidth.

However when I inspect the calls that the app is making (using CocoaDebug), I only see the GET methods in the log, meaning even when the image is cached we're still fetching all the image data.

On subsequent views where the images are visible, I see that images load instantly, implying the cache is working, but I still see the full GET methods and no HEAD method in the log.

Does SDWebImage support this HEAD followed by GET flow or am I doing something wrong or otherwise misunderstanding?

@dreampiggy
Copy link
Contributor

dreampiggy commented Dec 10, 2019

This is not used for prevent unnecessary download bandwidth usage on our backend
This is used for always ensure when server image changed, client ignore the local cache...Not the meaning you want.

It use URLSession's NSURLRequestUseProtocolCachePolicy .

This NSURLRequestUseProtocolCachePolicy behavior, it's that we actually do download with GET at the first time (then store the response with E-tag and cache expiration date). When next time query, URLSession trigger HEAD request. This is previous behavior I found.

If you found that it always request GET method, maybe this total solution need to be re-written, or the URLSession have something new configuration which we can use.

@mikemike396
Copy link

mikemike396 commented Dec 10, 2019

We are experiencing the exact same issue. We have a CollectionView at the bottom of our TableView cell that holds users account images (URLs from Facebook, Twitter, LinkedIn)

Here is a video showing how it downloads as we scroll up and down.

let url = URL(string: account.avitarLocation)
profileImageView.sd_imageIndicator = SDWebImageActivityIndicator.gray
profileImageView.sd_setImage(with: url, placeholderImage: nil, options: SDWebImageOptions.refreshCached, completed: nil)

https://imgur.com/XVMNQep

Here is a video showing it not downloading as we scroll up and down.

let url = URL(string: account.avitarLocation)
profileImageView.sd_imageIndicator = SDWebImageActivityIndicator.gray
profileImageView.sd_setImage(with: url, placeholderImage: nil, /*options: SDWebImageOptions.refreshCached,*/ completed: nil)

https://imgur.com/WirQfwo

Version of SDWebImage

SDWebImage (5.4.0):
SDWebImage/Core (= 5.4.0)

@dreampiggy dreampiggy added discussion NSURLSession URLSession network related issue labels Dec 11, 2019
@stale
Copy link

stale bot commented Feb 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Feb 9, 2020
@dreampiggy dreampiggy removed the stale label Feb 9, 2020
@stale
Copy link

stale bot commented Apr 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Apr 9, 2020
@mikemike396
Copy link

Still open

@stale stale bot removed the stale label Apr 10, 2020
@dreampiggy
Copy link
Contributor

dreampiggy commented Apr 10, 2020

Need some more works for this. The worst, re-implements the HTTP Cache Control all details, like E-tag, Expire-Date, etc.

All of this is because of that NSURLCache can not been used for customization, which means, the HTTP cache is different from image cache :)

@dreampiggy dreampiggy added the NSURLCache URLSession built-in cache related issue label Apr 10, 2020
@dreampiggy dreampiggy added this to the Future milestone Apr 11, 2020
@stale
Copy link

stale bot commented Jun 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Jun 29, 2020
@dreampiggy dreampiggy removed the stale label Jun 29, 2020
@dreampiggy
Copy link
Contributor

Still a issue, need more and feasibility solution for this task. Help wanted

@dreampiggy dreampiggy added the help wanted Extra attention is needed label Jun 29, 2020
@stale
Copy link

stale bot commented Aug 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Aug 29, 2020
@mikemike396
Copy link

Still an issue

@stale stale bot removed the stale label Aug 29, 2020
@surajrautela
Copy link

Even I am facing similar issue . do we have a workaround for it ?

@dreampiggy
Copy link
Contributor

I think this is URLSession's bug or wrong behavior.

When use NSURLRequestUseProtocolCachePolicy, it should use HEAD and use E-tag to determine the cache expiration.

It this does not work, we need rewrite this. But no one here provide a suitable MR or proposal for this problem :(

@surajrautela
Copy link

@dreampiggy I was trying to set up requestPolicy for NSUrlCache but seems like it is not allowing to do that on shared downloader. maybe setting up the right policy on NSUrlCache can help ??

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 8, 2020

@surajrautela
Copy link

I did that but it still didn't worked . However when I simply used NSUrlCache without using SDWebImage everything worked fine !. Seems like some issue with SDWebImageDownloader

@dreampiggy
Copy link
Contributor

dreampiggy commented Sep 9, 2020

Sounds strange.

You can debug the code. SDWebImage by default will override the cache policy to NSURLRequestReloadIgnoringLocalCacheData , because we have SDImageCache.

Howevr, you can still modify the cache policy by your own using SDWebImageRefreshCached options, or requestModifier I talked above.

Source code (where we set the cache policy) https://github.com/SDWebImage/SDWebImage/blob/master/SDWebImage/Core/SDWebImageDownloader.m#L270

Source code (where you can use request modifier) https://github.com/SDWebImage/SDWebImage/blob/master/SDWebImage/Core/SDWebImageDownloader.m#L286

@hoogendam
Copy link

@dreampiggy +1 for this issue, I'm running into this as well. The proposed workarounds do not work.

  • When setting .refreshCached the image is re-downloaded every time by a GET request, even though the server response contains a Last-Modified header.
  • Same when using requestModifier to set cachePolicy to NSURLRequestUseProtocolCachePolicy.

@surajrautela
Copy link

@dreampiggy I tried to debug the code . The policy is getting set correctly

@dreampiggy
Copy link
Contributor

I think this is URLSession's bug or wrong behavior.

When use NSURLRequestUseProtocolCachePolicy, it should use HEAD and use E-tag to determine the cache expiration.

Already talked about this.

@dreampiggy
Copy link
Contributor

Did you pass SDWebImageDownloaderUseNSURLCache options to SDWebImageDownloader (Which is automatically applied when use SDWebImageRefreshCached) ?

image

@surajrautela
Copy link

Did you pass SDWebImageDownloaderUseNSURLCache options to SDWebImageDownloader (Which is automatically applied when use SDWebImageRefreshCached) ?

image

no , I didn't .

@surajrautela
Copy link

any update on this ??

@mikemike396
Copy link

Any update?

@dreampiggy
Copy link
Contributor

If NSURLSession does not do something magic, the last idea, is to Implments HTTP Cache control from stratch, drop NSURLSession's NSURLRequestUseProtocolCachePolicy

Although I still think this is something mistake between SDWebImage's usage, or Apple's documentation: https://developer.apple.com/documentation/foundation/nsurlrequestcachepolicy/nsurlrequestuseprotocolcachepolicy

@dreampiggy
Copy link
Contributor

@stale
Copy link

stale bot commented Jan 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Jan 3, 2021
@mikemike396
Copy link

Still an issue

@stale stale bot removed the stale label Jan 3, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions.

@stale stale bot added the stale label Jul 21, 2021
@stale stale bot closed this as completed Apr 17, 2022
@mikemike396
Copy link

mikemike396 commented Apr 17, 2022

Still an issue, can we reopen? @dreampiggy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion help wanted Extra attention is needed NSURLCache URLSession built-in cache related issue NSURLSession URLSession network related issue
Projects
None yet
Development

No branches or pull requests

5 participants