From 7167e4a589ed23506c3885480e31b38a0b297e73 Mon Sep 17 00:00:00 2001 From: Victor Mayorov Date: Wed, 10 Jun 2015 15:38:51 +0300 Subject: [PATCH 1/4] Fixed ASBasicImageDownloader to handle multiple request with the same URL --- .../Details/ASBasicImageDownloader.mm | 123 +++++++++++++----- .../ASBasicImageDownloaderTests.m | 53 ++++++++ 2 files changed, 140 insertions(+), 36 deletions(-) create mode 100644 AsyncDisplayKitTests/ASBasicImageDownloaderTests.m diff --git a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm index 1d082bcecc..0c23a719e6 100644 --- a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm +++ b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm @@ -20,15 +20,21 @@ /** * Collection of properties associated with a download request. */ + +typedef void (^ASBasicImageDownloaderContextProgressBlock)(CGFloat); +typedef void (^ASBasicImageDownloaderContextCompletionBlock)(CGImageRef, NSError *); + +NSString * const kASBasicImageDownloaderContextCallbackQueue = @"kASBasicImageDownloaderContextCallbackQueue"; +NSString * const kASBasicImageDownloaderContextProgressBlock = @"kASBasicImageDownloaderContextProgressBlock"; +NSString * const kASBasicImageDownloaderContextCompletionBlock = @"kASBasicImageDownloaderContextCompletionBlock"; + @interface ASBasicImageDownloaderContext () { BOOL _invalid; ASDN::RecursiveMutex _propertyLock; } -@property (nonatomic, strong) dispatch_queue_t callbackQueue; -@property (nonatomic, copy) void (^downloadProgressBlock)(CGFloat); -@property (nonatomic, copy) void (^completionBlock)(CGImageRef, NSError *); +@property (nonatomic, strong) NSMutableArray *callbackDatas; @end @@ -63,6 +69,7 @@ static ASDN::RecursiveMutex currentRequestsLock; { if (self = [super init]) { _URL = URL; + _callbackDatas = [NSMutableArray array]; } return self; } @@ -87,6 +94,45 @@ static ASDN::RecursiveMutex currentRequestsLock; return _invalid; } +- (void)addCallbackData:(NSDictionary *)callbackData +{ + ASDN::MutexLocker l(_propertyLock); + [self.callbackDatas addObject:callbackData]; +} + +- (void)performProgressBlocks:(CGFloat)progress +{ + ASDN::MutexLocker l(_propertyLock); + for (NSDictionary *callbackData in self.callbackDatas) { + ASBasicImageDownloaderContextProgressBlock progressBlock = callbackData[kASBasicImageDownloaderContextProgressBlock]; + dispatch_queue_t callbackQueue = callbackData[kASBasicImageDownloaderContextCallbackQueue]; + + if (progressBlock) { + dispatch_async(callbackQueue, ^{ + progressBlock(progress); + }); + } + } +} + +- (void)completeWithImage:(CGImageRef)imageRef error:(NSError *)error +{ + ASDN::MutexLocker l(_propertyLock); + for (NSDictionary *callbackData in self.callbackDatas) { + ASBasicImageDownloaderContextCompletionBlock completionBlock = callbackData[kASBasicImageDownloaderContextCompletionBlock]; + dispatch_queue_t callbackQueue = callbackData[kASBasicImageDownloaderContextCallbackQueue]; + + if (completionBlock) { + dispatch_async(callbackQueue, ^{ + completionBlock(imageRef, error); + }); + } + } + + self.sessionTask = nil; + [self.callbackDatas removeAllObjects]; +} + @end @@ -150,30 +196,42 @@ static const char *kContextKey = NSStringFromClass(ASBasicImageDownloaderContext // NSURLSessionDownloadTask will do file I/O to create a temp directory. If called on the main thread this will // cause significant performance issues. dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - // the downloader may have been invalidated in the time it takes to async dispatch this block - if ([context isCancelled]) { - return; - } - - // create download task - NSURLSessionDownloadTask *task = [_session downloadTaskWithURL:URL]; - - // since creating the task does disk I/O, we should check if it has been invalidated - if ([context isCancelled]) { - return; - } - // associate metadata with it - context.callbackQueue = callbackQueue ?: dispatch_get_main_queue(); - context.downloadProgressBlock = downloadProgressBlock; - context.completionBlock = completion; - context.sessionTask = task; - task.originalRequest.asyncdisplaykit_context = context; + NSMutableDictionary *callbackData = [NSMutableDictionary dictionary]; + callbackData[kASBasicImageDownloaderContextCallbackQueue] = callbackQueue ?: dispatch_get_main_queue(); - // start downloading - [task resume]; + if (downloadProgressBlock) { + callbackData[kASBasicImageDownloaderContextProgressBlock] = [downloadProgressBlock copy]; + } - context.sessionTask = task; + if (completion) { + callbackData[kASBasicImageDownloaderContextCompletionBlock] = [completion copy]; + } + + [context addCallbackData:[NSDictionary dictionaryWithDictionary:callbackData]]; + + // Start new task only if previous one is not running + if (!context.sessionTask || (context.sessionTask.state != NSURLSessionTaskStateRunning)) { + // the downloader may have been invalidated in the time it takes to async dispatch this block + if ([context isCancelled]) { + return; + } + + // create download task + NSURLSessionDownloadTask *task = [_session downloadTaskWithURL:URL]; + + // since creating the task does disk I/O, we should check if it has been invalidated + if ([context isCancelled]) { + return; + } + + task.originalRequest.asyncdisplaykit_context = context; + + // start downloading + [task resume]; + + context.sessionTask = task; + } }); return context; @@ -200,9 +258,7 @@ static const char *kContextKey = NSStringFromClass(ASBasicImageDownloaderContext totalBytesExpectedToWrite:(int64_t)totalBytesExpectedToWrite { ASBasicImageDownloaderContext *context = downloadTask.originalRequest.asyncdisplaykit_context; - if (context.downloadProgressBlock) { - context.downloadProgressBlock((CGFloat)totalBytesWritten / (CGFloat)totalBytesExpectedToWrite); - } + [context performProgressBlocks:(CGFloat)totalBytesWritten / (CGFloat)totalBytesExpectedToWrite]; } // invoked if the download succeeded with no error @@ -214,12 +270,9 @@ static const char *kContextKey = NSStringFromClass(ASBasicImageDownloaderContext return; } - UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:location]]; - - if (context.completionBlock) { - dispatch_async(context.callbackQueue, ^{ - context.completionBlock(image.CGImage, nil); - }); + if (context) { + UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:location]]; + [context completeWithImage:image.CGImage error:nil]; } } @@ -229,9 +282,7 @@ static const char *kContextKey = NSStringFromClass(ASBasicImageDownloaderContext { ASBasicImageDownloaderContext *context = task.originalRequest.asyncdisplaykit_context; if (context && error) { - dispatch_async(context.callbackQueue, ^{ - context.completionBlock(NULL, error); - }); + [context completeWithImage:NULL error:error]; } } diff --git a/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m b/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m new file mode 100644 index 0000000000..279525c3ab --- /dev/null +++ b/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m @@ -0,0 +1,53 @@ +// +// ASBasicImageDownloaderTests.m +// AsyncDisplayKit +// +// Created by Victor Mayorov on 10/06/15. +// Copyright (c) 2015 Facebook. All rights reserved. +// + +#import + +#import + +@interface ASBasicImageDownloaderTests : XCTestCase + +@end + +@implementation ASBasicImageDownloaderTests + +- (void)testAsynchronouslyDownloadTheSameURLTwice { + ASBasicImageDownloader *downloader = [ASBasicImageDownloader new]; + + NSURL *URL = [NSURL URLWithString:@"http://wrongPath/wrongResource.png"]; + + dispatch_group_t group = dispatch_group_create(); + + __block BOOL firstDone = NO; + + dispatch_group_enter(group); + [downloader downloadImageWithURL:URL + callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0) + downloadProgressBlock:nil + completion:^(CGImageRef image, NSError *error) { + firstDone = YES; + dispatch_group_leave(group); + }]; + + __block BOOL secondDone = NO; + + dispatch_group_enter(group); + [downloader downloadImageWithURL:URL + callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0) + downloadProgressBlock:nil + completion:^(CGImageRef image, NSError *error) { + secondDone = YES; + dispatch_group_leave(group); + }]; + + XCTAssert(0 == dispatch_group_wait(group, dispatch_time(0, 10 * 1000000000)), @"URL loading takes to long"); + + XCTAssert(firstDone && secondDone, @"Not all handlers has been calles"); +} + +@end From 1f5813b835d81dba0de6faf0134aae6be904bf4d Mon Sep 17 00:00:00 2001 From: Victor Mayorov Date: Wed, 10 Jun 2015 16:19:54 +0300 Subject: [PATCH 2/4] Fixed typo --- AsyncDisplayKitTests/ASBasicImageDownloaderTests.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m b/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m index 279525c3ab..b174722341 100644 --- a/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m +++ b/AsyncDisplayKitTests/ASBasicImageDownloaderTests.m @@ -45,9 +45,9 @@ dispatch_group_leave(group); }]; - XCTAssert(0 == dispatch_group_wait(group, dispatch_time(0, 10 * 1000000000)), @"URL loading takes to long"); + XCTAssert(0 == dispatch_group_wait(group, dispatch_time(0, 10 * 1000000000)), @"URL loading takes too long"); - XCTAssert(firstDone && secondDone, @"Not all handlers has been calles"); + XCTAssert(firstDone && secondDone, @"Not all handlers has been called"); } @end From 7359e2255d68de21ea5a5b0f9dc21c4f721565b3 Mon Sep 17 00:00:00 2001 From: Victor Mayorov Date: Wed, 10 Jun 2015 17:50:21 +0300 Subject: [PATCH 3/4] Added synchronisation for sessionTask property --- .../Details/ASBasicImageDownloader.mm | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm index 0c23a719e6..f6b4dfdd34 100644 --- a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm +++ b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm @@ -133,6 +133,34 @@ static ASDN::RecursiveMutex currentRequestsLock; [self.callbackDatas removeAllObjects]; } +- (NSURLSessionTask *)createSessionTaskIfNecessaryWithBlock:(NSURLSessionTask *(^)())creationBlock { + { + ASDN::MutexLocker l(_propertyLock); + + if (self.isCancelled) { + return nil; + } + + if (self.sessionTask && (self.sessionTask.state == NSURLSessionTaskStateRunning)) { + return nil; + } + } + + NSURLSessionTask *newTask = creationBlock(); + + { + ASDN::MutexLocker l(_propertyLock); + + if (self.isCancelled) { + return nil; + } + + self.sessionTask = newTask; + + return self.sessionTask; + } +} + @end @@ -210,27 +238,14 @@ static const char *kContextKey = NSStringFromClass(ASBasicImageDownloaderContext [context addCallbackData:[NSDictionary dictionaryWithDictionary:callbackData]]; - // Start new task only if previous one is not running - if (!context.sessionTask || (context.sessionTask.state != NSURLSessionTaskStateRunning)) { - // the downloader may have been invalidated in the time it takes to async dispatch this block - if ([context isCancelled]) { - return; - } - - // create download task - NSURLSessionDownloadTask *task = [_session downloadTaskWithURL:URL]; - - // since creating the task does disk I/O, we should check if it has been invalidated - if ([context isCancelled]) { - return; - } + // Create new task if necessary + NSURLSessionDownloadTask *task = (NSURLSessionDownloadTask *)[context createSessionTaskIfNecessaryWithBlock:^(){return [_session downloadTaskWithURL:URL];}]; + if (task) { task.originalRequest.asyncdisplaykit_context = context; // start downloading [task resume]; - - context.sessionTask = task; } }); From 9c20edb310150356132d05c0a6c65f3531b77dc8 Mon Sep 17 00:00:00 2001 From: Victor Mayorov Date: Wed, 10 Jun 2015 17:58:35 +0300 Subject: [PATCH 4/4] Added checking that sessionTask hasn't been added during creation of previous one --- AsyncDisplayKit/Details/ASBasicImageDownloader.mm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm index f6b4dfdd34..8f69b50e45 100644 --- a/AsyncDisplayKit/Details/ASBasicImageDownloader.mm +++ b/AsyncDisplayKit/Details/ASBasicImageDownloader.mm @@ -154,7 +154,11 @@ static ASDN::RecursiveMutex currentRequestsLock; if (self.isCancelled) { return nil; } - + + if (self.sessionTask && (self.sessionTask.state == NSURLSessionTaskStateRunning)) { + return nil; + } + self.sessionTask = newTask; return self.sessionTask;