Merge pull request #476 from victormayorov/master

Fixed ASBasicImageDownloader to handle multiple requests with the same…

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,77 @@ 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];
+}
+
+- (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;
+    }
+
+    if (self.sessionTask && (self.sessionTask.state == NSURLSessionTaskStateRunning)) {
+      return nil;
+    }
+
+    self.sessionTask = newTask;
+    
+    return self.sessionTask;
+  }
+}
+
 @end
 
 
@@ -150,30 +228,29 @@ 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]];
+
+    // 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];
+    }
   });
 
   return context;
@@ -200,9 +277,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 +289,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 +301,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];
   }
 }
 

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 <XCTest/XCTest.h>
+
+#import <AsyncDisplayKit/ASBasicImageDownloader.h>
+
+@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 too long");
+    
+    XCTAssert(firstDone && secondDone, @"Not all handlers has been called");
+}
+
+@end