facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.06k stars 24.31k forks source link

[0.32] Image caching broken #9581

Closed sjmueller closed 7 years ago

sjmueller commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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

rturk commented 8 years ago

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 commented 8 years ago

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

javache commented 8 years ago

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 commented 8 years ago

@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: https://github.com/halilb/react-native-photo-browser/issues/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 https://github.com/facebook/react-native/commit/631785f2ee852c4621eed81053a63166887071e6

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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: https://github.com/facebook/react-native/compare/0.31-stable...0.32-stable Should be doable in a couple of hours and could lead to a much faster fix

javache commented 8 years ago

@bsiddiqui Can you please link the URL of the images you're trying to load? After https://github.com/facebook/react-native/commit/631785f2ee852c4621eed81053a63166887071e6 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 commented 8 years ago

Experiencing the same with RN 0.33

bsiddiqui commented 8 years ago

@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 commented 8 years ago

@bsiddiqui, we have a fix in master branch now https://github.com/facebook/react-native/pull/9795 for dev mode. Does this fix work for you?

bsiddiqui commented 8 years ago

@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 commented 8 years ago

Anyone observing this in RN 34 / 35RC?

rclai commented 8 years ago

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

bsiddiqui commented 8 years ago

@bestander @jeveloper not having the issue anymore in 0.34.0

jeveloper commented 8 years ago

superb! , ticket done :)

rclai commented 8 years ago

Uh did you not see my comment?

rclai commented 8 years ago

Actually what are the logs supposed to say if image caching does work?

ismdcf commented 8 years ago

Not working for me as well after RN update 0.34 is it a must for the header of the image to have cache control like @bsiddiqui said

jeanregisser commented 8 years ago

Header cache control is absolutely required. Does the problem occur on a brand new RN project (react-native init MyProject) ? Could you share a git repo where it happens?

jalmaas commented 8 years ago

Seems to work for me now @0.34. Images from imgix that didn't cache at 0.33 now seem to cache properly

bsiddiqui commented 8 years ago

@bestander update - it works fine in release mode but in debug mode the images still flash

ismdcf commented 8 years ago

@jeanregisser yep it works after setting the cache-control header. but got an exception for large images (approx 1.2mb) where the caching fails . Is there a place to patch the image cache size. And @bestander it works for both debug and release modes for me.

jeanregisser commented 8 years ago

@ismdcf to be sure larger images are cached you can play with the size of the shared NSURLCache

This might be interesting to you: http://stackoverflow.com/questions/21957378/how-to-cache-using-nsurlsession-and-nsurlcache-not-working

flare216 commented 8 years ago

Upgrade RN0.34 works for me, but i found that when i display a image which the size is large , (such as 1MB) , it downloads from the server everytime, small size image( such as 200KB) didn't. Why? @jeanregisser, is this problem related to the NSURLCache you just mentioned ?

xitaoque commented 7 years ago

@jeveloper I just init a new Project with RN 0.38 to test image cache problem. The test project is very simple, just a Image. and I use iOS Emulator to test. and here is the server response to my project which I captured with wireshark.

HTTP/1.1 200 OK Content-Type: image/png Cache-Control: max-age=31556926 Expires: Sun, 01 Jan 2034 00:00:00 GMT Date: Thu, 24 Nov 2016 04:30:08 GMT Connection: keep-alive Transfer-Encoding: chunked

I added Cache-Control and Expires in server's response, but everytime I press Ctrl+r to reload the project, a New HTTP Get is send to server. In other word, the Image cache is still not working.

The HTTP server is a simple program I written with node.js. I can post the code, if you need it.

xitaoque commented 7 years ago

I used nginx to open a HTTP service and set the Cache-Control header. Still no image cache for RN...

sahrens commented 7 years ago

@mmmulani has looked at this a little bit.

mmmulani commented 7 years ago

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 commented 7 years ago

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

wachunei commented 7 years ago

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 commented 7 years ago

Seems to be fixed for me in 0.41.2

raulmt commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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