React-native: RCTImageLoader always caches images (incl. 404s) and ignores HTTP cache response headers

[rocman]

As a result, if a file is missing at the beginning, a 404 response is cached and act as a direct response to the later requests, even when the file exists.
Reviewed the code (https://github.com/facebook/react-native/blob/ed1ee9bc0f3ec051ce0a10e030c532953ce7710c/Libraries/Image/RCTImageLoader.m), in line #408, storeCachedResponse is called no matter what the status code is.

[rocman]

And I guess the caches are not expiring...

[DanielMSchmidt]

@facebook-github-bot feature

[ghost]

Hey @rocman! Thanks for opening the issue, however it looks like a feature request. As noted in the Issue template we'd like to use the GitHub issues to track bugs only. Can you implement the feature as a standalone npm module? If not consider sending a pull request or a create an entry on Product Pains. It has a voting system and if the feature gets upvoted enough it might get implemented. Closing this now, thanks for understanding!

[rocman]

nonono, it's not a feature request. It's a bug report.

[rocman]

A 404 response is definitely not suitable to cache.

[DanielMSchmidt]

Oh sorry, I thought the bug you were reporting was that the cache never expires.
@facebook-github-bot reopen

[rocman]

And it seems that nowhere is doing the job to clear the expired cache.

[namse]

I think storeCachedResponse should be deleted and we just use NSURLRequest's cache policy. Because storeCachedResponse dosen't care about expiration of cache. that function just store on cache, isn't it?
If it is right, I will make a Pull Request for this. I'm just confusing a little bit about storeCachedResponse that really just store without cache expiration.

[ide]

Sometimes we should cache 404s if the server says to. @skatpgusskat's analysis looks right. If we want more control over the cache (and I think we do), we should make storeCachedResponse follow NSURLRequest or the HTTP spec's caching policy.

[namse]

@ide I've just tested RCTNetworking.m's networkTaskWithRequest, they just perfectly work caching when the image file on server was deleted(so, 404) but before set Cache-Control with enough max-age. and after max-age seconds later, the response become 404.

Do we really need more control of cache? for what?

I think the best picture is 'use default cache without storeCachedResponse, and additionally cache decoded data'.
And the flow is below,

1. Use networkTaskWithRequest first
2. On response, make cache key with URL & Date(of response's header) and check the decoded cached image is in _decodedCache(NSCache)
3. If there is Decoded and Cachced image, use that and return as UIImage, or there is not, return response's data as NSData
4. If the NSData did return, decode it and cache on _decodedCache with key(URL & Date)

I figured out that the 'Date' fields on response's header wasn't changed if it is cached by default protocol. so it could be part of cache(NSCache)'s key.

[ide]

@nicklockwood might be the best person to comment on that caching policy.

Do we really need more control of cache? for what?

Yes, being able to inspect what's in cache is useful. Not sure if we do that today but it's an API I think we will want eventually.

[namse]

@ide Could you explain me more why it is useful to be able to inspect what's in cache?
Because we could benchmark the cache?

At the exact, the reason Why do we need to be able to inspect what's in cache is, to use cached data. (I think.)
If it is true, I don't think we really need to provide API because as I said basically we can know the request was cached or not.

When we requested data and get response, we can know 2 facts, '(1) that was on cache' or '(2) that wasn't so we request to server'.
Do we need any more information more than (1) and (2)? I don't think we need more information.

And then finally if it was in cache, we just provide decoded cached image data(if we have decoded one on decodedImageCache. if we don't, just let him decode and store to cache), it seem simple and best, doesn't it?

In this way we couldn't know response is cached before request data. and my opinion is 'do we really need that information before request data even we could use well-working algorithm processing general caching policy like 'Cache-Control' things'.

[rocman]

@skatpgusskat

Do we need any more information more than (1) and (2)?

For example, the expire date of the cache. The cache can be expired, and as a result, the info about the expiring policy is needed, and as a further result, the logic to manage the cache is needed.

[namse]

@rocman

@ide I've just tested RCTNetworking.m's networkTaskWithRequest, they just perfectly work caching when the image file on server was deleted(so, 404) but before set Cache-Control with enough max-age. and after max-age seconds later, the response become 404.

Even default cache system make to expire data with caching policy, do we need to expire the cache ourselves?

[rocman]

@skatpgusskat
RCTNetworking is working well. However, RCTImageLoader create and manage another cache on top of it, without expiring the cache, which prevents all the later network requests on the cached image requests from sending to server.
It seems that, just let RCTNetworking do the cache jobs, and everything will be fine.

[namse]

Back to the basic, the main point is 'NSURLCache's storeCachedResponse and cachedResponseForRequest doesn't work with cache policy'.
And my point is 'why don't we stop using NSURLCache and just use NSURLRequest's default caching system'

Here is NSURLRequest's cache policy logic.

[rocman]

@skatpgusskat I have the same idea with you.

[namse]

If other guys, @ide, @nicklockwood or someone who manage this part agree this idea, I'll gladly make code and PR.

[tedzhou]

@skatpgusskat I agree. I believe NSURLRequest works just fine.

[nicklockwood]

In my experience, cache control headers are very rarely set correctly on the server side, leading to images being expired unnecessarily. iOS's default caching policy is also somewhat opaque and subject to change at Apple's whim - I don't think we want to just rely on the defaults.

That said, caching a 404 is obviously a mistake. We should only be caching successful image responses, so please do put up a PR to fix that.

I'm open to making the caching system more accessible - e.g. moving the image/network cache into its own module and exposing it to JS. I did have a diff along those lines long ago, but it never came to anything.

[namse]

@nicklockwood I agree that iOS's default cahcing policy is opaque.

But if we use 'NSURLRequest'(we didn't set any NSURLRequestCachePolicy, right?), it mean we still use iOS's default caching policy as above picture with defeault setting : NSURLRequestUseProtocolCachePolicy.

What is the opaque one on iOS's default caching policy? I think the only one is 'when the cache control doesn't set up, how long time caching work'.
As you know, Apple said 'the stale cache will fetch again'. and the opaque one is what the exactly 'stale' means.

from the experiences, people said on iOS, 6 ~ 12 hours later the cached data(that has no cache control or expiration setting before) is deleted, so I think that is 'stale'.

What we can get when we provide image/network cache module is

1. can set the 'stale's standard time.
2. can provide information that url's response is cached.
3. can provide decoded image cahcing (I think It is really good)

And as I said above, the No 2. is not useful comparing that we use default caching system. no reason why we should provide No 2., logically.

Do we really need provide No 1.? If there are people who care about 'stale', he should set the cache control up on server side. Let's think about the general case.
And the biggest problem is we should make code that our customized cache work with HTTP Spec's cache control, FOR ONLY ONE - Setting the OUR 'STALE' STANDARD.

Without making the code which provide our cache with HTTP Spec, We cannot solve 404 response cache bug. Not only 404, the Image has changed on server, the Client's Image will not change before using all cache(200MB, on disk cache, code). but if we use NSURLRequest's default cache policy, we don't have to make that codes. I think that is waste of time or overtechnologizing.

I strongly suggest just using default cache policy. and if you don't agree with me, please let me know why. I'm so glad to be helpful to solve this problem.

[ide]

In my experience, cache control headers are very rarely set correctly on the server side, leading to images being expired unnecessarily.

I really think we should honor the server's cache headers by default unless we explicitly override them on the client. It's better to refetch an image and err on the side of correctness than to have a stale cache.

It could be useful to let the client override the server's cache headers if the programmer is explicit about it. For example: <Image src={...} cachePolicy={???} />, where cachePolicy is a string like "always", "never", "server" (HTTP spec), or "system" (Apple/Google arbitrary behavior), and later could be a function but let's start simple.

[namse]

@ide to provide client cachePolicy like 'always', or 'never', we can use NSURLRequestReloadIgnoringLocalCacheData or NSURLRequestReturnCacheDataDontLoad.
Also If we want to provide cache's max-old on client side, we can set the request's header with Cache-Control: max-old=XXX. That's how HTTP control the cache on client side, I believe.

[namse]

I have my doubts about creating the Cache Module. So, here is my road map,

1. provide cachePolicy property on JS side, with string 'always', 'never', 'server', 'system', or number N (N second later expired), by setting NSURLRequest's NSURLRequestCachePolicy
2. defaultly use NSURLRequestUseProtocolCachePolicy
3. control the cache with default cache system.
4. provide decoded image cache with stored with key - ${URL}-${server response header's Date}(because response has cached, the date will not change until fetching new one.)

Anything else? do you agree with me?

[tedzhou]

@skatpgusskat
One more thing. Assume that we receive a image with Cache-Control:mag-age=600, then 10mins later client should make a request from server right? If the client is offline at that moment, we would get a empty imageView.

Generally, image frameworks should use the last received image instead.

[namse]

@tedzhou good point. the NSURLRequestReturnCacheDataElseLoad is fit to it. then we could provide 'offline' mode for cachePolicy property.

I think it couldn't be default because generally programmer think if server or cache has no that data, then it should be fail, so they will make a logic with that thought.

reference is here.

[ide]

do you agree with me?

Wait for sign-off from facebook and whether that proposal meets their current needs and provides groundwork for their future plans.

cc @vjeux -- how much of a plan for the network cache have you thought through yet, and are there explicit goals and non-goals for it?

[vjeux]

I have no idea what's the status on this. @nicklockwood commented on the thread, any thoughts?

[namse]

@vjeux may it be helpful to give you summary arranged this issue and discussion above?

It just looks like... there is a plan of network cache for react-native, so we should be careful with that plan's direction, right?

[namse]

@ide could you let us when it is ready please?

[ide]

Just as a heads up, it sounds like there is no explicit plan or timeline.

[namse]

@ide even there are no dicision, it looks better provide Image module with default cache system. I start to make that code.
it is so ugly situation to let user use odd image url to ignore stupid current cache system.

[namse]

Hey guys, here is good news! the PR has accepted!

[ide]

^ Closing this issue since RN iOS is now using NSURLCache's behavior on master.

[npomfret]

What in the 404 response headers causes images to be cached? The server I'm using responds with the following (only), is it the e-tag?

Accept-Ranges:bytes
Content-Length:34
Content-Type:application/json
Date:Fri, 05 Aug 2016 13:29:55 GMT
Etag:"7-301c4533490e06afd01653ddbcafab48"
Server:CouchbaseLite 1.3.0 (build 62)

[namse]

@npomfret you mean, the server responds the 404 code with that header? Nothing caches on 404 response. I actually dont understand what you say.

[rocman]

@npomfret A "max-age" or "expires-date" header field would be needed.

[npomfret]

@skatpgusskat the title of this issue is 'RCTImageLoader always caches images (incl. 404s) and ignores HTTP cache response headers', and @ide says "_Sometimes we should cache 404s if the server says to_".

I'm experiencing cached images that are 404ing, and the headers I posted are the headers that the server responds with when there's a 404. My question is: do/should these headers result in a cached 404? Because it seems they are being cached (in RN 0.29) and I just get a blank image - even when the image becomes available and is no longer 404ing.

[rocman]

@npomfret I was suffering from the same issue with you. It seems that @skatpgusskat has fixed it with a pull request, and the pull request was accepted few days ago. Maybe in the next version, the problem will be gone.

[npomfret]

Ah! Awesome, thanks.