Disk caching of `UIImageView+AFNetworking` category #1136

Closed
wants to merge 3 commits into
from

Projects

None yet
@robertmryan

I have attempted to add disk caching of UIImageView+AFNetworking category. I've made best efforts to ensure backward compatibility (i.e. it defaults to RAM-only cache unless you invoke the variation of the methods with an options parameter in which you specify disk-based caching as well).

In an otherwise quite simple change, the one wrinkle was that AFImageRequestOperation did not expose the responseData, so I had to make new methods in AFImageRequestOperation. As a result, although I would have liked to limit my edits to the UIImageView category, I had to add a couple of methods to AFImageRequestOperation.

The one change that I'm particularly apprehensive about is my removal of setCompletionBlockWithSuccess. I removed this because I needed it to return the NSData for the responseObject, and as that's the standard completion block behavior, I concluded that setCompletionBlockWithSuccess was no longer necessary. If you really need it, I can restore the setCompletionBlockWithSuccess that has the UIImage variation of responseObject. But I'll have to implement some other rendition that returns the NSData object rather than the UIImage object.

By the way, while I did my AFImageRequestOperation modifications (to add a variation that supports returning the NSData as well) to optimizing disk caching, this might be a useful feature in general. When retrieving an image in the network, the naive intuition, to use UIImagePNGRepresentation or UIImageJPEGRepresentation is often not a good idea. You lose meta data, you can either lose image quality or make the file larger (if original was already JPEG) or both, etc. It's really quite important when retrieving images to be able to capture the original resource, the NSData, rather than only getting the UIImage. For lots of web stuff, sure, the UIImage is fine, but for serious image stuff, having access to the original NSData can be very useful.

Anyway, there you go. There's no pride of authorship here, so I'm open to comments, suggestions and other feedback. But disk caching of asynchronous image requests is a very useful feature. Currently when someone on Stack Overflow needs disk-based caching (for example, this is an example of a recent question), we have to tell them, "er, well, then don't use AFNetworking, but use something like SDWebImage instead." And for me, at least, it's rare that I want caching of images, and don't want disk caching, and I can't be the only person for whom this is the case. So disk caching of images seems like a very logical extension.

@mattt

Hi @robertmryan. Thanks so much for submitting this pull request. However, image caching is a feature that AFNetworking has supported by its architecture from the outset.

As described in the AFNetworking FAQ:

Does AFNetworking have any caching mechanisms built-in?

AFNetworking takes advantage of the caching functionality already provided by NSURLCache and any of its subclasses.

UIImageView's setImageWithURL: uses AFImageCache (a private NSCache) internally to optimize performance on scroll views (NSURLCache is not fast enough to load seamlessly). If the in-memory AFImageCache does not have an entry for a requested image, it will then be passed off to the Foundation URL Loading System. If NSURLCache has a cache entry for the request—either on disk or in-memory—(and assuming the caching headers allow for it) the request will be transparently and automatically loaded from URL cache, passed to the UIImageView, and added to AFImageCache.

Adding another disk caching layer only serves to complicate the matter of expiring old data and ensuring fresh data when requested. NSURLCache encapsulates all of this by following HTTP caching semantics. It's built in and works reasonably well, so there's not much need in reinventing the wheel.

Does that make sense? Is there anything that you think is missing from the current approach?

@mattt mattt closed this Jul 15, 2013
@robertmryan

A NSCache solves a very different problem than a "disk"-based cache. NSCache is wonderful at solving the intra-session caching of images. But it's useless for cross-session caching or where an app might encounter low memory situations.

First, NSCache is not persistent. When the app is terminated, that cache is lost. Second, it's using RAM, which can easily be purged in memory pressure. (And that's a good thing: It should be purged quickly when RAM runs low.)

When you cache to persistent storage, it doesn't consume the limited RAM available to the app. Most of us need this second tier of caching that persistent caching offers. (There's a reason that so many of the asynchronous image loading classes, such as SDWebImage provide it.)

When you're dealing just with thumbnails (where the cost of re-retrieving the image isn't all that expensive), a simple NSCache-based approach is fine. But when dealing with larger files, it just isn't up to the task. And re-retrieving the large images when the NSCache and NSURLCache is purged (such as what happens automatically under memory pressure, or didReceiveMemoryWarning, or after the app terminates) from the server is a horrible solution. Worse, the AFImageRequestOperation doesn't return the NSData to the app, so even if you wanted to do some app-level persistent storage, you can't do that (without a UIImage round trip which results in data loss), and you have to fall back to AFHTTPRequestOperation (and thus losing some of the benefits of AFImageRequestOperation).

But, you're quite right, you really have to be careful about using persistent storage caches, and there are many times when it's simply not appropriate (e.g. something like Snapchat where the experience is lots of random, short-lived images). But there are many scenarios where persistent caching is critical. And I also recognize that the persistent storage cache that I implemented is fairly primitive (though no more primitive than the others I saw out there), so I'd "get it" if you wanted to so something a little more sophisticated.

But, hey, it's completely up to you. I completely get it if you don't want to try to be all things to all people. But in the absence of persistent storage caching, the UIImageView category is rendered unusable for many apps. And because of the way AFImageRequestOperation is structured, it renders that unusable, too. Fortunately, people can bypass AFNetworking and use one of those other UIImageView categories out there. It's just too bad that you then end up with dueling network queues, etc.

Bottom line, I obviously think it's valuable add-on, but if you don't that's fine. No worries. Have a good one.

@mattt
@robertmryan

@mattt Thanks for your patience. I understand what you're saying conceptually, but in practice, I'm finding that AFNetworking is not caching the images in persistent storage. The image is downloaded first time (slow). If I immediately re-retrieve it, it's fast (presumably NSCache in action). But if I terminate the app (not just background, but kill it) and run the app again, it doesn't retrieve it from persistent storage, but rather downloads it again. This is the problem I tried to address with my pull request.

But, I took your advice and reviewed the FAQ, and the paragraph that immediately follows the one you quoted actually points out that NSURLCache doesn't perform persistent storage caching in iOS, and thus an existing fork is recommended:

We recommend you try out Pete Steinberger's fork of SDURLCache, which provides disk caching, which is not otherwise implemented by NSURLCache on iOS. You may find it useful to set ignoreMemoryOnlyStoragePolicy to YES for the SDURLCache instance, which will ensure that images will be cached to disk more consistently for offline use (which, as reported in the SDURLCache README, iOS 5 supports, but only for http, so it still remains a good option if you want to support iOS 4 or https).

So, this issue is acknowledged and there is an existing fork that addresses it. Any chance of merging a persistent-storage cache with the main branch of AFNetworking for iOS users? It doesn't have to be mine. I'm ok if it's Pete's.

@tomasikp

@robertmryan at the top of Pete Steinberger's README:

Note: as of iOS5, this is no longer needed. NSURLCache now properly caches objects, as long as the Cache-Control header is set.

@robertmryan

Thanks, @tomasikp for clarifying that.

I've tried the following in iOS, but when I terminate the app and restart it, the image is re-downloaded:

self.label.text = @"Retrieving image";
CFAbsoluteTime start = CFAbsoluteTimeGetCurrent();
NSURL *url = [NSURL URLWithString:@"http://my.url.was.here.com/_DSC1416c.jpg"];
NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:url];
request.cachePolicy = NSURLRequestReturnCacheDataElseLoad; // trying explicitly setting cache policy
self.imageView.image = nil;
[self.imageView setImageWithURLRequest:request
                      placeholderImage:nil
                               success:^(NSURLRequest *request, NSHTTPURLResponse *response, UIImage *image) {
                                   self.imageView.image = image;
                                   self.label.text = [NSString stringWithFormat:@"%.3f elapsed", CFAbsoluteTimeGetCurrent() - start];
                               }
                               failure:^(NSURLRequest *request, NSHTTPURLResponse *response, NSError *error) {
                                   self.label.text = [NSString stringWithFormat:@"failed after %.3f", CFAbsoluteTimeGetCurrent() - start];
                               }];

