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

[0.32] Image caching broken #9581

Closed
sjmueller opened this issue Aug 25, 2016 · 64 comments
Closed

[0.32] Image caching broken #9581

sjmueller opened this issue Aug 25, 2016 · 64 comments
Labels
Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@sjmueller
Copy link
Contributor

There has been quite a noticeable regression with image caching in 0.31. The symptoms in my app are that any image rendered multiple times is fetched (and re-decoded?) for each Image instance. This is especially apparent with small avatar images, which in the past would have no delay in appearing if the same one was rendered in in a previous Image component.

The symptoms can be observed on iOS; both simulator and device, in both debug and release mode; I have not checked on Android, and have not tested with 0.33 rc. I can create a video to illustrate the problem, but currently traveling to London and won't be able until I return back to the states next week.

I suspect it has to do with the significant image cache refactor that happened on these commits: f12d0a2 54244e1 ddc70ff

I originally mentioned #9431 but here we can track the issue separately.

cc @bestander @mkonicek @javache
Would like to cc Maria Mateescu (author on all three commits above) but don't actually know her github handle. Maybe @matemariaescu ?

@bestander
Copy link
Contributor

@sjmueller, any help would be welcome.
If you don't mind, please create a video and a way to reproduce it.
I'll contact Maria and ask her to have a look

@matemariaescu
Copy link

@sjmueller, I looked into the caching and the image cache is being hit. My diffs did not change the implementation, rather separated the cache from the loader. What I suspect may be causing the issue would be the fetchDate being part of the cacheKey. This was added in 631785f, @javache may be able to provide some more context.

@javache
Copy link
Member

javache commented Aug 25, 2016

@sjmueller: can you provide an example of an image (URL) that fails to be cached?

@rturk
Copy link

rturk commented Aug 26, 2016

It would be really nice to understand how image caching works in RN and how one could leverage the nuances of the code. The cache behavior/logic is documented somewhere?

@nihgwu
Copy link
Contributor

nihgwu commented Aug 27, 2016

any progress? seems the images are only cached in several seconds, and then need new fetch

@javache
Copy link
Member

javache commented Aug 27, 2016

React Native for iOS does not implement its own network caching for images. Instead we rely on the system's default NSURLCache (see http://nshipster.com/nsurlcache/ for details). If your images are refetched continuously, this may point to a misconfiguration of the cache headers sent by your server.

@halilb
Copy link

halilb commented Aug 29, 2016

@javache, I'm maintaining a react-native photo library component and I've been experiencing cache problem since version 0.25. I tested the problem a lot and I see that's happening when I create jsbundle file with react-native bundler and embed it within the app. Cache is working fine when I serve jsbundle from my computer. Here is the Github issue for the problem: halilb/react-native-photo-browser#8

I've used some Flickr images for this test and I can also confirm http responses have valid cache headers.

@sjmueller Can you check my issue to see if it seems relevant with yours?

@jalmaas
Copy link

jalmaas commented Aug 30, 2016

I started seeing caching problems when upgrading from 0.30 to 0.32. Looked like images reloaded every time. Reverted back to 0.30

@chrisnojima
Copy link
Contributor

I'm seeing this as well. My packager is echoing the same images over and over (which is new to 0.32 vs 0.31)

