Respect Cache-Control headers #462

wiseoldduck · 2018-06-08T00:54:41Z

The PINCache upgrade bundled into this has also been submitted as a standalone PR

Update CHANGELOG

Correct selector [NSDate stopMockingDate]; Update .podspec

…into cache-control

appleguy

This is a really great improvement. Thanks for working on it, @wiseoldduck ! It definitely adds value to PINRemoteImage (in my opinion), as most sophisticated backends do set these headers.

@garrettmoon take a look when you have time — since we'll be merging this internally by Wednesday EOD, it would be ideal to have the final tweaks done. We can re-sync again later if it takes longer to review, though. Thanks!

appleguy · 2018-06-10T06:47:56Z

Source/Classes/PINRemoteImageManager.m

@@ -116,6 +116,7 @@ @interface PINRemoteImageManager () <PINURLSessionManagerDelegate>
 @property (nonatomic, copy) id<PINRequestRetryStrategy> (^retryStrategyCreationBlock)(void);
 @property (nonatomic, copy) PINRemoteImageManagerRequestConfigurationHandler requestConfigurationHandler;
 @property (nonatomic, strong) NSMutableDictionary <NSString *, NSString *> *httpHeaderFields;
+@property(nonatomic, assign) BOOL isTtlCache;


To match the codebase, space before ( -- which is not done in some other codebases.

I would suggest a different name, maybe respectCacheControlHeaders, trackImageExpiry, enforceCacheAgeLimits - since this class is not by itself a cache, it's more like a class managing a cache than "isTtlCache".

Spacing added.
Since the underlying cache is not necessarily PINCache, this var is storing (caching) whether the cache implements the optional - (void)setObjectOnDisk:(id)object forKey:(NSString *)key withAgeLimit:(NSTimeInterval)ageLimit method, that is whether it is a Ttl cache or not. So I think the name is accurate, (although maybe could be cacheIsTtlCache)...

appleguy · 2018-06-10T06:48:26Z

Source/Classes/PINRemoteImageManager.m

@@ -784,13 +786,62 @@ - (void)downloadImageWithURL:(NSURL *)url
            NSError *remoteImageError = error;
            PINImage *image = nil;
            id alternativeRepresentation = nil;
-             
+            __block NSNumber *maxAge = nil;


Any reason to box this as an NSNumber? If not, NSInteger should result shorter / faster code, as long as cases like NSNotFound or other sentinel values are handled.

Yes, we need to distinguish between "set to 0" and "not set" (nil)

appleguy · 2018-06-10T06:48:56Z

Source/Classes/PINRemoteImageManager.m

            if (remoteImageError == nil) {
-                 //stores the object in the caches
-                 [self materializeAndCacheObject:data cacheInDisk:data additionalCost:0 url:url key:key options:options outImage:&image outAltRep:&alternativeRepresentation];
+                if (_isTtlCache && !(options & PINRemoteImageManagerDownloadOptionsIgnoreCacheControlHeaders)) {


Believe it or not, this bitwise-AND should not be inverted / handled as a BOOL as it can fail on specifically 32-bit ARM! Our team had some nasty issues with this last Fall.

If other parts of the file have this bug as well, we should introduce a macro similar to ASInterfaceStateIncludes() or ASCellLayoutModeIncludes() to resolve.

lolwut Citation Needed

No I believe you of course but would love to learn more! The details would inform me how to rewrite this, is putting the x & y result into a BOOL first sufficient?

appleguy · 2018-06-10T06:49:20Z

Source/Classes/PINRemoteImageManager.m

+                    if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
+                        NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *) response;
+
+                        [[[httpResponse allHeaderFields][@"Cache-Control"] componentsSeparatedByString:@","] enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) {


Since these are complex lines of code, and we access this twice, local variable?

NSArray *allHeaderFields = ...

appleguy · 2018-06-10T06:50:06Z

Source/Classes/PINRemoteImageManager.m

+
+                            if ([trimmed isEqualToString:@"no-store"] || [trimmed isEqualToString:@"must-revalidate"] || [trimmed isEqualToString:@"no-cache"]) {
+                                maxAge = @(0);
+                                *stop = YES;


I'd suggest we just use for( ... in ...), as it is both faster - NSFastEnumeration and avoids the internal @autorelease pool that is otherwise created around every block invocation - and it also is a bit nicer looking (less block boilerplate with the arguments, easier to write "break";, etc)

This could be, for (NSString *field in [allHeaderFields[@"Cache-Control"] componentsSeparatedByString:@",") { }

appleguy · 2018-06-10T06:50:41Z

Source/Classes/PINRemoteImageManager.m

+                        if (!maxAge && (expires = [httpResponse allHeaderFields][@"Expires"])) {
+                            // https://developer.apple.com/library/archive/qa/qa1480/_index.html
+
+                            static NSDateFormatter *sRFC7231PreferredDateFormatter;


Assign nil. I'm not sure we should rely on static variables being zero-initialized.

Also, we should move this out to an accessor method. Ideally static variables are declared outside methods (even if just above the method). In this case a relatively complex method would be shortened if we moved the cached formatter to an accessor like +preferredCacheControlDateFormatter.

appleguy · 2018-06-10T06:51:00Z

Source/Classes/PINRemoteImageManager.m

+                        }
+                    }
+                }
+                //stores the object in the caches


Space & caps after //

appleguy · 2018-06-10T06:51:23Z

Source/Classes/PINRemoteImageManager.m

@@ -1356,7 +1419,15 @@ - (BOOL)materializeAndCacheObject:(id)object
    }

    if (diskData) {
-        [self.cache setObjectOnDisk:diskData forKey:key];
+        // maxAge of 0 is not stored at all


Could you clarify this comment? Does this mean "maxAge of 0 means that images should not be stored at all" ?

Is there a value for maxAge that means infinite?

Done (yes that's what it meant).
There is no header that can be sent to indicate "infinite", which is why you will see so many HTTP responses with values like "one year in the future". However not setting a value at all, which in our case is represented by maxAge == nil, does effectively mean that. (In our case it will use the cache-global maxAge)

appleguy · 2018-06-10T06:52:05Z

Source/Classes/PINRemoteImageManager.h

+    /**
+     * Do not honor HTTP Cache-Control headers
+     * Currently PINRemoteImage will by default respect 'no-store', 'no-cache', 'max-age', 'Expires', and 'must-revalidate'. Set this flag to ignore those headers.
+     * TODO: Currently PINRemoteImage will re-download images that only must be re-validated. In the future this could be improved with revalidation behavior that stores ETag or Last-Modified values and only


Minor / optional: "By default, PINRemoteImage will respect...".

Could also drop "Currently" in the next line, as presumably that will be updated if it becomes supported in the future. The next line also seems like it could use a line break to stay within a ~120 (or a bit more) column width — which isn't a hard rule for these frameworks, but up to ~150 works pretty well for Obj-C.

Great comment overall, thanks for including the IETF and Mozilla links!

appleguy · 2018-06-10T06:52:27Z

Tests/PINRemoteImageTests.m

@@ -943,6 +944,51 @@ - (void)testQOS
    XCTAssert(dispatch_semaphore_wait(semaphore, [self timeout]) == 0, @"Semaphore timed out.");
 }

+- (void)testMaxAge


Thanks for adding tests! Maybe call this testCacheControlMaxAgeExpiry, or just testCacheControl? Since the maxAge comes from cache control, the cache-control seems like the root nature / focus of the test.

"testCacheControlSupport"?

Whitespace

Reword `PINRemoteImageManagerDownloadOptionsIgnoreCacheControlHeaders` comment create static accessor for `RFC7231PreferredDateFormatter` Replace `enumerateObjectsUsingBlock` with `for (NSString *component in`... Rename test

Added a comment

garrettmoon · 2018-06-11T17:31:29Z

Source/Classes/PINCache/PINCache+PINRemoteImageCaching.m

+{
+    [self.diskCache setObject:object forKey:key withAgeLimit:ageLimit];
+}
+
 -(void)setObjectOnDisk:(id)object forKey:(NSString *)key
 {
    [self.diskCache setObject:object forKey:key];


Super minor: maybe have this call [self setObjectOnDisk:object forKey:key withAgeLimit:0];?

garrettmoon · 2018-06-11T21:41:39Z

Source/Classes/PINRemoteImageManager.m

        if ([key hasPrefix:PINRemoteImageCacheKeyResumePrefix]) {
            return [NSKeyedUnarchiver unarchiveObjectWithData:data];
        }
        return data;
-    }];
+    } keyEncoder:nil keyDecoder:nil ttlCache:YES];


Dang, so setting this to YES will cause a significant performance hit (PINDiskCache can't asynchronously startup), even if you don't end up using the feature. I'm wondering if we should actually have a flag on init which allows you to disable (or perhaps it should actually be off by default) support of TTL? We could possibly log warnings if we encounter a cache control header and this is NO.

Nooooooooooooooooooooooooo

garrettmoon · 2018-06-11T21:43:28Z

Source/Classes/PINRemoteImageManager.m

@@ -116,6 +116,7 @@ @interface PINRemoteImageManager () <PINURLSessionManagerDelegate>
 @property (nonatomic, copy) id<PINRequestRetryStrategy> (^retryStrategyCreationBlock)(void);
 @property (nonatomic, copy) PINRemoteImageManagerRequestConfigurationHandler requestConfigurationHandler;
 @property (nonatomic, strong) NSMutableDictionary <NSString *, NSString *> *httpHeaderFields;
+@property (nonatomic, assign) BOOL isTtlCache;


This should probably be readonly since it's accessed outside the lock and only set on init?

garrettmoon · 2018-06-11T21:46:14Z

Source/Classes/PINRemoteImageManager.m

            if (remoteImageError == nil) {
-                 //stores the object in the caches
-                 [self materializeAndCacheObject:data cacheInDisk:data additionalCost:0 url:url key:key options:options outImage:&image outAltRep:&alternativeRepresentation];
+                BOOL ignoreHeaders = (options & PINRemoteImageManagerDownloadOptionsIgnoreCacheControlHeaders) != 0;


nit: name this ignoreCacheHeaders

Would it be more readable to make a category on the response which returns maxAge? RFC7231PreferredDateFormatter could live there too?

garrettmoon · 2018-06-11T21:53:39Z

Source/Classes/PINRemoteImageManager.m

+                                    // and while I cannot find any explicit instruction of how to behave
+                                    // with a malformed "max-age" header, it seems like a reasonable approach.
+                                    maxAge = @([split[1] integerValue]);
+                                }


Since invalid means do not cache, should there be an else here setting maxAge to 0?

hmm well the else would be if [split count] == 0, in which case it was not set at all (and nil is correct), or [split count] > 2, in which case you are correct but it's a pretty weird case with a header like Cache-Control: maxage=maxage=444

I added a condition for the pretty weird case

garrettmoon · 2018-06-11T22:00:53Z

Source/Classes/PINRemoteImageManager.m

+        // maxAge of 0 means that images should not be stored at all.
+        // There is no HTTP header that can be sent to indicate "infinite". However not setting a value at all, which in
+        // our case is represented by maxAge == nil, effectively means that.
+        if (!(maxAge && [maxAge integerValue] == 0)) {


Is this more readable (honest question)? (I think it's still correct)

if (maxAge && [maxAge integerValue] != 0 && _isTtlCache) { [self.cache setObjectOnDisk:diskData forKey:key withAgeLimit:[maxAge integerValue]]; } else if (!maxAge) { // unset (nil) maxAge, or a cache that is not _isTtlCache behave as before (will use cache global ageLimit) [self.cache setObjectOnDisk:diskData forKey:key]; }

I dunno? Yes it looks still correct, and the first case is more readable, but it seems harder to reason through the other two possibilities...

No I think it is also incorrect in that if maxAge is set but _isTtlCache is false, the proposal fails to call the old method (does not cache at all)

I think this may suggest we should have a simple local variable with a more descriptive name, like:

BOOL neverStoreImage = (maxAge && [maxAge integerValue] == 0);
if (neverStoreImage == NO) {

}

There's probably a better variable name or another way to do it that is more readable still, like you could check:

BOOL storeImageIndefinitely = !maxAge;
if (storeImageIndefinitely || [maxAge integerValue] > 0) {

}

This second one might be the most readable?

Excellent suggestion! See the newest edition 👍

garrettmoon · 2018-06-11T22:05:46Z

Tests/PINRemoteImageTests.m

@@ -943,6 +944,51 @@ - (void)testQOS
    XCTAssert(dispatch_semaphore_wait(semaphore, [self timeout]) == 0, @"Semaphore timed out.");
 }

+- (void)testCacheControlSupport


I think these would be (slightly) better tests if they actually checked the results of a second download instead of just seeing if the objects are still in the cache. Or perhaps both? PINRemoteImage will tell you how it got an object in result.resultType.

OK.

Well it would be second & third downloads, to confirm that it got it from the cache and then did not.

This change will make more obvious another limitation - The PINCache TTL implementation only applies to the disk cache. If you kept an app alive long enough, without memory warnings, you could still get results from the memory cache that the header would have suggested you shouldn't.

isTtlCache is now readonly property setObjectOnDisk:forKey: uses setObjectOnDisk:forKey:withAgeLimit: weird case of multiple maxage=maxage=maxages covered

…sk cache

…rent startup impl of ttlCache) Provide + (nonnull id<PINRemoteImageCaching>)defaultImageTtlCache; to get a TTL cache if you want it Other code review updates: add local vars for readability in -[PINRemoteImageManager materializeAndCacheObject...

garrettmoon · 2018-06-13T19:58:17Z

Thanks so much for your contribution @wiseoldduck !

wiseoldduck force-pushed the cache-control branch from 090834c to 4549931 Compare June 8, 2018 17:37

Update Cartfile and update (PINCache)

34a842c

wiseoldduck force-pushed the cache-control branch from 4549931 to a5e7f7b Compare June 8, 2018 19:09

Respect Cache-Control headers

f047d3c

Update CHANGELOG

wiseoldduck force-pushed the cache-control branch from a5e7f7b to f047d3c Compare June 8, 2018 19:13

Cleanup and clarification

4534575

Correct selector [NSDate stopMockingDate]; Update .podspec

wiseoldduck force-pushed the cache-control branch from f645a38 to 4534575 Compare June 8, 2018 23:55

Merge branch 'master' of https://github.com/pinterest/PINRemoteImage …

9a79eba

…into cache-control

appleguy reviewed Jun 10, 2018

View reviewed changes

wiseoldduck added 4 commits June 10, 2018 12:18

Update PINRemoteImageManager.m

ba0e9a8

Whitespace

Code review updates

c5fad07

Reword `PINRemoteImageManagerDownloadOptionsIgnoreCacheControlHeaders` comment create static accessor for `RFC7231PreferredDateFormatter` Replace `enumerateObjectsUsingBlock` with `for (NSString *component in`... Rename test

extract ignoreHeaders into an explicit BOOL

b1ae25f

Code review updates

4dca22a

Added a comment

wiseoldduck force-pushed the cache-control branch from 08b54d0 to 4dca22a Compare June 11, 2018 17:59

pinterest deleted a comment Jun 11, 2018

garrettmoon reviewed Jun 11, 2018

View reviewed changes

wiseoldduck added 4 commits June 11, 2018 19:07

Code review updates

c3eb246

isTtlCache is now readonly property setObjectOnDisk:forKey: uses setObjectOnDisk:forKey:withAgeLimit: weird case of multiple maxage=maxage=maxages covered

Update test to verify download result types as well as presence in di…

eacd424

…sk cache

memoryCache does support TTL! Let's use it.

86b9ece

garrettmoon approved these changes Jun 13, 2018

View reviewed changes

garrettmoon merged commit 65f564b into pinterest:master Jun 13, 2018

rainypixels mentioned this pull request Apr 3, 2019

Allow clients to configure URLSession URLCache if they choose. #477

Merged

Respect Cache-Control headers #462

Respect Cache-Control headers #462

Conversation

wiseoldduck commented Jun 8, 2018

appleguy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettmoon commented Jun 13, 2018

appleguy left a comment •

edited

Loading