[ASNetworkImageNode] Don't lock while calling downloader (#2864)

* Don't lock while calling downloader

Addresses #2785
To avoid performance issues, we should avoid locking the downloader.
To achieve this we need to do some kinda gross things. Essentially
the cost is the code is more complex and potentially far less performant
in edge cases. In testing, edge cases are nearly never hit, but I'm not
sure how good I feel about the cost in code complexity. This exacerbates
the locking issues in ASNetworkImageNode:

1. There is no convention for which methods lock.
2. There's no indication which vars are only set on init and therefore
safe to access except in the class extension definition.

* Shouldn't have checked in product changes.

* Using ivar instead of local var copied within lock.

Author: garrettmoon

AsyncDisplayKit/ASNetworkImageNode.mm

@@ -25,9 +25,6 @@
 
 @interface ASNetworkImageNode ()
 {
-  __weak id<ASImageCacheProtocol> _cache;
-  __weak id<ASImageDownloaderProtocol> _downloader;
-
   // Only access any of these with __instanceLock__.
   __weak id<ASNetworkImageNodeDelegate> _delegate;
 
@@ -53,6 +50,9 @@ @interface ASNetworkImageNode ()
   } _delegateFlags;
 
   //set on init only
+  __weak id<ASImageDownloaderProtocol> _downloader;
+  __weak id<ASImageCacheProtocol> _cache;
+  
   struct {
     unsigned int downloaderImplementsSetProgress:1;
     unsigned int downloaderImplementsSetPriority:1;
@@ -236,11 +236,13 @@ - (void)setDelegate:(id<ASNetworkImageNodeDelegate>)delegate
 
 - (void)setShouldRenderProgressImages:(BOOL)shouldRenderProgressImages
 {
-  ASDN::MutexLocker l(__instanceLock__);
-  if (shouldRenderProgressImages == _shouldRenderProgressImages) {
-    return;
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    if (shouldRenderProgressImages == _shouldRenderProgressImages) {
+      return;
+    }
+    _shouldRenderProgressImages = shouldRenderProgressImages;
   }
-  _shouldRenderProgressImages = shouldRenderProgressImages;
 
   [self _updateProgressImageBlockOnDownloaderIfNeeded];
 }
@@ -293,28 +295,41 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously
 - (void)didEnterVisibleState
 {
   [super didEnterVisibleState];
-  ASDN::MutexLocker l(__instanceLock__);
+  
+  id downloadIdentifier = nil;
+  
+  {
+    ASDN::MutexLocker l(__instanceLock__);
 
-  if (_downloaderFlags.downloaderImplementsSetPriority) {
-    if (_downloadIdentifier != nil) {
-      [_downloader setPriority:ASImageDownloaderPriorityVisible withDownloadIdentifier:_downloadIdentifier];
+    if (_downloaderFlags.downloaderImplementsSetPriority) {
+      downloadIdentifier = _downloadIdentifier;
     }
   }
 
+  if (downloadIdentifier != nil) {
+    [_downloader setPriority:ASImageDownloaderPriorityVisible withDownloadIdentifier:downloadIdentifier];
+  }
+  
   [self _updateProgressImageBlockOnDownloaderIfNeeded];
 }
 
 - (void)didExitVisibleState
 {
   [super didExitVisibleState];
-  ASDN::MutexLocker l(__instanceLock__);
+  
+  id downloadIdentifier = nil;
 
-  if (_downloaderFlags.downloaderImplementsSetPriority) {
-    if (_downloadIdentifier != nil) {
-      [_downloader setPriority:ASImageDownloaderPriorityPreload withDownloadIdentifier:_downloadIdentifier];
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    if (_downloaderFlags.downloaderImplementsSetPriority) {
+      downloadIdentifier = _downloadIdentifier;
     }
   }
 
+  if (downloadIdentifier != nil) {
+    [_downloader setPriority:ASImageDownloaderPriorityPreload withDownloadIdentifier:downloadIdentifier];
+  }
+  
   [self _updateProgressImageBlockOnDownloaderIfNeeded];
 }
 
@@ -370,9 +385,16 @@ - (void)_updateProgressImageBlockOnDownloaderIfNeeded
   }
 
   // Read state.
-  BOOL shouldRender = _shouldRenderProgressImages && ASInterfaceStateIncludesVisible(_interfaceState);
-  id oldDownloadIDForProgressBlock = _downloadIdentifierForProgressBlock;
-  id newDownloadIDForProgressBlock = shouldRender ? _downloadIdentifier : nil;
+  BOOL shouldRender;
+  id oldDownloadIDForProgressBlock;
+  id newDownloadIDForProgressBlock;
+  BOOL clearAndReattempt = NO;
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    shouldRender = _shouldRenderProgressImages && ASInterfaceStateIncludesVisible(_interfaceState);
+    oldDownloadIDForProgressBlock = _downloadIdentifierForProgressBlock;
+    newDownloadIDForProgressBlock = shouldRender ? _downloadIdentifier : nil;
+  }
 
   // If we're already bound to the correct download, we're done.
   if (ASObjectIsEqual(oldDownloadIDForProgressBlock, newDownloadIDForProgressBlock)) {
@@ -393,7 +415,19 @@ - (void)_updateProgressImageBlockOnDownloaderIfNeeded
   }
 
   // Update state.
-  _downloadIdentifierForProgressBlock = newDownloadIDForProgressBlock;
+  {
+    ASDN::MutexLocker l(__instanceLock__);
+    if (_downloadIdentifierForProgressBlock == oldDownloadIDForProgressBlock) {
+      _downloadIdentifierForProgressBlock = newDownloadIDForProgressBlock;
+    } else {
+      clearAndReattempt = YES;
+    }
+  }
+  
+  if (clearAndReattempt) {
+    [_downloader setProgressImageBlock:nil callbackQueue:dispatch_get_main_queue() withDownloadIdentifier:newDownloadIDForProgressBlock];
+    [self _updateProgressImageBlockOnDownloaderIfNeeded];
+  }
 }
 
 - (void)_cancelDownloadAndClearImage
@@ -443,19 +477,46 @@ - (void)_clearImage
 - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> imageContainer, NSError*, id downloadIdentifier))finished
 {
   ASPerformBlockOnBackgroundThread(^{
+    NSURL *url;
+    id downloadIdentifier;
+    BOOL cancelAndReattempt = NO;
 
-    ASDN::MutexLocker l(__instanceLock__);
-    _downloadIdentifier = [_downloader downloadImageWithURL:_URL
-                                              callbackQueue:dispatch_get_main_queue()
-                                           downloadProgress:NULL
-                                                 completion:^(id <ASImageContainerProtocol> _Nullable imageContainer, NSError * _Nullable error, id  _Nullable downloadIdentifier) {
-                                                   if (finished != NULL) {
-                                                     finished(imageContainer, error, downloadIdentifier);
-                                                   }
-                                                 }];
+    // Below, to avoid performance issues, we're calling downloadImageWithURL without holding the lock. This is a bit ugly because
+    // We need to reobtain the lock after and ensure that the task we've kicked off still matches our URL. If not, we need to cancel
+    // it and try again.
+    {
+      ASDN::MutexLocker l(__instanceLock__);
+      url = _URL;
+    }
+    
+    downloadIdentifier = [_downloader downloadImageWithURL:url
+                                             callbackQueue:dispatch_get_main_queue()
+                                          downloadProgress:NULL
+                                                completion:^(id <ASImageContainerProtocol> _Nullable imageContainer, NSError * _Nullable error, id  _Nullable downloadIdentifier) {
+                                                  if (finished != NULL) {
+                                                    finished(imageContainer, error, downloadIdentifier);
+                                                  }
+                                                }];
 
+    {
+      ASDN::MutexLocker l(__instanceLock__);
+      if ([_URL isEqual:url]) {
+        // The download we kicked off is correct, no need to do any more work.
+        _downloadIdentifier = downloadIdentifier;
+      } else {
+        // The URL changed since we kicked off our download task. This shouldn't happen often so we'll pay the cost and
+        // cancel that request and kick off a new one.
+        cancelAndReattempt = YES;
+      }
+    }
+    
+    if (cancelAndReattempt) {
+      [_downloader cancelImageDownloadForIdentifier:downloadIdentifier];
+      [self _downloadImageWithCompletion:finished];
+      return;
+    }
+    
     [self _updateProgressImageBlockOnDownloaderIfNeeded];
-      
   });
 }