[9:35:07 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (256ms)
[9:35:07 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:07 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:07 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:07 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (178ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (178ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (176ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (178ms)
[9:35:08 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (40ms)
[9:35:08 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <START> processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (208ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (207ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (207ms)
[9:35:08 AM] <END>   processing asset request shared/images/icons/icon-placeholder-avatar-64-x-64@2x.png (197ms)

@rclai
Copy link
Contributor

rclai commented Aug 31, 2016

I'm trying to reproduce a crash that happens when I toggle images, unfortunately I was not able to reproduce the crash, but here's basically what I'm trying to do:

https://github.com/rclai/react-native-image-caching-problem

My original problem is that if I toggle the images without the style attribute, it's fine, but once I introduce the style tag, I get a crash that looks like this:

2016-08-31 14:49:33.681 [info][tid:main][RCTImageView.m:354] [PERF IMAGEVIEW] Reloading image http://192.168.1.130:8081/assets/images/play.png?platform=ios&hash=8857a07b95f27926aae603a0d11605ac as size {123, 180.5}
2016-08-31 14:49:33.681 [error][tid:com.facebook.react.JavaScript] Module RCTLog is not a registered callable module.
2016-08-31 14:49:33.684 [fatal][tid:com.facebook.react.RCTExceptionsManagerQueue] Unhandled JS Exception: Module RCTLog is not a registered callable module.
2016-08-31 14:49:33.687 [error][tid:com.facebook.react.JavaScript] Module RCTLog is not a registered callable module.
2016-08-31 14:49:33.690 [fatal][tid:com.facebook.react.RCTExceptionsManagerQueue] Unhandled JS Exception: Module RCTLog is not a registered callable module.
2016-08-31 14:49:33.691 [error][tid:com.facebook.react.JavaScript] Module RCTLog is not a registered callable module.

@jeanregisser
Copy link
Contributor

jeanregisser commented Sep 7, 2016

Looking at this because I was seeing the same static local asset being fetched over and over from the packager server like @chrisnojima, I found out that this specific behavior was introduced by 631785f

As @javache was saying, React Native is relying on the system's default NSURLCache.

However the packager is not setting any cache headers for assets requests. Hence in development, everytime a local asset is needed it's fetched again from the packager server.

Adding a Cache-Control header to the packager response for local assets fixes the issue.
I'll send a PR with the fix.

@mkonicek
Copy link
Contributor

mkonicek commented Sep 8, 2016

Does this happen with remote images (http://, https://) or with local assets stored in the filesystem that are part of the app?

@sjmueller I understand this happens in release mode, with the packager not running, correct?

@dryganets
Copy link
Contributor

I recently have reported multiple issues to NSUrlSession caching in iOS:

  1. if you provide conditional request headers response would be cached forever
  2. if server would return must-revalidate header max-age header would be ignored and url caching would for for this request (it would work like if server returned no-cache)

@pavlelekic
Copy link

I encountered this bug too. Images are being refetched on every component mount. It doesn't matter if you set caching headers, it omits caching completely. This is happening only with remote images, as far as I can tell, and it is happening in both development and release mode. I'm using RN 0.32.1 on iOS only.

@rclai
Copy link
Contributor

rclai commented Sep 12, 2016

This is happening to my local assets as well. To be more specific, local as in images inside a folder in my JS code, not Images.xcassets.

@bsiddiqui
Copy link

Having this issue on 0.33.0 in release mode when loading remote images. Every time the component renders it reloads the images. Have a simple ListView with image thumbnails. Downgrading to 0.30.0 "fixes" the issue.

@tnortman
Copy link

tnortman commented Sep 14, 2016

@bsiddiqui I downgraded to react-native@0.30.0 but this did not seem to solve my issue. Remote images still did not fetch from cache; they are still being loaded from the url. Just an fyi for anyone else attempting to downgrade to resolve this issue, perhaps downgrading further down would do it?

@bsiddiqui
Copy link

@bestander here's a video of the issue (think it's the same issue)

0.32.1 - see how images flash when ListView re-renders
https://www.dropbox.com/s/tmsi8q39w9efkfs/rn32.mov?dl=0

0.31.0 - and previous versions did not have this issue
https://www.dropbox.com/s/vf7r9cls2bvklod/rn31.mov?dl=0

@bestander
Copy link
Contributor

Thanks a lot, guys, we hear you.
I'll chase this with the team.
Any help with pin-pointing which commit did this is very much appreciated.

There are 182 commits between 0.31 and 0.32: 0.31-stable...0.32-stable
Should be doable in a couple of hours and could lead to a much faster fix

@javache
Copy link
Member

javache commented Sep 15, 2016

@bsiddiqui Can you please link the URL of the images you're trying to load? After 631785f we now respect the cache-control headers sent by the server, so if the server is not marking this image as cacheable, we need to refetch it.

@ismdcf
Copy link

ismdcf commented Sep 15, 2016

Experiencing the same with RN 0.33

@bsiddiqui
Copy link

bsiddiqui commented Sep 15, 2016

@javache https://ramble.s3.amazonaws.com/uploads/5f314937-7041-462a-ae4a-34d4435382a1

HTTP/1.1 200 OK
x-amz-id-2: FIMyEUqsILJ7QatIWRcrNl6ITeixE5vQFhDoH9pAkcK4Le9i4nDo9yY7BlmNs/3qiVMSbqvDf6g=
x-amz-request-id: E60F6765593CC1BE
Date: Thu, 15 Sep 2016 15:40:22 GMT
Last-Modified: Thu, 15 Sep 2016 15:26:35 GMT
ETag: "8c508849f3269fc01c639b3abdd3a2ac"
Cache-Control: max-age=31556926
Expires: Sun, 01 Jan 2034 00:00:00 GMT
Accept-Ranges: bytes
Content-Type: binary/octet-stream
Content-Length: 35884
Server: AmazonS3

My image is set with

// Setting key to uri so that it updates properly
// https://github.com/facebook/react-native/issues/1417
<Image key={uri} source{{ uri }} />

Perhaps this is a different problem. If I remove the key it doesn't flash but the images seem to re-render a couple seconds after the rest of the list:

https://www.dropbox.com/s/8nsqe3mg7jh28cv/imageswap.mov?dl=0

@bestander
Copy link
Contributor

@bsiddiqui, we have a fix in master branch now #9795 for dev mode.
Does this fix work for you?

@bsiddiqui
Copy link

@bestander just tested on master and same issue is happening: https://www.dropbox.com/s/3a7uj2phwhb7rgv/rnmaster.mov?dl=0

You can see the flash on the simulator on the right. Using master.

@jeveloper
Copy link

Anyone observing this in RN 34 / 35RC?

@rclai
Copy link
Contributor

rclai commented Sep 25, 2016

Yes I'm still getting this even after deleting all node modules and reinstalling. In on 0.34.0.

@bsiddiqui
Copy link

bsiddiqui commented Sep 26, 2016

@bestander @jeveloper not having the issue anymore in 0.34.0

@mmmulani
Copy link
Contributor

as @pieterdb said earlier, the default React image cacher uses NSURLCache for the url request. Can someone who is running into this issue set some breakpoints in RCTImageCache.m when calling RCTCacheKeyForImage and tell us what they're seeing

@baldurpan
Copy link

Same issue here in RN 0.40 fetching remote images.

HTTP/1.1 200 OK
Server: nginx/1.10.2
Date: Wed, 18 Jan 2017 22:35:13 GMT
Content-Type: image/png
Content-Length: 776
Connection: keep-alive
ETag: "mo-e118a8fe12b0cb5d5e5f53fd57ffbd99"
Last-Modified: Sat, 17 Dec 2016 17:05:29 GMT
Cache-Control: public
Pragma: cache
Expires: Thu, 18 Jan 2018 22:35:13 GMT

cpojer pushed a commit to facebook/metro that referenced this issue Jan 26, 2017
Summary:
Hi,

This PR fixes the problem described by chrisnojima in facebook/react-native#9581 (comment)

**Test plan**

In development mode,
- Run an app with an image: `<Image source={ require('./logo.png') }/>`
- Notice that you see the following in packager console:
```txt
6:46:42 PM] <START> processing asset request logo.png
[6:46:42 PM] <END>   processing asset request logo.png (1ms)
```
- Reload the app, or navigate to another page of the app with the same image
- Notice that you see again:
```txt
6:47:23 PM] <START> processing asset request logo.png
[6:47:23 PM] <END>   processing asset request logo.png (1ms)
```

Now wih the fix applied,
notice that you only see `logo.png` fetched once, even if you reload or show the same image in a different part of the app.

Let me know what you think.
Closes facebook/react-native#9795

Differential Revision: D3876945

Pulled By: davidaurelio

fbshipit-source-id: f41f4719e87644692a690123fd6e54eead9cc87d
@wachunei
Copy link

I'm having this issue with RN 0.40.

Debugging I found that with small images: cacheKey in RCTImageLoader.m matches the cache because the response has a date and time of when the image was cached for the first time. But with larger images, response is always with current date, so cacheKey is not the same as the one that was used when that Image was cached, then retrieved image is null so it requests it again.

@baldurpan
Copy link

Seems to be fixed for me in 0.41.2

@raulmt
Copy link

raulmt commented Feb 16, 2017

Debugging into this, it seems the problem is related to these lines.

NSURLSession created there is using the default NSURLSessionConfiguration, which comes with a "default" NSURLCache. I don't know the size of this cache, but it seems that requests bigger than some percentage of that cache size won't be cached, and that's by design, not an error. This is why for some of us, "small" images are cached, but "bigger" ones are not.

The solution is to just use a bigger cache here instead of the default one. Initially I thought we would need to expose the cache size to be configurable, and in the aforementioned lines use that size configuration. But if I'm not mistaken, the defaultSessionConfiguration will just use NSURLCache.sharedURLCache. So if in some callback just after the app is initialized, we set NSURLCache.sharedURLCache to a bigger one, "larger" images should start being cached. Example:

NSURLCache.sharedURLCache = [[NSURLCache alloc] initWithMemoryCapacity: 4 * 1024 * 1024
                                                   diskCapacity: 20 * 1024 * 1024
                                                       diskPath: nil];

inside didFinishLaunchingWithOptions.

For us, this started caching large enough images as we needed.

@jordanmkoncz
Copy link

I'm experiencing this issue as well. I'm testing by going back and forth between two different routes in my application, with one of these routes loading an Image component. The effect of navigating between the two routes is that the component that contains the Image component is mounted/unmounted on each route change. Every time I go to the route containing the Image, the actual jpeg image is being fully reloaded, rather than being cached after the first load and then loaded from the cache after that.

For reference, the image I'm testing this with is https://images.pexels.com/photos/73763/rugby-sports-players-competition-73763.jpeg?w=8192, which is a 6.6MB image that has the following response headers:

accept-ranges:bytes
cache-control:public, max-age=31536000
cf-cache-status:HIT
cf-ray:33c2cce7c8c465f9-SYD
content-length:6616893
content-type:image/jpeg
date:Wed, 08 Mar 2017 03:45:12 GMT
expires:Thu, 08 Mar 2018 03:45:12 GMT
fastly-debug-digest:7be3df018a0df1c0dcb57b88c942ef81295299aac63add6676f1dd1fb6cf80af
last-modified:Wed, 08 Mar 2017 0:59:49 GMT
server:cloudflare-nginx
status:200
vary:Accept-Encoding
x-cache:MISS, HIT
x-cache-hits:0, 2
x-content-type-options:nosniff
x-imgix-request-id:3b8d9497d00354a8f93e1d59608ab9e127b3dad4
x-served-by:cache-lax8648-LAX, cache-syd1626-SYD

I believe these response headers should result in the image being cached, as the response headers are indicating that the image should be cached for 1 year.

I tried the solution @raulmt posted, but this did not seem to have any effect. I also tried increasing the amounts like so:

NSURLCache.sharedURLCache = [[NSURLCache alloc] initWithMemoryCapacity: 12 * 1024 * 1024
                                                   diskCapacity: 60 * 1024 * 1024
                                                       diskPath: nil];

This also did not seem to have any effect.

I'm not doing anything fancy with the Image component, it looks like the following:

<Image
    style={{ flex: 1 }}
    source={{ uri: 'https://images.pexels.com/photos/73763/rugby-sports-players-competition-73763.jpeg?w=8192' }}
    resizeMode="cover"
/>

I'm using the following dependencies:

"react": "~15.4.0",
"react-native": "0.41.2",

@leaskc
Copy link

leaskc commented Mar 20, 2017

We think we are seeing this also with 0.40 on iOS, Android is unaffected.

Has anyone had a chance to see if the changes for caching updates in 0.42 (https://facebook.github.io/react-native/docs/images.html#cache-control-ios-only) have improved anything?

@sahrens
Copy link
Contributor

sahrens commented Mar 20, 2017

We use different Image and caching modules internally - this would be a nice opportunity for someone in the community to tackle. Maybe @janicduplessis? Sounds like there are a lot of karma points to be earned :)

@janicduplessis
Copy link
Contributor

I've been using a custom Image module on iOS that is a simple wrapper around https://github.com/rs/SDWebImage since I've had issues with flickers and caching with the current implementation. Leveraging an existing image library seemed like an easier solution for me than trying to fix our current implementation. If this would make sense for RN to use this instead of our current implementation I could work on cleaning it up and upstreaming it.

I think image loading is really complex and makes sense to leverage an external library a bit like Android does with Fresco, especially considering the iOS implementation is more or less maintained since it is not used in FB apps.

@sahrens
Copy link
Contributor

sahrens commented Mar 20, 2017

@brentvatne / @ide : what does exponent do? Would be nice to fix this by default in the core rather than using an external plugin.

@janicduplessis
Copy link
Contributor

janicduplessis commented Mar 21, 2017

@sahrens It uses the core one.

I guess the main changes that would be needed to make the current implementation have better UX is

  1. Synchronous memory cache
  2. Better / more aggressive cache, SDWebImage assumes urls are immutable to achieve better perf.
  3. Persistent disk cache (not 100% sure about that one but I think the cache is reset when killing the app).
  4. Support fadeDuration prop like Android, only applies when loading from disk or network.

Number 2 is probably the hardest to get right since it may differ depending on the app usage of images. One thing that the current implementation does that causes issues for one of my use cases is it caches a resized version of the image in memory instead of the original which causes flickers when loading the same image with different sizes. It should probably be customizable from objc.

@daesan
Copy link
Contributor

daesan commented Apr 19, 2017

@janicduplessis I am thinking of using SDWebImage/wrapper for our app. Do you think building an animated version of wrapper for SDWebImage would be complicated?

@SteffeyDev
Copy link

I can confirm that in RN 0.44 cached images in iOS are not working, using "force-cache" simply reloads from URL and using "only-if-cached" does not load the images at all, despite the images having been loaded previously using the same url.

@gitlovenotwar
Copy link

Same issue on 0.44 image still not cached. It request every mount of component.

@baba43
Copy link

baba43 commented Jul 21, 2017

Are there any updates on this?

Will this be fixed / investigated or should we go with external libraries to achieve reliable caching?

@JLLLinn
Copy link

JLLLinn commented Jul 25, 2017

I'm using 0.46 and it is not caching right, if needed I can provide image urls that does cache on chrome but not in react native.

@JLLLinn
Copy link

JLLLinn commented Jul 27, 2017

Looks like it caches some small image but not big ones (my 200K images are cached but the 8m ones are not)

@samouss
Copy link

samouss commented Jul 27, 2017

There is a limit for store the image based on his size. Check this file for more information:
https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageCache.m#L71

@JLLLinn
Copy link

JLLLinn commented Jul 27, 2017

@samouss Hey, thanks so much for point me to that code!

@JLLLinn
Copy link

JLLLinn commented Jul 27, 2017

@samouss It looks like it has a cap of 1mb. However, even if I downgrade my images to 550kb it still wouldn't cache it. It is using CGFloat bytes = image.size.width * image.size.height * image.scale * image.scale * 4; to calculate the size which does not look like the actual file size..

@samouss
Copy link

samouss commented Jul 30, 2017

Yes you are right.

But if you upload the Image from your app you can crop it for reduce his size with the ImageEditor API. Or do it on the server to avoid too large image. I don't see any other solutions for now.

Maybe in future release the caching strategy will be more performant as mentioned in this thread and you can upvote for this feature on Canny.

@gitlovenotwar
Copy link

It looks like this is only happening on iOS, for Android it is working perfectly.

@stale
Copy link

stale bot commented Oct 17, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 17, 2017
@stale stale bot closed this as completed Oct 24, 2017
@facebook facebook locked and limited conversation to collaborators May 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests