From 1b954b010bc5db24466c5ab91706e33f4bed4513 Mon Sep 17 00:00:00 2001 From: zhang Date: Thu, 1 Dec 2016 11:21:04 +0800 Subject: [PATCH 1/3] clear image downloading cache policy to fix issue #1623 --- SDWebImage/SDWebImageDownloader.h | 1 + SDWebImage/SDWebImageDownloader.m | 11 +++++++++- SDWebImage/SDWebImageDownloaderOperation.m | 25 +++++++++++++++------- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/SDWebImage/SDWebImageDownloader.h b/SDWebImage/SDWebImageDownloader.h index 94bc76e5..724aef74 100644 --- a/SDWebImage/SDWebImageDownloader.h +++ b/SDWebImage/SDWebImageDownloader.h @@ -23,6 +23,7 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageDownloaderOptions) { /** * Call completion block with nil image/imageData if the image was read from NSURLCache * (to be combined with `SDWebImageDownloaderUseNSURLCache`). + * I think this option should be renamed to 'SDWebImageDownloaderUsingCachedResponseDontLoad' */ SDWebImageDownloaderIgnoreCachedResponse = 1 << 3, diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index 189d1630..84c02753 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -153,7 +153,16 @@ } // In order to prevent from potential duplicate caching (NSURLCache + SDImageCache) we disable the cache for image requests if told otherwise - NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:url cachePolicy:(options & SDWebImageDownloaderUseNSURLCache ? NSURLRequestUseProtocolCachePolicy : NSURLRequestReloadIgnoringLocalCacheData) timeoutInterval:timeoutInterval]; + NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:url cachePolicy:NSURLRequestReloadIgnoringLocalCacheData timeoutInterval:timeoutInterval]; + if (options & SDWebImageDownloaderUseNSURLCache) { + if (options & SDWebImageDownloaderIgnoreCachedResponse) { + request.cachePolicy = NSURLRequestReturnCacheDataDontLoad; + } + else { + request.cachePolicy = NSURLRequestUseProtocolCachePolicy; + } + } + request.HTTPShouldHandleCookies = (options & SDWebImageDownloaderHandleCookies); request.HTTPShouldUsePipelining = YES; if (sself.headersFilter) { diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index 63df1afe..d54a104e 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -52,7 +52,8 @@ typedef NSMutableDictionary SDCallbacksDictionary; #if SD_UIKIT || SD_WATCH UIImageOrientation orientation; #endif - BOOL responseFromCached; + //useless now +// BOOL responseFromCached; } @synthesize executing = _executing; @@ -74,7 +75,7 @@ typedef NSMutableDictionary SDCallbacksDictionary; _finished = NO; _expectedSize = 0; _unownedSession = session; - responseFromCached = YES; // Initially wrong until `- URLSession:dataTask:willCacheResponse:completionHandler: is called or not called +// responseFromCached = YES; // Initially wrong until `- URLSession:dataTask:willCacheResponse:completionHandler: is called or not called _barrierQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderOperationBarrierQueue", DISPATCH_QUEUE_CONCURRENT); } return self; @@ -387,7 +388,7 @@ didReceiveResponse:(NSURLResponse *)response willCacheResponse:(NSCachedURLResponse *)proposedResponse completionHandler:(void (^)(NSCachedURLResponse *cachedResponse))completionHandler { - responseFromCached = NO; // If this method is called, it means the response wasn't read from cache +// responseFromCached = NO; // If this method is called, it means the response wasn't read from cache NSCachedURLResponse *cachedResponse = proposedResponse; if (self.request.cachePolicy == NSURLRequestReloadIgnoringLocalCacheData) { @@ -422,10 +423,18 @@ didReceiveResponse:(NSURLResponse *)response * and images for which responseFromCached is YES (only the ones that cannot be cached). * Note: responseFromCached is set to NO inside `willCacheResponse:`. This method doesn't get called for large images or images behind authentication */ - if (self.options & SDWebImageDownloaderIgnoreCachedResponse && responseFromCached && [[NSURLCache sharedURLCache] cachedResponseForRequest:self.request]) { - // hack - [self callCompletionBlocksWithImage:nil imageData:nil error:nil finished:YES]; - } else if (self.imageData) { + + /** + If you specified to use `NSURLCache`, then the response you get here is what you need. + if you specified to only use cached data via `SDWebImageDownloaderIgnoreCachedResponse`(This name is confusing), + the response data will be nil. + So we don't need to check the cache option here, because we have set the cache option of the request already, and system will obey it. + */ + +// if (self.options & SDWebImageDownloaderUseNSURLCache) { +// // hack +// [self callCompletionBlocksWithImage:nil imageData:nil error:nil finished:YES]; +// } else if (self.imageData) { UIImage *image = [UIImage sd_imageWithData:self.imageData]; NSString *key = [[SDWebImageManager sharedManager] cacheKeyForURL:self.request.URL]; image = [self scaledImageForKey:key image:image]; @@ -451,7 +460,7 @@ didReceiveResponse:(NSURLResponse *)response } else { [self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}]]; } - } +// } } [self done]; } From d40012491c0bb2a6405155bffdc19fd7abec06c6 Mon Sep 17 00:00:00 2001 From: zhang Date: Thu, 1 Dec 2016 11:39:46 +0800 Subject: [PATCH 2/3] restore a if statement checking --- SDWebImage/SDWebImageDownloaderOperation.m | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index d54a104e..b2eaaeaa 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -430,11 +430,7 @@ didReceiveResponse:(NSURLResponse *)response the response data will be nil. So we don't need to check the cache option here, because we have set the cache option of the request already, and system will obey it. */ - -// if (self.options & SDWebImageDownloaderUseNSURLCache) { -// // hack -// [self callCompletionBlocksWithImage:nil imageData:nil error:nil finished:YES]; -// } else if (self.imageData) { + if (self.imageData) { UIImage *image = [UIImage sd_imageWithData:self.imageData]; NSString *key = [[SDWebImageManager sharedManager] cacheKeyForURL:self.request.URL]; image = [self scaledImageForKey:key image:image]; @@ -460,7 +456,7 @@ didReceiveResponse:(NSURLResponse *)response } else { [self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}]]; } -// } + } } [self done]; } From d02c58f1f71d1a5caf91c4ba3eb79a4ecaaf45f0 Mon Sep 17 00:00:00 2001 From: Bogdan Poplauschi Date: Fri, 10 Mar 2017 12:38:37 +0200 Subject: [PATCH 3/3] Code review and cleanup for #1737 --- SDWebImage/SDWebImageDownloader.h | 3 +-- SDWebImage/SDWebImageDownloader.m | 11 ++++++----- SDWebImage/SDWebImageDownloaderOperation.m | 21 +++++---------------- 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/SDWebImage/SDWebImageDownloader.h b/SDWebImage/SDWebImageDownloader.h index 724aef74..557c0af5 100644 --- a/SDWebImage/SDWebImageDownloader.h +++ b/SDWebImage/SDWebImageDownloader.h @@ -25,13 +25,12 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageDownloaderOptions) { * (to be combined with `SDWebImageDownloaderUseNSURLCache`). * I think this option should be renamed to 'SDWebImageDownloaderUsingCachedResponseDontLoad' */ - SDWebImageDownloaderIgnoreCachedResponse = 1 << 3, + /** * In iOS 4+, continue the download of the image if the app goes to background. This is achieved by asking the system for * extra time in background to let the request finish. If the background task expires the operation will be cancelled. */ - SDWebImageDownloaderContinueInBackground = 1 << 4, /** diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index 84c02753..f6481612 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -153,16 +153,17 @@ } // In order to prevent from potential duplicate caching (NSURLCache + SDImageCache) we disable the cache for image requests if told otherwise - NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:url cachePolicy:NSURLRequestReloadIgnoringLocalCacheData timeoutInterval:timeoutInterval]; + NSURLRequestCachePolicy cachePolicy = NSURLRequestReloadIgnoringLocalCacheData; if (options & SDWebImageDownloaderUseNSURLCache) { if (options & SDWebImageDownloaderIgnoreCachedResponse) { - request.cachePolicy = NSURLRequestReturnCacheDataDontLoad; - } - else { - request.cachePolicy = NSURLRequestUseProtocolCachePolicy; + cachePolicy = NSURLRequestReturnCacheDataDontLoad; + } else { + cachePolicy = NSURLRequestUseProtocolCachePolicy; } } + NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:url cachePolicy:cachePolicy timeoutInterval:timeoutInterval]; + request.HTTPShouldHandleCookies = (options & SDWebImageDownloaderHandleCookies); request.HTTPShouldUsePipelining = YES; if (sself.headersFilter) { diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index b2eaaeaa..df1366e1 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -52,8 +52,6 @@ typedef NSMutableDictionary SDCallbacksDictionary; #if SD_UIKIT || SD_WATCH UIImageOrientation orientation; #endif - //useless now -// BOOL responseFromCached; } @synthesize executing = _executing; @@ -75,7 +73,6 @@ typedef NSMutableDictionary SDCallbacksDictionary; _finished = NO; _expectedSize = 0; _unownedSession = session; -// responseFromCached = YES; // Initially wrong until `- URLSession:dataTask:willCacheResponse:completionHandler: is called or not called _barrierQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderOperationBarrierQueue", DISPATCH_QUEUE_CONCURRENT); } return self; @@ -387,8 +384,7 @@ didReceiveResponse:(NSURLResponse *)response dataTask:(NSURLSessionDataTask *)dataTask willCacheResponse:(NSCachedURLResponse *)proposedResponse completionHandler:(void (^)(NSCachedURLResponse *cachedResponse))completionHandler { - -// responseFromCached = NO; // If this method is called, it means the response wasn't read from cache + NSCachedURLResponse *cachedResponse = proposedResponse; if (self.request.cachePolicy == NSURLRequestReloadIgnoringLocalCacheData) { @@ -418,17 +414,10 @@ didReceiveResponse:(NSURLResponse *)response } else { if ([self callbacksForKey:kCompletedCallbackKey].count > 0) { /** - * See #1608 and #1623 - apparently, there is a race condition on `NSURLCache` that causes a crash - * Limited the calls to `cachedResponseForRequest:` only for cases where we should ignore the cached response - * and images for which responseFromCached is YES (only the ones that cannot be cached). - * Note: responseFromCached is set to NO inside `willCacheResponse:`. This method doesn't get called for large images or images behind authentication - */ - - /** - If you specified to use `NSURLCache`, then the response you get here is what you need. - if you specified to only use cached data via `SDWebImageDownloaderIgnoreCachedResponse`(This name is confusing), - the response data will be nil. - So we don't need to check the cache option here, because we have set the cache option of the request already, and system will obey it. + * If you specified to use `NSURLCache`, then the response you get here is what you need. + * if you specified to only use cached data via `SDWebImageDownloaderIgnoreCachedResponse`, + * the response data will be nil. + * So we don't need to check the cache option here, since the system will obey the cache option */ if (self.imageData) { UIImage *image = [UIImage sd_imageWithData:self.imageData];