I tried with and without the cachePolicy line. Pursuant to Pete's README, I also tried adding the following to my viewDidLoad:

NSURLCache *urlCache = [[NSURLCache alloc] initWithMemoryCapacity:1024*1024*10 // 10MB mem cache
                                                     diskCapacity:1024*1024*10 // 10MB disk cache
                                                         diskPath:@"AFNetworking"];
[NSURLCache setSharedURLCache:urlCache];

There must be something I'm missing, as I'm not finding the cache to persist once the app is terminated and then restarted.


Update:

I'm wondering if this is related to issue #566.


Update 2:

As I scour Stack Overflow and other sources, there are many disparaging comments about the inconsistent nature of NSURLCache on iOS. I've found no silver bullets and am concluding that a separate caching mechanism is needed.

@TeknoloGenie

@robertmryan Were having the same issue with images being forced to be re downloaded if the app is put in background or re opened. We'll keep you posted if we find out before you do.

@vdaubry

Hi,

We are also facing a lot of issues with disk caching using the UIImageView+AFNetworking category.

We are serving images from Cloudfront, in order to get disk caching to work we had to make sure that the cache headers were properly set :

  • Content-Type
  • Cache-Control : public or private BUT MINIMUM max-age > 1835400 (empirical)

All of them are mandatory, if any of these fields is missing or if the max-age in less than 1835400 (21 days) , then the image won't be cached to disk.

I couldn't find any documentation for this behavior. I suspect these rules might change with any release of iOS...

Can we have some feedback about this situation from the AFNetworking team ? It seems to us NSURLCache behavior is not reliable enough to implement any offline functionality.

On top of that do you know how what is the behavior of the cache in case the ipad is running low of disk space ?

Any situation where the app might launch offline and no images are shown would be a no go for most offline functionality.

What do you think ?

PS : Here is a list of the header we configured to make disk caching work

curl -I https://d1clevg57kuegd.cloudfront.net/production/book/5981/252x342/5981_1368711866.jpg
HTTP/1.0 200 OK
Content-Type: image/jpeg
Content-Length: 11298
Connection: keep-alive
Date: Tue, 16 Jul 2013 09:24:03 GMT
Cache-Control: public, max-age=1835400
Last-Modified: Fri, 31 May 2013 22:17:32 GMT
ETag: "448e1fe301acc3ff5b8501b6b5429fa6"
Accept-Ranges: bytes
Server: AmazonS3
Age: 20114
Via: 1.0 821625c707a1767ac0d9b16edba740c7.cloudfront.net (CloudFront)
X-Cache: Hit from cloudfront
X-Amz-Cf-Id: PeZ7SzECzcWjRVIY13XRbYQWeQM7vyp1MdfcpUqWN0DIrPhX3EdE8Q==

@mattt

NSURLCache is the correct level of abstraction in the correct part of the stack in the URL Loading System. Optimizing its performance for your particular application is an exercise for the developer, and outside the scope of AFNetworking.

If anyone is unsatisfied with the behaviors or undocumented nature of NSURLCache, this project has had a standing recommendation of @steipete's fork of SDURLCache as a viable replacement.

@vdaubry

Thanks for your answer, i didn't mention that we were already using SDURLCache and still have a lot of trouble regarding headers configuration when using the cache offline.

I understand your point and won't try to argue. In our case we decided the standard caching mechanism of iOS is not reliable enough for our offline usage. We switched to SDWebImage which offers a replacement for image caching.

I'm not saying AFNetworking should provide such a custom caching mechanism, but i think it might be interesting to mention some alternative if some other developers aren't satisfied with the NSURLCache/SDURLCache behavior.

