-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Change RCTImageLoader's Cache System to default NSURLRequest's cache system #8235
Conversation
By analyzing the blame information on this pull request, we identified @nicklockwood and @rigdern to be potential reviewers. |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
I don't understand why CI Test failed. Could you explain this to me please? |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
b9e8951
to
fab719b
Compare
while (cachedResponse) { | ||
if ([cachedResponse.response isKindOfClass:[NSHTTPURLResponse class]]) { | ||
NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)cachedResponse.response; | ||
if (httpResponse.statusCode == 301 || httpResponse.statusCode == 302) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this logic of following redirects for getting a cache hit still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I heard that redirect response cannot arrive as NSURLResponse
, and I tested and checked it.
And by line 374, that code doesn't process 301 or 302. I never heard the NSURLResponse
whit GET
method has status code 301 or 302. If it's wrong, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc'ing original author @hayeah
We don't implement URLSession:willPerformHTTPRedirection: in RCTHTTPRequestHandler. I'm not sure what the default behaviour is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested that situation, and figured out that NSURLRequest(imageURLRequest)
's URL wasn't same with NSHTTPURLResponse
's URL(set as final location of file), and the response's status code is 200(or 404 if there are no file) with that server's response headers.
I think without implementation about URLSession:willPerformHTTPRedirection
, the user-agent(in this case, iOS) try redirect and get final server's response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I finally found the docs about this.
If the response is an HTTP redirect response, the NSURLSession object calls the delegate’s URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler: method. That delegate method calls the provided completion handler with either the provided NSURLRequest object (to follow the redirect), a new NSURLRequest object (to redirect to a different URL), or nil (to treat the redirect’s response body as a valid response and return it as the result).
If you decide to follow the redirect, go back to step 2.
If the delegate doesn’t implement this method, the redirect is followed up to the maximum number of redirects.
This is already being looked at in #8102, could you not do it here? |
@@ -377,9 +371,26 @@ - (RCTImageLoaderCancellationBlock)loadImageOrDataWithURLRequest:(NSURLRequest * | |||
userInfo:nil], nil); | |||
return; | |||
} | |||
|
|||
NSString* responseDate = ((NSHTTPURLResponse *)response).allHeaderFields[@"Date"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: our formatting guideline is NSString *responseDate
. Could you apply that consistently everywhere?
Actually I didn't touch anything about local asset images. And my point is, "because we use |
edited wrong formatted codes and useless nested block. |
|
||
// Call handler | ||
[weakSelf decodeImageData:data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you nest this in the else
block and have a shared return statement after the block?
Where in this diff do we configure |
I found we didn't set any cache-policy about you can find this on this doc Actually I don't know very much wher the NSURLRequest created, but I searched whole code which create I can't edit all of that because I have no idea how much do we have to set the timeout Interval. |
(line 366 ~ 391) Done.
(line 730) Done. |
Thanks! I'll check this out internally and verify that caching works correctly in our use-cases. |
@facebook-github-bot import |
Thanks for importing. If you are an FB employee go to Phabricator to review. |
|
||
// Call handler | ||
completionHandler(nil, data); | ||
NSString *responseDate = ((NSHTTPURLResponse *)response).allHeaderFields[@"Date"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you handle the case where this is nil and replace with the empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good point, because the Date header is not 'MUST' thing on RFC2616.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmmulani But, before editing, then how can we know that response has cached or not which has no Date header?
In that case, what about ignore checking cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I briefly discussed that with @javache, our thinking was that since the server has to allow us to cache a request but is not providing a Date header, it is okay to cache it.
to be honest, this should really never show up, so I am also okay with ignoring checking the cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmmulani Because we never know the response has cached without Date
header, so I just ignore checking decoded cache and just let them decoded.
This case is so rare, so it could be ok to ignore caching, I guess.
I've just tested with simple NodeJS ExpressJS server with removing Date
header (res.removeHeader('Date')
), and I checked there are no Date header with Chrome Developer Tools, and with react-native's UIExplorer example, finally checked this logic work nicely.
Summary: Because there are no cache-control system on RCTImageLoader, we must have to add random value on URL to fetch dynamic images. So without implement of NSURLCache's custom cache control system, I just put the NSURLRequest's default cache system which is well-working. And on local image, [UIImage named:] has its own cache system, so I just consider only url request one. After fetching, it check that response was on cache with the clue - response's Date header. the cached response always has the same date. So I made cacheKey adding response's date, and store decoded image with that cacheKey. If the image was on NSURLRequest's cache, without decoding imageData again, just got decoded image on _decodedImageCache.
Then you should split your PR into multiple PRs. One PR = one commit = one idea. |
Could I ask you how it is going? I just wonder how facebook's engineer work or test or anything. |
@skatpgusskat hey we're taking a look at it still, hoping to land it by the end of this week |
…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
intro
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 What did I do?
So I remove custom
NSURLCache
, and make logic like below,try fetch image,
on response, get
Date
on response's header and makecacheKey
withDate
.find decoded image on
_decodedImageCache
withcacheKey
if there isn't, decode that image and store to
_decodedImageCache
withcacheKey
.In addition, I didn't stored local image asset on
_decodedImageCache
because[UIImage imageNamed:]
work with its own cache.Test Plan
just make one simple web-server which provide static images with cache-control header.
set max-old or anything else and test that image was really caching during max-old time.
I've just tested with simple nodejs express server and checked it was perfectly working.