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
RCTImageLoader always caches images (incl. 404s) and ignores HTTP cache response headers #7571
Comments
And I guess the caches are not expiring... |
@facebook-github-bot feature |
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! |
nonono, it's not a feature request. It's a bug report. |
A 404 response is definitely not suitable to cache. |
Oh sorry, I thought the bug you were reporting was that the cache never expires. |
And it seems that nowhere is doing the job to clear the expired cache. |
I think |
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 |
@ide I've just tested Do we really need more control of cache? for what? I think the best picture is 'use default cache without
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( |
@nicklockwood might be the best person to comment on that caching policy.
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. |
@ide Could you explain me more why it is useful to be able to inspect what's in 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.) 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'. 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'. |
@skatpgusskat
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. |
Even default cache system make to expire data with caching policy, do we need to expire the cache ourselves? |
@skatpgusskat |
@skatpgusskat I have the same idea with you. |
If other guys, @ide, @nicklockwood or someone who manage this part agree this idea, I'll gladly make code and PR. |
@skatpgusskat I agree. I believe |
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. |
@nicklockwood I agree that iOS's default cahcing policy is opaque. But if we use 'NSURLRequest'(we didn't set any 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'. 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
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. 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 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. |
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: |
@ide to provide client cachePolicy like 'always', or 'never', we can use |
I have my doubts about creating the Cache Module. So, here is my road map,
Anything else? do you agree with me? |
@skatpgusskat Generally, image frameworks should use the last received image instead. |
@tedzhou good point. the 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. |
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? |
I have no idea what's the status on this. @nicklockwood commented on the thread, any thoughts? |
@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? |
@ide could you let us when it is ready please? |
Just as a heads up, it sounds like there is no explicit plan or timeline. |
@ide even there are no dicision, it looks better provide Image module with default cache system. I start to make that code. |
…system Summary: Before this PR, ```RCTImageLodaer```'s Cache was too big(200MB on disk) and It doesn't work with HTTP Cache-Control header. So to provide dynamic image, the users must have to add random value on url( ex. adding current date) to avoid cache. So I change that cache system to default ```NSURLRequest```'s cache system, which is well-working with HTTP specs. As the discussion on this issue #7571 , making custom cache policy processor is not ready yet and useless, over-tech things, I think. Even we have no plan about image cache system(or would change plan later), before having a nice plan, I think we should let user use image module with common HTTP Specs. So I remove custom ```NSURLCache```, and make logic like below, 1. try fetch image, 2. on response, get ```Date``` on response's header and make ```cacheKey``` with ```Date```. > (why? because if ```NSURLRequest```'s response was cached, the response's ```Date``` header dosen't change.) 3. find decoded imag Closes #8235 Reviewed By: bnham Differential Revision: D3469086 Pulled By: javache fbshipit-source-id: 35a5552cda6e6c367481020bbf3c28eb4a9d0207
Hey guys, here is good news! the PR has accepted! |
^ Closing this issue since RN iOS is now using NSURLCache's behavior on master. |
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?
|
@npomfret you mean, the server responds the 404 code with that header? Nothing caches on 404 response. I actually dont understand what you say. |
@npomfret A "max-age" or "expires-date" header field would be needed. |
@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. |
@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. |
Ah! Awesome, thanks. |
…system Summary: Before this PR, ```RCTImageLodaer```'s Cache was too big(200MB on disk) and It doesn't work with HTTP Cache-Control header. So to provide dynamic image, the users must have to add random value on url( ex. adding current date) to avoid cache. So I change that cache system to default ```NSURLRequest```'s cache system, which is well-working with HTTP specs. As the discussion on this issue facebook#7571 , making custom cache policy processor is not ready yet and useless, over-tech things, I think. Even we have no plan about image cache system(or would change plan later), before having a nice plan, I think we should let user use image module with common HTTP Specs. So I remove custom ```NSURLCache```, and make logic like below, 1. try fetch image, 2. on response, get ```Date``` on response's header and make ```cacheKey``` with ```Date```. > (why? because if ```NSURLRequest```'s response was cached, the response's ```Date``` header dosen't change.) 3. find decoded imag Closes facebook#8235 Reviewed By: bnham Differential Revision: D3469086 Pulled By: javache fbshipit-source-id: 35a5552cda6e6c367481020bbf3c28eb4a9d0207
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.
The text was updated successfully, but these errors were encountered: