From 917c3d479dd67de8ad73bf5f3f9149c5af7c60cc Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Mon, 1 Sep 2014 11:11:27 +0100 Subject: [PATCH 1/7] Fix multiple requests for same image and then canceling one --- SDWebImage/SDWebImageDownloader.h | 17 +++-- SDWebImage/SDWebImageDownloader.m | 44 ++++++++++-- SDWebImage/SDWebImageManager.m | 6 +- .../project.pbxproj | 4 ++ Tests/Tests/SDWebImageDownloaderTests.m | 70 +++++++++++++++++++ 5 files changed, 126 insertions(+), 15 deletions(-) create mode 100644 Tests/Tests/SDWebImageDownloaderTests.m diff --git a/SDWebImage/SDWebImageDownloader.h b/SDWebImage/SDWebImageDownloader.h index b64fb130..b11d4f48 100644 --- a/SDWebImage/SDWebImageDownloader.h +++ b/SDWebImage/SDWebImageDownloader.h @@ -176,12 +176,19 @@ typedef NSDictionary *(^SDWebImageDownloaderHeadersFilterBlock)(NSURL *url, NSDi * before to be called a last time with the full image and finished argument * set to YES. In case of error, the finished argument is always YES. * - * @return A cancellable SDWebImageOperation + * @return A token that can be passed to -cancel: to cancel this operation */ -- (id )downloadImageWithURL:(NSURL *)url - options:(SDWebImageDownloaderOptions)options - progress:(SDWebImageDownloaderProgressBlock)progressBlock - completed:(SDWebImageDownloaderCompletedBlock)completedBlock; +- (id)downloadImageWithURL:(NSURL *)url + options:(SDWebImageDownloaderOptions)options + progress:(SDWebImageDownloaderProgressBlock)progressBlock + completed:(SDWebImageDownloaderCompletedBlock)completedBlock; + +/** + * Cancels a download that was previously queued using -downloadImageWithURL:options:progress:completed: + * + * @param token The token received from -downloadImageWithURL:options:progress:completed: that should be canceled. + */ +- (void)cancel:(id)token; /** * Sets the download queue suspension state diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index 1fdcfc92..23fe351e 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -13,6 +13,16 @@ static NSString *const kProgressCallbackKey = @"progress"; static NSString *const kCompletedCallbackKey = @"completed"; +@interface _SDWebImageDownloaderToken : NSObject + +@property (nonatomic, strong) NSURL *url; +@property (nonatomic, strong) id callbacks; + +@end + +@implementation _SDWebImageDownloaderToken +@end + @interface SDWebImageDownloader () @property (strong, nonatomic) NSOperationQueue *downloadQueue; @@ -112,11 +122,11 @@ static NSString *const kCompletedCallbackKey = @"completed"; _operationClass = operationClass ?: [SDWebImageDownloaderOperation class]; } -- (id )downloadImageWithURL:(NSURL *)url options:(SDWebImageDownloaderOptions)options progress:(SDWebImageDownloaderProgressBlock)progressBlock completed:(SDWebImageDownloaderCompletedBlock)completedBlock { +- (id)downloadImageWithURL:(NSURL *)url options:(SDWebImageDownloaderOptions)options progress:(SDWebImageDownloaderProgressBlock)progressBlock completed:(SDWebImageDownloaderCompletedBlock)completedBlock { __block SDWebImageDownloaderOperation *operation; - __weak __typeof(self)wself = self; + __weak SDWebImageDownloader *wself = self; - [self addProgressCallback:progressBlock completedBlock:completedBlock forURL:url createCallback:^{ + return [self addProgressCallback:progressBlock completedBlock:completedBlock forURL:url createCallback:^SDWebImageDownloaderOperation *{ NSTimeInterval timeoutInterval = wself.downloadTimeout; if (timeoutInterval == 0.0) { timeoutInterval = 15.0; @@ -190,20 +200,34 @@ static NSString *const kCompletedCallbackKey = @"completed"; [wself.lastAddedOperation addDependency:operation]; wself.lastAddedOperation = operation; } - }]; - return operation; + return operation; + }]; } -- (void)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock forURL:(NSURL *)url createCallback:(SDWebImageNoParamsBlock)createCallback { +- (void)cancel:(id)token { + if (![token isKindOfClass:[_SDWebImageDownloaderToken class]]) { + return; + } + + dispatch_barrier_async(self.barrierQueue, ^{ + _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; + NSMutableArray *callbacksForURL = self.URLCallbacks[typedToken.url]; + [callbacksForURL removeObjectIdenticalTo:typedToken.callbacks]; + }); +} + +- (id)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock forURL:(NSURL *)url createCallback:(SDWebImageDownloaderOperation *(^)())createCallback { // The URL will be used as the key to the callbacks dictionary so it cannot be nil. If it is nil immediately call the completed block with no image or data. if (url == nil) { if (completedBlock != nil) { completedBlock(nil, nil, nil, NO); } - return; + return nil; } + __block _SDWebImageDownloaderToken *token = nil; + dispatch_barrier_sync(self.barrierQueue, ^{ BOOL first = NO; if (!self.URLCallbacks[url]) { @@ -222,7 +246,13 @@ static NSString *const kCompletedCallbackKey = @"completed"; if (first) { createCallback(); } + + token = [_SDWebImageDownloaderToken new]; + token.url = url; + token.callbacks = callbacks; }); + + return token; } - (void)setSuspended:(BOOL)suspended { diff --git a/SDWebImage/SDWebImageManager.m b/SDWebImage/SDWebImageManager.m index c804ee9a..81f68280 100644 --- a/SDWebImage/SDWebImageManager.m +++ b/SDWebImage/SDWebImageManager.m @@ -179,7 +179,7 @@ // ignore image read from NSURLCache if image if cached but force refreshing downloaderOptions |= SDWebImageDownloaderIgnoreCachedResponse; } - id subOperation = [self.imageDownloader downloadImageWithURL:url options:downloaderOptions progress:progressBlock completed:^(UIImage *downloadedImage, NSData *data, NSError *error, BOOL finished) { + id subOperation = [self.imageDownloader downloadImageWithURL:url options:downloaderOptions progress:progressBlock completed:^(UIImage *downloadedImage, NSData *data, NSError *error, BOOL finished) { __strong __typeof(weakOperation) strongOperation = weakOperation; if (!strongOperation || strongOperation.isCancelled) { // Do nothing if the operation was cancelled @@ -255,8 +255,8 @@ } }]; operation.cancelBlock = ^{ - [subOperation cancel]; - + [self.imageDownloader cancel:subOperation]; + @synchronized (self.runningOperations) { __strong __typeof(weakOperation) strongOperation = weakOperation; if (strongOperation) { diff --git a/Tests/SDWebImage Tests.xcodeproj/project.pbxproj b/Tests/SDWebImage Tests.xcodeproj/project.pbxproj index 34a8e11b..31cb7118 100644 --- a/Tests/SDWebImage Tests.xcodeproj/project.pbxproj +++ b/Tests/SDWebImage Tests.xcodeproj/project.pbxproj @@ -8,6 +8,7 @@ /* Begin PBXBuildFile section */ 5F7F38AD1AE2A77A00B0E330 /* TestImage.jpg in Resources */ = {isa = PBXBuildFile; fileRef = 5F7F38AC1AE2A77A00B0E330 /* TestImage.jpg */; }; + 1E3C51E919B46E370092B5E6 /* SDWebImageDownloaderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 1E3C51E819B46E370092B5E6 /* SDWebImageDownloaderTests.m */; }; ABC8501F672447AA91C788DA /* libPods-ios.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EB0D107E6B4C4094BA2FEE29 /* libPods-ios.a */; }; DA248D57195472AA00390AB0 /* XCTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DA248D56195472AA00390AB0 /* XCTest.framework */; }; DA248D59195472AA00390AB0 /* Foundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DA248D58195472AA00390AB0 /* Foundation.framework */; }; @@ -22,6 +23,7 @@ 1A6DF883515E8008203AB352 /* Pods-ios.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-ios.debug.xcconfig"; path = "Pods/Target Support Files/Pods-ios/Pods-ios.debug.xcconfig"; sourceTree = ""; }; 5F7F38AC1AE2A77A00B0E330 /* TestImage.jpg */ = {isa = PBXFileReference; lastKnownFileType = image.jpeg; path = TestImage.jpg; sourceTree = ""; }; CA88E6BDE3581B2BFE933C10 /* Pods-ios.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-ios.release.xcconfig"; path = "Pods/Target Support Files/Pods-ios/Pods-ios.release.xcconfig"; sourceTree = ""; }; + 1E3C51E819B46E370092B5E6 /* SDWebImageDownloaderTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SDWebImageDownloaderTests.m; sourceTree = ""; }; DA248D53195472AA00390AB0 /* Tests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = Tests.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; DA248D56195472AA00390AB0 /* XCTest.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = XCTest.framework; path = Library/Frameworks/XCTest.framework; sourceTree = DEVELOPER_DIR; }; DA248D58195472AA00390AB0 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = Library/Frameworks/Foundation.framework; sourceTree = DEVELOPER_DIR; }; @@ -96,6 +98,7 @@ DA248D68195475D800390AB0 /* SDImageCacheTests.m */, DA248D6A195476AC00390AB0 /* SDWebImageManagerTests.m */, DA91BEBB19795BC9006F2536 /* UIImageMultiFormatTests.m */, + 1E3C51E819B46E370092B5E6 /* SDWebImageDownloaderTests.m */, ); path = Tests; sourceTree = ""; @@ -207,6 +210,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( + 1E3C51E919B46E370092B5E6 /* SDWebImageDownloaderTests.m in Sources */, DA248D69195475D800390AB0 /* SDImageCacheTests.m in Sources */, DA248D6B195476AC00390AB0 /* SDWebImageManagerTests.m in Sources */, DA91BEBC19795BC9006F2536 /* UIImageMultiFormatTests.m in Sources */, diff --git a/Tests/Tests/SDWebImageDownloaderTests.m b/Tests/Tests/SDWebImageDownloaderTests.m new file mode 100644 index 00000000..972e8faf --- /dev/null +++ b/Tests/Tests/SDWebImageDownloaderTests.m @@ -0,0 +1,70 @@ +// +// SDWebImageDownloaderTests.m +// SDWebImage Tests +// +// Created by Matt Galloway on 01/09/2014. +// +// + +#define EXP_SHORTHAND // required by Expecta + + +#import +#import + +#import "SDWebImageDownloader.h" + +@interface SDWebImageDownloaderTests : XCTestCase + +@end + +@implementation SDWebImageDownloaderTests + +- (void)setUp +{ + [super setUp]; + // Put setup code here. This method is called before the invocation of each test method in the class. +} + +- (void)tearDown +{ + // Put teardown code here. This method is called after the invocation of each test method in the class. + [super tearDown]; +} + +- (void)testThatDownloadingSameURLTwiceAndCancellingFirstWorks { + NSURL *imageURL = [NSURL URLWithString:@"http://static2.dmcdn.net/static/video/656/177/44771656:jpeg_preview_small.jpg?20120509154705"]; + + id token1 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL + options:0 + progress:nil + completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { + XCTFail(@"Shouldn't have completed here."); + }]; + expect(token1).toNot.beNil(); + + __block BOOL success = NO; + id token2 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL + options:0 + progress:nil + completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { + success = YES; + }]; + expect(token2).toNot.beNil(); + + [[SDWebImageDownloader sharedDownloader] cancel:token1]; + + CFTimeInterval timeoutDate = CACurrentMediaTime() + 5.; + while (true) { + if (CACurrentMediaTime() > timeoutDate || success) { + break; + } + CFRunLoopRunInMode(kCFRunLoopDefaultMode, 1., true); + } + + if (!success) { + XCTFail(@"Failed to download image"); + } +} + +@end From 52f7204c3482139d9a328bb162e185dd4bf0f3c3 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Mon, 1 Sep 2014 11:19:21 +0100 Subject: [PATCH 2/7] Cancel the download operation if it's the last token to be canceled --- SDWebImage/SDWebImageDownloader.m | 33 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index 23fe351e..6242774c 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -29,6 +29,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; @property (weak, nonatomic) NSOperation *lastAddedOperation; @property (assign, nonatomic) Class operationClass; @property (strong, nonatomic) NSMutableDictionary *URLCallbacks; +@property (strong, nonatomic) NSMutableDictionary *URLOperations; @property (strong, nonatomic) NSMutableDictionary *HTTPHeaders; // This queue is used to serialize the handling of the network responses of all the download operation in a single queue @property (SDDispatchQueueSetterSementics, nonatomic) dispatch_queue_t barrierQueue; @@ -77,6 +78,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; _downloadQueue = [NSOperationQueue new]; _downloadQueue.maxConcurrentOperationCount = 6; _URLCallbacks = [NSMutableDictionary new]; + _URLOperations = [NSMutableDictionary new]; #ifdef SD_WEBP _HTTPHeaders = [@{@"Accept": @"image/webp,image/*;q=0.8"} mutableCopy]; #else @@ -206,15 +208,22 @@ static NSString *const kCompletedCallbackKey = @"completed"; } - (void)cancel:(id)token { - if (![token isKindOfClass:[_SDWebImageDownloaderToken class]]) { - return; - } + if (![token isKindOfClass:[_SDWebImageDownloaderToken class]]) { + return; + } - dispatch_barrier_async(self.barrierQueue, ^{ - _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; - NSMutableArray *callbacksForURL = self.URLCallbacks[typedToken.url]; - [callbacksForURL removeObjectIdenticalTo:typedToken.callbacks]; - }); + dispatch_barrier_async(self.barrierQueue, ^{ + _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; + + NSMutableArray *callbacksForURL = self.URLCallbacks[typedToken.url]; + [callbacksForURL removeObjectIdenticalTo:typedToken.callbacks]; + + // If this was the last set of callbacks, then cancel the operation + if (callbacksForURL.count == 0) { + SDWebImageDownloaderOperation *operation = self.URLOperations[typedToken.url]; + [operation cancel]; + } + }); } - (id)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock forURL:(NSURL *)url createCallback:(SDWebImageDownloaderOperation *(^)())createCallback { @@ -229,10 +238,8 @@ static NSString *const kCompletedCallbackKey = @"completed"; __block _SDWebImageDownloaderToken *token = nil; dispatch_barrier_sync(self.barrierQueue, ^{ - BOOL first = NO; if (!self.URLCallbacks[url]) { self.URLCallbacks[url] = [NSMutableArray new]; - first = YES; } // Handle single download of simultaneous download request for the same URL @@ -243,8 +250,10 @@ static NSString *const kCompletedCallbackKey = @"completed"; [callbacksForURL addObject:callbacks]; self.URLCallbacks[url] = callbacksForURL; - if (first) { - createCallback(); + SDWebImageDownloaderOperation *operation = self.URLOperations[url]; + if (!operation) { + operation = createCallback(); + self.URLOperations[url] = operation; } token = [_SDWebImageDownloaderToken new]; From b00b09e70627a78e2af31c937356cb9136993bc3 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Fri, 5 Sep 2014 11:43:30 +0100 Subject: [PATCH 3/7] Move progress / completion handler logic to the operation --- SDWebImage/SDWebImageDownloader.m | 105 +++++---------------- SDWebImage/SDWebImageDownloaderOperation.h | 33 +++++-- SDWebImage/SDWebImageDownloaderOperation.m | 74 ++++++++------- Tests/Tests/SDWebImageDownloaderTests.m | 53 +++++++++-- 4 files changed, 138 insertions(+), 127 deletions(-) diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index 6242774c..f63c95ee 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -10,13 +10,10 @@ #import "SDWebImageDownloaderOperation.h" #import -static NSString *const kProgressCallbackKey = @"progress"; -static NSString *const kCompletedCallbackKey = @"completed"; - @interface _SDWebImageDownloaderToken : NSObject @property (nonatomic, strong) NSURL *url; -@property (nonatomic, strong) id callbacks; +@property (nonatomic, strong) id downloadOperationCancelToken; @end @@ -28,7 +25,6 @@ static NSString *const kCompletedCallbackKey = @"completed"; @property (strong, nonatomic) NSOperationQueue *downloadQueue; @property (weak, nonatomic) NSOperation *lastAddedOperation; @property (assign, nonatomic) Class operationClass; -@property (strong, nonatomic) NSMutableDictionary *URLCallbacks; @property (strong, nonatomic) NSMutableDictionary *URLOperations; @property (strong, nonatomic) NSMutableDictionary *HTTPHeaders; // This queue is used to serialize the handling of the network responses of all the download operation in a single queue @@ -77,7 +73,6 @@ static NSString *const kCompletedCallbackKey = @"completed"; _executionOrder = SDWebImageDownloaderFIFOExecutionOrder; _downloadQueue = [NSOperationQueue new]; _downloadQueue.maxConcurrentOperationCount = 6; - _URLCallbacks = [NSMutableDictionary new]; _URLOperations = [NSMutableDictionary new]; #ifdef SD_WEBP _HTTPHeaders = [@{@"Accept": @"image/webp,image/*;q=0.8"} mutableCopy]; @@ -125,7 +120,6 @@ static NSString *const kCompletedCallbackKey = @"completed"; } - (id)downloadImageWithURL:(NSURL *)url options:(SDWebImageDownloaderOptions)options progress:(SDWebImageDownloaderProgressBlock)progressBlock completed:(SDWebImageDownloaderCompletedBlock)completedBlock { - __block SDWebImageDownloaderOperation *operation; __weak SDWebImageDownloader *wself = self; return [self addProgressCallback:progressBlock completedBlock:completedBlock forURL:url createCallback:^SDWebImageDownloaderOperation *{ @@ -144,44 +138,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; else { request.allHTTPHeaderFields = wself.HTTPHeaders; } - operation = [[wself.operationClass alloc] initWithRequest:request - options:options - progress:^(NSInteger receivedSize, NSInteger expectedSize) { - SDWebImageDownloader *sself = wself; - if (!sself) return; - __block NSArray *callbacksForURL; - dispatch_sync(sself.barrierQueue, ^{ - callbacksForURL = [sself.URLCallbacks[url] copy]; - }); - for (NSDictionary *callbacks in callbacksForURL) { - dispatch_async(dispatch_get_main_queue(), ^{ - SDWebImageDownloaderProgressBlock callback = callbacks[kProgressCallbackKey]; - if (callback) callback(receivedSize, expectedSize); - }); - } - } - completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { - SDWebImageDownloader *sself = wself; - if (!sself) return; - __block NSArray *callbacksForURL; - dispatch_barrier_sync(sself.barrierQueue, ^{ - callbacksForURL = [sself.URLCallbacks[url] copy]; - if (finished) { - [sself.URLCallbacks removeObjectForKey:url]; - } - }); - for (NSDictionary *callbacks in callbacksForURL) { - SDWebImageDownloaderCompletedBlock callback = callbacks[kCompletedCallbackKey]; - if (callback) callback(image, data, error, finished); - } - } - cancelled:^{ - SDWebImageDownloader *sself = wself; - if (!sself) return; - dispatch_barrier_async(sself.barrierQueue, ^{ - [sself.URLCallbacks removeObjectForKey:url]; - }); - }]; + SDWebImageDownloaderOperation *operation = [[wself.operationClass alloc] initWithRequest:request options:options]; operation.shouldDecompressImages = wself.shouldDecompressImages; if (wself.urlCredential) { @@ -212,18 +169,12 @@ static NSString *const kCompletedCallbackKey = @"completed"; return; } - dispatch_barrier_async(self.barrierQueue, ^{ - _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; - - NSMutableArray *callbacksForURL = self.URLCallbacks[typedToken.url]; - [callbacksForURL removeObjectIdenticalTo:typedToken.callbacks]; - - // If this was the last set of callbacks, then cancel the operation - if (callbacksForURL.count == 0) { - SDWebImageDownloaderOperation *operation = self.URLOperations[typedToken.url]; - [operation cancel]; - } - }); + _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; + SDWebImageDownloaderOperation *operation = self.URLOperations[typedToken.url]; + BOOL canceled = [operation cancel:typedToken.downloadOperationCancelToken]; + if (canceled) { + [self.URLOperations removeObjectForKey:typedToken.url]; + } } - (id)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock forURL:(NSURL *)url createCallback:(SDWebImageDownloaderOperation *(^)())createCallback { @@ -235,31 +186,25 @@ static NSString *const kCompletedCallbackKey = @"completed"; return nil; } - __block _SDWebImageDownloaderToken *token = nil; + SDWebImageDownloaderOperation *operation = self.URLOperations[url]; + if (!operation) { + operation = createCallback(); + self.URLOperations[url] = operation; - dispatch_barrier_sync(self.barrierQueue, ^{ - if (!self.URLCallbacks[url]) { - self.URLCallbacks[url] = [NSMutableArray new]; - } + __weak SDWebImageDownloaderOperation *woperation = operation; + operation.completionBlock = ^{ + SDWebImageDownloaderOperation *soperation = woperation; + if (!soperation) return; + if (self.URLOperations[url] == soperation) { + [self.URLOperations removeObjectForKey:url]; + }; + }; + } + id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock]; - // Handle single download of simultaneous download request for the same URL - NSMutableArray *callbacksForURL = self.URLCallbacks[url]; - NSMutableDictionary *callbacks = [NSMutableDictionary new]; - if (progressBlock) callbacks[kProgressCallbackKey] = [progressBlock copy]; - if (completedBlock) callbacks[kCompletedCallbackKey] = [completedBlock copy]; - [callbacksForURL addObject:callbacks]; - self.URLCallbacks[url] = callbacksForURL; - - SDWebImageDownloaderOperation *operation = self.URLOperations[url]; - if (!operation) { - operation = createCallback(); - self.URLOperations[url] = operation; - } - - token = [_SDWebImageDownloaderToken new]; - token.url = url; - token.callbacks = callbacks; - }); + _SDWebImageDownloaderToken *token = [_SDWebImageDownloaderToken new]; + token.url = url; + token.downloadOperationCancelToken = downloadOperationCancelToken; return token; } diff --git a/SDWebImage/SDWebImageDownloaderOperation.h b/SDWebImage/SDWebImageDownloaderOperation.h index dd48b228..68abec23 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.h +++ b/SDWebImage/SDWebImageDownloaderOperation.h @@ -61,18 +61,33 @@ extern NSString *const SDWebImageDownloadFinishNotification; * * @param request the URL request * @param options downloader options - * @param progressBlock the block executed when a new chunk of data arrives. - * @note the progress block is executed on a background queue - * @param completedBlock the block executed when the download is done. - * @note the completed block is executed on the main queue for success. If errors are found, there is a chance the block will be executed on a background queue - * @param cancelBlock the block executed if the download (operation) is cancelled * * @return the initialized instance */ - (id)initWithRequest:(NSURLRequest *)request - options:(SDWebImageDownloaderOptions)options - progress:(SDWebImageDownloaderProgressBlock)progressBlock - completed:(SDWebImageDownloaderCompletedBlock)completedBlock - cancelled:(SDWebImageNoParamsBlock)cancelBlock; + options:(SDWebImageDownloaderOptions)options; + +/** + * Adds handlers for progress and completion. Returns a tokent that can be passed to -cancel: to cancel this set of + * callbacks. + * + * @param progressBlock the block executed when a new chunk of data arrives. + * @note the progress block is executed on a background queue + * @param completedBlock the block executed when the download is done. + * @note the completed block is executed on the main queue for success. If errors are found, there is a chance the block will be executed on a background queue + * + * @return the token to use to cancel this set of handlers + */ +- (id)addHandlersForProgress:(SDWebImageDownloaderProgressBlock)progressBlock + completed:(SDWebImageDownloaderCompletedBlock)completedBlock; + +/** + * Cancels a set of callbacks. Once all callbacks are canceled, the operation is cancelled. + * + * @param token the token representing a set of callbacks to cancel + * + * @return YES if the operation was stopped because this was the last token to be canceled. NO otherwise. + */ +- (BOOL)cancel:(id)token; @end diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index 5a8bd11f..4b856549 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -17,11 +17,12 @@ NSString *const SDWebImageDownloadReceiveResponseNotification = @"SDWebImageDown NSString *const SDWebImageDownloadStopNotification = @"SDWebImageDownloadStopNotification"; NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinishNotification"; +static NSString *const kProgressCallbackKey = @"progress"; +static NSString *const kCompletedCallbackKey = @"completed"; + @interface SDWebImageDownloaderOperation () -@property (copy, nonatomic) SDWebImageDownloaderProgressBlock progressBlock; -@property (copy, nonatomic) SDWebImageDownloaderCompletedBlock completedBlock; -@property (copy, nonatomic) SDWebImageNoParamsBlock cancelBlock; +@property (strong, nonatomic) NSMutableArray *callbackBlocks; @property (assign, nonatomic, getter = isExecuting) BOOL executing; @property (assign, nonatomic, getter = isFinished) BOOL finished; @@ -45,18 +46,13 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis @synthesize finished = _finished; - (id)initWithRequest:(NSURLRequest *)request - options:(SDWebImageDownloaderOptions)options - progress:(SDWebImageDownloaderProgressBlock)progressBlock - completed:(SDWebImageDownloaderCompletedBlock)completedBlock - cancelled:(SDWebImageNoParamsBlock)cancelBlock { + options:(SDWebImageDownloaderOptions)options { if ((self = [super init])) { _request = request; _shouldDecompressImages = YES; _shouldUseCredentialStorage = YES; _options = options; - _progressBlock = [progressBlock copy]; - _completedBlock = [completedBlock copy]; - _cancelBlock = [cancelBlock copy]; + _callbackBlocks = [NSMutableArray new]; _executing = NO; _finished = NO; _expectedSize = 0; @@ -65,6 +61,24 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis return self; } +- (id)addHandlersForProgress:(SDWebImageDownloaderProgressBlock)progressBlock + completed:(SDWebImageDownloaderCompletedBlock)completedBlock { + NSMutableDictionary *callbacks = [NSMutableDictionary new]; + if (progressBlock) callbacks[kProgressCallbackKey] = [progressBlock copy]; + if (completedBlock) callbacks[kCompletedCallbackKey] = [completedBlock copy]; + [self.callbackBlocks addObject:callbacks]; + return callbacks; +} + +- (BOOL)cancel:(id)token { + [self.callbackBlocks removeObjectIdenticalTo:token]; + if (self.callbackBlocks.count == 0) { + [self cancel]; + return YES; + } + return NO; +} + - (void)start { @synchronized (self) { if (self.isCancelled) { @@ -100,8 +114,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis [self.connection start]; if (self.connection) { - if (self.progressBlock) { - self.progressBlock(0, NSURLResponseUnknownLength); + for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + progressBlock(0, NSURLResponseUnknownLength); } dispatch_async(dispatch_get_main_queue(), ^{ [[NSNotificationCenter defaultCenter] postNotificationName:SDWebImageDownloadStartNotification object:self]; @@ -123,8 +137,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis } } else { - if (self.completedBlock) { - self.completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Connection can't be initialized"}], YES); + for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Connection can't be initialized"}], YES); } } @@ -161,7 +175,6 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis - (void)cancelInternal { if (self.isFinished) return; [super cancel]; - if (self.cancelBlock) self.cancelBlock(); if (self.connection) { [self.connection cancel]; @@ -185,9 +198,7 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis } - (void)reset { - self.cancelBlock = nil; - self.completedBlock = nil; - self.progressBlock = nil; + [self.callbackBlocks removeAllObjects]; self.connection = nil; self.imageData = nil; self.thread = nil; @@ -217,8 +228,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis if (![response respondsToSelector:@selector(statusCode)] || ([((NSHTTPURLResponse *)response) statusCode] < 400 && [((NSHTTPURLResponse *)response) statusCode] != 304)) { NSInteger expected = response.expectedContentLength > 0 ? (NSInteger)response.expectedContentLength : 0; self.expectedSize = expected; - if (self.progressBlock) { - self.progressBlock(0, expected); + for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + progressBlock(0, expected); } self.imageData = [[NSMutableData alloc] initWithCapacity:expected]; @@ -241,8 +252,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis [[NSNotificationCenter defaultCenter] postNotificationName:SDWebImageDownloadStopNotification object:self]; }); - if (self.completedBlock) { - self.completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:[((NSHTTPURLResponse *)response) statusCode] userInfo:nil], YES); + for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:[((NSHTTPURLResponse *)response) statusCode] userInfo:nil], YES); } CFRunLoopStop(CFRunLoopGetCurrent()); [self done]; @@ -252,7 +263,7 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis - (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data { [self.imageData appendData:data]; - if ((self.options & SDWebImageDownloaderProgressiveDownload) && self.expectedSize > 0 && self.completedBlock) { + if ((self.options & SDWebImageDownloaderProgressiveDownload) && self.expectedSize > 0) { // The following code is from http://www.cocoaintheshell.com/2011/05/progressive-images-download-imageio/ // Thanks to the author @Nyx0uf @@ -319,8 +330,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis } CGImageRelease(partialImageRef); dispatch_main_sync_safe(^{ - if (self.completedBlock) { - self.completedBlock(image, nil, nil, NO); + for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + completedBlock(image, nil, nil, NO); } }); } @@ -329,8 +340,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis CFRelease(imageSource); } - if (self.progressBlock) { - self.progressBlock(self.imageData.length, self.expectedSize); + for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + progressBlock(self.imageData.length, self.expectedSize); } } @@ -362,7 +373,7 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis } - (void)connectionDidFinishLoading:(NSURLConnection *)aConnection { - SDWebImageDownloaderCompletedBlock completionBlock = self.completedBlock; + NSArray *completionBlocks = [[self.callbackBlocks valueForKey:kCompletedCallbackKey] copy]; @synchronized(self) { CFRunLoopStop(CFRunLoopGetCurrent()); self.thread = nil; @@ -377,7 +388,7 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis responseFromCached = NO; } - if (completionBlock) { + for (SDWebImageDownloaderCompletedBlock completionBlock in completionBlocks) { if (self.options & SDWebImageDownloaderIgnoreCachedResponse && responseFromCached) { completionBlock(nil, nil, nil, YES); } else if (self.imageData) { @@ -401,7 +412,6 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis completionBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}], YES); } } - self.completionBlock = nil; [self done]; } @@ -415,8 +425,8 @@ NSString *const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinis }); } - if (self.completedBlock) { - self.completedBlock(nil, nil, error, YES); + for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + completedBlock(nil, nil, error, YES); } self.completionBlock = nil; [self done]; diff --git a/Tests/Tests/SDWebImageDownloaderTests.m b/Tests/Tests/SDWebImageDownloaderTests.m index 972e8faf..27cece8f 100644 --- a/Tests/Tests/SDWebImageDownloaderTests.m +++ b/Tests/Tests/SDWebImageDownloaderTests.m @@ -32,6 +32,20 @@ [super tearDown]; } +- (BOOL)spinRunLoopWithTimeout:(NSTimeInterval)timeout untilBlockIsTrue:(BOOL(^)())block { + CFTimeInterval timeoutDate = CACurrentMediaTime() + 5.; + while (true) { + if (block()) { + return YES; + } + if (CACurrentMediaTime() > timeoutDate) { + return NO; + } + CFRunLoopRunInMode(kCFRunLoopDefaultMode, 1., true); + } + return NO; +} + - (void)testThatDownloadingSameURLTwiceAndCancellingFirstWorks { NSURL *imageURL = [NSURL URLWithString:@"http://static2.dmcdn.net/static/video/656/177/44771656:jpeg_preview_small.jpg?20120509154705"]; @@ -54,13 +68,40 @@ [[SDWebImageDownloader sharedDownloader] cancel:token1]; - CFTimeInterval timeoutDate = CACurrentMediaTime() + 5.; - while (true) { - if (CACurrentMediaTime() > timeoutDate || success) { - break; - } - CFRunLoopRunInMode(kCFRunLoopDefaultMode, 1., true); + success = [self spinRunLoopWithTimeout:5. untilBlockIsTrue:^BOOL{ + return success; + }]; + + if (!success) { + XCTFail(@"Failed to download image"); } +} + +- (void)testThatCancelingDownloadThenRequestingAgainWorks { + NSURL *imageURL = [NSURL URLWithString:@"http://static2.dmcdn.net/static/video/656/177/44771656:jpeg_preview_small.jpg?20120509154705"]; + + id token1 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL + options:0 + progress:nil + completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { + XCTFail(@"Shouldn't have completed here."); + }]; + expect(token1).toNot.beNil(); + + [[SDWebImageDownloader sharedDownloader] cancel:token1]; + + __block BOOL success = NO; + id token2 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL + options:0 + progress:nil + completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { + success = YES; + }]; + expect(token2).toNot.beNil(); + + success = [self spinRunLoopWithTimeout:5. untilBlockIsTrue:^BOOL{ + return success; + }]; if (!success) { XCTFail(@"Failed to download image"); From ae91053a80811ee585c68d4def0ffb76368495f7 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Mon, 8 Sep 2014 20:15:43 +0100 Subject: [PATCH 4/7] Don't create a new image for each completion block --- SDWebImage/SDWebImageDownloaderOperation.m | 24 ++++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index 4b856549..711e2a32 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -387,10 +387,12 @@ static NSString *const kCompletedCallbackKey = @"completed"; if (![[NSURLCache sharedURLCache] cachedResponseForRequest:_request]) { responseFromCached = NO; } - - for (SDWebImageDownloaderCompletedBlock completionBlock in completionBlocks) { + + if (completionBlocks.count > 0) { if (self.options & SDWebImageDownloaderIgnoreCachedResponse && responseFromCached) { - completionBlock(nil, nil, nil, YES); + for (SDWebImageDownloaderCompletedBlock completionBlock in completionBlocks) { + completionBlock(nil, nil, nil, YES); + } } else if (self.imageData) { UIImage *image = [UIImage sd_imageWithData:self.imageData]; NSString *key = [[SDWebImageManager sharedManager] cacheKeyForURL:self.request.URL]; @@ -402,14 +404,18 @@ static NSString *const kCompletedCallbackKey = @"completed"; image = [UIImage decodedImageWithImage:image]; } } - if (CGSizeEqualToSize(image.size, CGSizeZero)) { - completionBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Downloaded image has 0 pixels"}], YES); - } - else { - completionBlock(image, self.imageData, nil, YES); + for (SDWebImageDownloaderCompletedBlock completionBlock in completionBlocks) { + if (CGSizeEqualToSize(image.size, CGSizeZero)) { + completionBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Downloaded image has 0 pixels"}], YES); + } + else { + completionBlock(image, self.imageData, nil, YES); + } } } else { - completionBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}], YES); + for (SDWebImageDownloaderCompletedBlock completionBlock in completionBlocks) { + completionBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}], YES); + } } } [self done]; From f773870d2cc859b114e4f4fa278e154d6f0ecb56 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Mon, 15 Sep 2014 11:41:16 +0100 Subject: [PATCH 5/7] Add back some required locking --- SDWebImage/SDWebImageDownloader.m | 52 ++++++++++++---------- SDWebImage/SDWebImageDownloaderOperation.m | 51 +++++++++++++++------ 2 files changed, 66 insertions(+), 37 deletions(-) diff --git a/SDWebImage/SDWebImageDownloader.m b/SDWebImage/SDWebImageDownloader.m index f63c95ee..08ae9407 100644 --- a/SDWebImage/SDWebImageDownloader.m +++ b/SDWebImage/SDWebImageDownloader.m @@ -169,12 +169,14 @@ return; } - _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; - SDWebImageDownloaderOperation *operation = self.URLOperations[typedToken.url]; - BOOL canceled = [operation cancel:typedToken.downloadOperationCancelToken]; - if (canceled) { - [self.URLOperations removeObjectForKey:typedToken.url]; - } + dispatch_barrier_async(self.barrierQueue, ^{ + _SDWebImageDownloaderToken *typedToken = (_SDWebImageDownloaderToken *)token; + SDWebImageDownloaderOperation *operation = self.URLOperations[typedToken.url]; + BOOL canceled = [operation cancel:typedToken.downloadOperationCancelToken]; + if (canceled) { + [self.URLOperations removeObjectForKey:typedToken.url]; + } + }); } - (id)addProgressCallback:(SDWebImageDownloaderProgressBlock)progressBlock completedBlock:(SDWebImageDownloaderCompletedBlock)completedBlock forURL:(NSURL *)url createCallback:(SDWebImageDownloaderOperation *(^)())createCallback { @@ -186,25 +188,29 @@ return nil; } - SDWebImageDownloaderOperation *operation = self.URLOperations[url]; - if (!operation) { - operation = createCallback(); - self.URLOperations[url] = operation; + __block _SDWebImageDownloaderToken *token = nil; - __weak SDWebImageDownloaderOperation *woperation = operation; - operation.completionBlock = ^{ - SDWebImageDownloaderOperation *soperation = woperation; - if (!soperation) return; - if (self.URLOperations[url] == soperation) { - [self.URLOperations removeObjectForKey:url]; - }; - }; - } - id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock]; + dispatch_barrier_sync(self.barrierQueue, ^{ + SDWebImageDownloaderOperation *operation = self.URLOperations[url]; + if (!operation) { + operation = createCallback(); + self.URLOperations[url] = operation; - _SDWebImageDownloaderToken *token = [_SDWebImageDownloaderToken new]; - token.url = url; - token.downloadOperationCancelToken = downloadOperationCancelToken; + __weak SDWebImageDownloaderOperation *woperation = operation; + operation.completionBlock = ^{ + SDWebImageDownloaderOperation *soperation = woperation; + if (!soperation) return; + if (self.URLOperations[url] == soperation) { + [self.URLOperations removeObjectForKey:url]; + }; + }; + } + id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock]; + + token = [_SDWebImageDownloaderToken new]; + token.url = url; + token.downloadOperationCancelToken = downloadOperationCancelToken; + }); return token; } diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index 711e2a32..8d5f8feb 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -29,6 +29,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; @property (strong, nonatomic) NSMutableData *imageData; @property (strong, nonatomic) NSURLConnection *connection; @property (strong, atomic) NSThread *thread; +@property (SDDispatchQueueSetterSementics, nonatomic) dispatch_queue_t barrierQueue; #if TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_4_0 @property (assign, nonatomic) UIBackgroundTaskIdentifier backgroundTaskId; @@ -57,26 +58,46 @@ static NSString *const kCompletedCallbackKey = @"completed"; _finished = NO; _expectedSize = 0; responseFromCached = YES; // Initially wrong until `connection:willCacheResponse:` is called or not called + _barrierQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderOperationBarrierQueue", DISPATCH_QUEUE_CONCURRENT); } return self; } +- (void)dealloc { + SDDispatchQueueRelease(_barrierQueue); +} + - (id)addHandlersForProgress:(SDWebImageDownloaderProgressBlock)progressBlock completed:(SDWebImageDownloaderCompletedBlock)completedBlock { NSMutableDictionary *callbacks = [NSMutableDictionary new]; if (progressBlock) callbacks[kProgressCallbackKey] = [progressBlock copy]; if (completedBlock) callbacks[kCompletedCallbackKey] = [completedBlock copy]; - [self.callbackBlocks addObject:callbacks]; + dispatch_barrier_async(self.barrierQueue, ^{ + [self.callbackBlocks addObject:callbacks]; + }); + return callbacks; +} + +- (NSArray *)callbacksForKey:(NSString *)key { + __block NSArray *callbacks = nil; + dispatch_sync(self.barrierQueue, ^{ + callbacks = [self.callbackBlocks valueForKey:key]; + }); return callbacks; } - (BOOL)cancel:(id)token { - [self.callbackBlocks removeObjectIdenticalTo:token]; - if (self.callbackBlocks.count == 0) { + __block BOOL shouldCancel = NO; + dispatch_barrier_sync(self.barrierQueue, ^{ + [self.callbackBlocks removeObjectIdenticalTo:token]; + if (self.callbackBlocks.count == 0) { + shouldCancel = YES; + } + }); + if (shouldCancel) { [self cancel]; - return YES; } - return NO; + return shouldCancel; } - (void)start { @@ -114,7 +135,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; [self.connection start]; if (self.connection) { - for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + for (SDWebImageDownloaderProgressBlock progressBlock in [self callbacksForKey:kProgressCallbackKey]) { progressBlock(0, NSURLResponseUnknownLength); } dispatch_async(dispatch_get_main_queue(), ^{ @@ -137,7 +158,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; } } else { - for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + for (SDWebImageDownloaderCompletedBlock completedBlock in [self callbacksForKey:kCompletedCallbackKey]) { completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey : @"Connection can't be initialized"}], YES); } } @@ -198,7 +219,9 @@ static NSString *const kCompletedCallbackKey = @"completed"; } - (void)reset { - [self.callbackBlocks removeAllObjects]; + dispatch_barrier_async(self.barrierQueue, ^{ + [self.callbackBlocks removeAllObjects]; + }); self.connection = nil; self.imageData = nil; self.thread = nil; @@ -228,7 +251,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; if (![response respondsToSelector:@selector(statusCode)] || ([((NSHTTPURLResponse *)response) statusCode] < 400 && [((NSHTTPURLResponse *)response) statusCode] != 304)) { NSInteger expected = response.expectedContentLength > 0 ? (NSInteger)response.expectedContentLength : 0; self.expectedSize = expected; - for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + for (SDWebImageDownloaderProgressBlock progressBlock in [self callbacksForKey:kProgressCallbackKey]) { progressBlock(0, expected); } @@ -252,7 +275,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; [[NSNotificationCenter defaultCenter] postNotificationName:SDWebImageDownloadStopNotification object:self]; }); - for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + for (SDWebImageDownloaderCompletedBlock completedBlock in [self callbacksForKey:kCompletedCallbackKey]) { completedBlock(nil, nil, [NSError errorWithDomain:NSURLErrorDomain code:[((NSHTTPURLResponse *)response) statusCode] userInfo:nil], YES); } CFRunLoopStop(CFRunLoopGetCurrent()); @@ -330,7 +353,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; } CGImageRelease(partialImageRef); dispatch_main_sync_safe(^{ - for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + for (SDWebImageDownloaderCompletedBlock completedBlock in [self callbacksForKey:kCompletedCallbackKey]) { completedBlock(image, nil, nil, NO); } }); @@ -340,7 +363,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; CFRelease(imageSource); } - for (SDWebImageDownloaderProgressBlock progressBlock in [self.callbackBlocks valueForKey:kProgressCallbackKey]) { + for (SDWebImageDownloaderProgressBlock progressBlock in [self callbacksForKey:kProgressCallbackKey]) { progressBlock(self.imageData.length, self.expectedSize); } } @@ -373,7 +396,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; } - (void)connectionDidFinishLoading:(NSURLConnection *)aConnection { - NSArray *completionBlocks = [[self.callbackBlocks valueForKey:kCompletedCallbackKey] copy]; + NSArray *completionBlocks = [[self callbacksForKey:kCompletedCallbackKey] copy]; @synchronized(self) { CFRunLoopStop(CFRunLoopGetCurrent()); self.thread = nil; @@ -431,7 +454,7 @@ static NSString *const kCompletedCallbackKey = @"completed"; }); } - for (SDWebImageDownloaderCompletedBlock completedBlock in [self.callbackBlocks valueForKey:kCompletedCallbackKey]) { + for (SDWebImageDownloaderCompletedBlock completedBlock in [self callbacksForKey:kCompletedCallbackKey]) { completedBlock(nil, nil, error, YES); } self.completionBlock = nil; From a02aa422560b97d5222a8a5a4b43ea3b8ca257c9 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Mon, 15 Sep 2014 12:03:50 +0100 Subject: [PATCH 6/7] Switch to Xcode 6 async tests --- Tests/Tests/SDWebImageDownloaderTests.m | 40 +++++-------------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/Tests/Tests/SDWebImageDownloaderTests.m b/Tests/Tests/SDWebImageDownloaderTests.m index 27cece8f..233711fa 100644 --- a/Tests/Tests/SDWebImageDownloaderTests.m +++ b/Tests/Tests/SDWebImageDownloaderTests.m @@ -32,21 +32,9 @@ [super tearDown]; } -- (BOOL)spinRunLoopWithTimeout:(NSTimeInterval)timeout untilBlockIsTrue:(BOOL(^)())block { - CFTimeInterval timeoutDate = CACurrentMediaTime() + 5.; - while (true) { - if (block()) { - return YES; - } - if (CACurrentMediaTime() > timeoutDate) { - return NO; - } - CFRunLoopRunInMode(kCFRunLoopDefaultMode, 1., true); - } - return NO; -} - - (void)testThatDownloadingSameURLTwiceAndCancellingFirstWorks { + XCTestExpectation *expectation = [self expectationWithDescription:@"Correct image downloads"]; + NSURL *imageURL = [NSURL URLWithString:@"http://static2.dmcdn.net/static/video/656/177/44771656:jpeg_preview_small.jpg?20120509154705"]; id token1 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL @@ -57,27 +45,22 @@ }]; expect(token1).toNot.beNil(); - __block BOOL success = NO; id token2 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL options:0 progress:nil completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { - success = YES; + [expectation fulfill]; }]; expect(token2).toNot.beNil(); [[SDWebImageDownloader sharedDownloader] cancel:token1]; - success = [self spinRunLoopWithTimeout:5. untilBlockIsTrue:^BOOL{ - return success; - }]; - - if (!success) { - XCTFail(@"Failed to download image"); - } + [self waitForExpectationsWithTimeout:5. handler:nil]; } - (void)testThatCancelingDownloadThenRequestingAgainWorks { + XCTestExpectation *expectation = [self expectationWithDescription:@"Correct image downloads"]; + NSURL *imageURL = [NSURL URLWithString:@"http://static2.dmcdn.net/static/video/656/177/44771656:jpeg_preview_small.jpg?20120509154705"]; id token1 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL @@ -90,22 +73,15 @@ [[SDWebImageDownloader sharedDownloader] cancel:token1]; - __block BOOL success = NO; id token2 = [[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL options:0 progress:nil completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) { - success = YES; + [expectation fulfill]; }]; expect(token2).toNot.beNil(); - success = [self spinRunLoopWithTimeout:5. untilBlockIsTrue:^BOOL{ - return success; - }]; - - if (!success) { - XCTFail(@"Failed to download image"); - } + [self waitForExpectationsWithTimeout:5. handler:nil]; } @end From 91fcbb635c08d293fe0f01069401daa9b85e1533 Mon Sep 17 00:00:00 2001 From: Matt Galloway Date: Thu, 8 Jan 2015 22:09:35 +0000 Subject: [PATCH 7/7] Fix issue where progress block is [NSNull null] and that tries to be executed --- SDWebImage/SDWebImageDownloaderOperation.m | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/SDWebImage/SDWebImageDownloaderOperation.m b/SDWebImage/SDWebImageDownloaderOperation.m index 8d5f8feb..51f83a8a 100644 --- a/SDWebImage/SDWebImageDownloaderOperation.m +++ b/SDWebImage/SDWebImageDownloaderOperation.m @@ -79,9 +79,11 @@ static NSString *const kCompletedCallbackKey = @"completed"; } - (NSArray *)callbacksForKey:(NSString *)key { - __block NSArray *callbacks = nil; + __block NSMutableArray *callbacks = nil; dispatch_sync(self.barrierQueue, ^{ - callbacks = [self.callbackBlocks valueForKey:key]; + // We need to remove [NSNull null] because there might not always be a progress block for each callback + callbacks = [[self.callbackBlocks valueForKey:key] mutableCopy]; + [callbacks removeObjectIdenticalTo:[NSNull null]]; }); return callbacks; }