Caching images served through HTTPS has proven to be painful for us and it seems this is a common issue, would you consider an addition to the AFNetworking FAQ on caching ?

@benmos

Just to add another "me too". After wasting countless hours messing around with various NSURLCache params and Cache-Control configurations, I've also come to the conclusion that the built-in iOS support here is horribly broken. And, like others, I've also resorted to switching to SDWebImage to get this essential behaviour.

@danpe

+1

@auibrian

Maybe I'm not understanding the issue properly but couldn't you implement the custom cache as a user of AFNetworking without a pull request? i.e. without having to change the framework to support something that can be added out of the box pretty easily?

For instance: setImageWithURLRequest: placeholderImage: success: failure:
The success block for that has the image as an argument, you could implement your own caching there, right?

@robertmryan

@auibrian You are right that you could implement your own application-level persistent cache, but it doesn't strike me as the ideal level of abstraction. But given Mattt's reticence on this matter, that is might be your best alternative if using AFNetworking.

@expkzb

@robertmryan I understand what you said and I have done it myself.It's very necessary for a disk caching. 👍

@askutarenko

+1 to the disk caching. I've been using SDWebImage and it's UIImageView category just because it has disk caching, but I would rather use AFNetworking UIImageView category instead, because I think it is more convenient to have just one category instead of two. Of course, this is not hard to implement by myself at all, but I really like when everything comes out of the box fully implemented and I don't have to write my own code, subclass NSURLCach, search for another framework or etc. Maybe I am too lazy?

Anyway, Mattt thank you for the great job on AFNetworking!

@ngoccuong291

@robertmryan We're having memory pressure issue with Image caching. +1 for this issue. Our app has more than 1000 images, when scrolling up and down, the app just could not allocate enough memory and crash.
@mattt Is there any way to just clear the cache when we need?

@robertmryan

@ngoccuong291 Before we consider the cache, are you 100% sure you're releasing the images in your app as images scroll off of the visible portion of the scroll view? Are you sure it's a cache problem, and not a basic failure to release image views (and their model objects, the images) as they scroll out off screen?

Also, are you using the UIImageView category? If so, you have access to the sharedImageCache, and you can empty that when you want. But you shouldn't have to, because it observes UIApplicationDidReceiveMemoryWarningNotification and purges itself upon memory warnings.

@getaaron

@ngoccuong291 It's almost certainly not an issue with the AFNetworking cache - one of my apps has well over 5,000 images loaded with no memory pressure issues.

@ngoccuong291

@robertmryan I used the UIImageView category and reuse the cells on UITableView as per the guideline. I did Profile the app to monitor memory allocation, there was no leak and 67% of memory was allocated to [AFImageRequestOperation responseImage]. This 67% did not get deallocated and just lived until we kill the app or the app crashes.
@getaaron I'm not sure if there is any big improvement in term of caching for AFNetwork because I'm using AFNetwork v1.3.3. Cache just got bigger until the app crashed. Another one is that our images has high resolution (600x600)

@robertmryan

@ngoccuong291 I'd suggest creating a question on Stack Overflow (sharing the link here, if you'd like). Make sure to include code snippets (notably cellForRowAtindexpath). We can't help you without seeing the source code, and this issue might not be the right forum.

@getaaron

@ngoccuong291 If you're on 1.x, add this to the end of your sharedImageCache method to fix the memory issue:

[[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationDidReceiveMemoryWarningNotification object:nil queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification * __unused notification) {
    [_af_defaultImageCache removeAllObjects];
}];

You can see it on the 2.x branch here.

@mattt I've seen this come up a bunch on Stack Overflow. Might be worth cherry-picking this commit to the 1.x branch.

@wsnook wsnook referenced this pull request in mapbox/mbxmapkit Mar 7, 2014
Closed

investigate NSURLCache #67

@haihw

Hi all,
Does NSURLCache work for the response that contains Tranfers-Encoding: Chunked and no Content-Length provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment