Exposes a new option in ASImageDownloaderProtocol to retry image downloads (#1948)

chggr · web-flow · commit 17d4d13463c9 · 2021-02-19T08:23:01.000-08:00
* Exposes a new option in ASImageDownloaderProtocol to retry image downloads

At the moment the ASBasicImageDownloader does not automatically retry image downloads if the remote
host is unreachable. On the contrary the ASPINRemoteImageDownloader automatically retries. Retrying is
something that ultimately clients need to be able to control, for example to fail fast to an alternative image
rather than keep retrying for more than one minute while not displaying any image. This change exposes
a new option in the ASImageDownloaderProtocol to retry image downloads. It also uses this new option
in both ASNetworkImageNode and also ASMultiplexImageNode, setting it to YES to preserve the current
behaviour.

* Fixes a failing test in ASMultiplexImageNodeTests

* Fixes ScreenNode.m

ScreenNode.m is implementing ASImageDownloaderProtocol and needs to
be fixed to reflect changes in the latter.
diff --git a/Source/ASMultiplexImageNode.h b/Source/ASMultiplexImageNode.h
@@ -130,6 +130,12 @@ typedef NS_ENUM(NSUInteger, ASMultiplexImageNodeErrorCode) {
  */
 @property (nonatomic) BOOL shouldRenderProgressImages;
 
+/**
+ * Specifies whether the underlying image downloader should attempt to retry downloading the image if the remote
+ * host is unreachable. It will have no effect if the downloader does not support retrying. The default is YES.
+ */
+@property BOOL shouldRetryImageDownload;
+
 /**
  * @abstract The image manager that this image node should use when requesting images from the Photos framework. If this is `nil` (the default), then `PHImageManager.defaultManager` is used.
 
diff --git a/Source/ASMultiplexImageNode.mm b/Source/ASMultiplexImageNode.mm
@@ -176,13 +176,14 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI
   
   _downloaderFlags.downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
   _downloaderFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
-  _downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];
+  _downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:shouldRetry:priority:callbackQueue:downloadProgress:completion:)];
 
   _cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];
   
   _shouldRenderProgressImages = YES;
 
   self.shouldBypassEnsureDisplay = YES;
+  self.shouldRetryImageDownload = YES;
 
   return self;
 }
@@ -862,6 +863,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
        */
       ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(strongSelf.interfaceState);
       downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
+                                                             shouldRetry:[self shouldRetryImageDownload]
                                                                 priority:priority
                                                            callbackQueue:callbackQueue
                                                         downloadProgress:downloadProgressBlock
@@ -877,6 +879,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
         and their requests are put into the same pool.
        */
       downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
+                                                             shouldRetry:[self shouldRetryImageDownload]
                                                            callbackQueue:callbackQueue
                                                         downloadProgress:downloadProgressBlock
                                                               completion:completion];
diff --git a/Source/ASNetworkImageNode.h b/Source/ASNetworkImageNode.h
@@ -116,6 +116,12 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @property BOOL shouldRenderProgressImages;
 
+/**
+ * Specifies whether the underlying image downloader should attempt to retry downloading the image if the remote
+ * host is unreachable. It will have no effect if the downloader does not support retrying. The default is YES.
+ */
+@property BOOL shouldRetryImageDownload;
+
 /**
  * The image quality of the current image.
  *
diff --git a/Source/ASNetworkImageNode.mm b/Source/ASNetworkImageNode.mm
@@ -96,14 +96,15 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI
   _networkImageNodeFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
   _networkImageNodeFlags.downloaderImplementsAnimatedImage = [downloader respondsToSelector:@selector(animatedImageWithData:)];
   _networkImageNodeFlags.downloaderImplementsCancelWithResume = [downloader respondsToSelector:@selector(cancelImageDownloadWithResumePossibilityForIdentifier:)];
-  _networkImageNodeFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];
+  _networkImageNodeFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:shouldRetry:priority:callbackQueue:downloadProgress:completion:)];
 
   _networkImageNodeFlags.cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];
   _networkImageNodeFlags.cacheSupportsSynchronousFetch = [cache respondsToSelector:@selector(synchronouslyFetchedCachedImageWithURL:)];
   
   _networkImageNodeFlags.shouldCacheImage = YES;
   _networkImageNodeFlags.shouldRenderProgressImages = YES;
   self.shouldBypassEnsureDisplay = YES;
+  self.shouldRetryImageDownload = YES;
 
   return self;
 }
@@ -656,8 +657,8 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima
       ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(interfaceState);
 
       downloadIdentifier = [self->_downloader downloadImageWithURL:url
-                           
-                            priority:priority
+                                                       shouldRetry:[self shouldRetryImageDownload]
+                                                          priority:priority
                                                      callbackQueue:callbackQueue
                                                   downloadProgress:downloadProgress
                                                         completion:completion];
@@ -672,6 +673,7 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima
         and their requests are put into the same pool.
       */
       downloadIdentifier = [self->_downloader downloadImageWithURL:url
+                                                       shouldRetry:[self shouldRetryImageDownload]
                                                      callbackQueue:callbackQueue
                                                   downloadProgress:downloadProgress
                                                         completion:completion];
diff --git a/Source/Details/ASBasicImageDownloader.h b/Source/Details/ASBasicImageDownloader.h
@@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN
  * A shared image downloader which can be used by @c ASNetworkImageNodes and @c ASMultiplexImageNodes.
  * The userInfo provided by this downloader is `nil`.
  *
- * This is a very basic image downloader. It does not support caching, progressive downloading and likely
+ * This is a very basic image downloader. It does not support caching, retrying, progressive downloading and likely
  * isn't something you should use in production. If you'd like something production ready, see @c ASPINRemoteImageDownloader
  *
  * @note It is strongly recommended you include PINRemoteImage and use @c ASPINRemoteImageDownloader instead.
diff --git a/Source/Details/ASBasicImageDownloader.mm b/Source/Details/ASBasicImageDownloader.mm
@@ -253,18 +253,21 @@ - (instancetype)_init
 #pragma mark ASImageDownloaderProtocol.
 
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
                          completion:(ASImageDownloaderCompletion)completion
 {
   return [self downloadImageWithURL:URL
+                        shouldRetry:shouldRetry
                            priority:ASImageDownloaderPriorityImminent // maps to default priority
                       callbackQueue:callbackQueue
                    downloadProgress:downloadProgress
                          completion:completion];
 }
 
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                            priority:(ASImageDownloaderPriority)priority
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(ASImageDownloaderProgress)downloadProgress
diff --git a/Source/Details/ASImageProtocols.h b/Source/Details/ASImageProtocols.h
@@ -92,6 +92,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
 /**
  @abstract Downloads an image with the given URL.
  @param URL The URL of the image to download.
+ @param shouldRetry Whether to attempt to retry downloading if the remote host is currently unreachable.
  @param callbackQueue The queue to call `downloadProgressBlock` and `completion` on.
  @param downloadProgress The block to be invoked when the download of `URL` progresses.
  @param completion The block to be invoked when the download has completed, or has failed.
@@ -100,6 +101,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
  retain the identifier if you wish to use it later.
  */
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
                          completion:(ASImageDownloaderCompletion)completion;
@@ -117,6 +119,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
 /**
  @abstract Downloads an image with the given URL.
  @param URL The URL of the image to download.
+ @param shouldRetry Whether to attempt to retry downloading if the remote host is currently unreachable.
  @param priority The priority at which the image should be downloaded.
  @param callbackQueue The queue to call `downloadProgressBlock` and `completion` on.
  @param downloadProgress The block to be invoked when the download of `URL` progresses.
@@ -127,6 +130,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
  retain the identifier if you wish to use it later.
  */
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                            priority:(ASImageDownloaderPriority)priority
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
diff --git a/Source/Details/ASPINRemoteImageDownloader.mm b/Source/Details/ASPINRemoteImageDownloader.mm
@@ -255,18 +255,21 @@ - (void)clearFetchedImageFromCacheWithURL:(NSURL *)URL
 }
 
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(ASImageDownloaderProgress)downloadProgress
                          completion:(ASImageDownloaderCompletion)completion;
 {
   return [self downloadImageWithURL:URL
+                        shouldRetry:shouldRetry
                            priority:ASImageDownloaderPriorityImminent // maps to default priority
                       callbackQueue:callbackQueue
                    downloadProgress:downloadProgress
                          completion:completion];
 }
 
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                            priority:(ASImageDownloaderPriority)priority
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(ASImageDownloaderProgress)downloadProgress
@@ -301,8 +304,13 @@ - (nullable id)downloadImageWithURL:(NSURL *)URL
   // extra downloads isn't worth the effort of rechecking caches every single time. In order to provide
   // feedback to the consumer about whether images are cached, we can't simply make the cache a no-op and
   // check the cache as part of this download.
+  PINRemoteImageManagerDownloadOptions options = PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache;
+  if (!shouldRetry) {
+    options |= PINRemoteImageManagerDownloadOptionsSkipRetry;
+  }
+
   return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL
-                                                          options:PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache
+                                                          options:options
                                                          priority:pi_priority
                                                     progressImage:nil
                                                  progressDownload:progressDownload
diff --git a/Tests/ASBasicImageDownloaderTests.mm b/Tests/ASBasicImageDownloaderTests.mm
@@ -28,13 +28,15 @@ - (void)testAsynchronouslyDownloadTheSameURLTwice
                                                          subdirectory:@"TestResources"];
 
   [downloader downloadImageWithURL:URL
+                       shouldRetry:YES
                      callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)
                   downloadProgress:nil
                         completion:^(id<ASImageContainerProtocol>  _Nullable image, NSError * _Nullable error, id  _Nullable downloadIdentifier, id _Nullable userInfo) {
                           [firstExpectation fulfill];
                         }];
   
   [downloader downloadImageWithURL:URL
+                       shouldRetry:YES
                      callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)
                   downloadProgress:nil
                         completion:^(id<ASImageContainerProtocol>  _Nullable image, NSError * _Nullable error, id  _Nullable downloadIdentifier, id _Nullable userInfo) {
diff --git a/Tests/ASMultiplexImageNodeTests.mm b/Tests/ASMultiplexImageNodeTests.mm
@@ -222,14 +222,14 @@ - (void)testUncachedDownload
 
   // Mock a 50%-progress URL download.
   const CGFloat mockedProgress = 0.5;
-  OCMExpect([mockDownloader downloadImageWithURL:[self _testImageURL] priority:ASImageDownloaderPriorityPreload callbackQueue:OCMOCK_ANY downloadProgress:[OCMArg isNotNil] completion:[OCMArg isNotNil]])
+  OCMExpect([mockDownloader downloadImageWithURL:[self _testImageURL] shouldRetry:YES priority:ASImageDownloaderPriorityPreload callbackQueue:OCMOCK_ANY downloadProgress:[OCMArg isNotNil] completion:[OCMArg isNotNil]])
   .andDo(^(NSInvocation *inv){
     // Simulate progress.
-    ASImageDownloaderProgress progressBlock = [inv as_argumentAtIndexAsObject:5];
+    ASImageDownloaderProgress progressBlock = [inv as_argumentAtIndexAsObject:6];
     progressBlock(mockedProgress);
 
     // Simulate completion.
-    ASImageDownloaderCompletion completionBlock = [inv as_argumentAtIndexAsObject:6];
+    ASImageDownloaderCompletion completionBlock = [inv as_argumentAtIndexAsObject:7];
     completionBlock([self _testImage], nil, nil, nil);
   });
 
diff --git a/Tests/ASNetworkImageNodeTests.mm b/Tests/ASNetworkImageNodeTests.mm
@@ -43,7 +43,7 @@ - (void)DISABLED_testThatProgressBlockIsSetAndClearedCorrectlyOnVisibility
   node.URL = [NSURL URLWithString:@"http://imageA"];
 
   // Enter preload range, wait for download start.
-  [[[downloader expect] andForwardToRealObject] downloadImageWithURL:[OCMArg isNotNil] callbackQueue:OCMOCK_ANY downloadProgress:OCMOCK_ANY completion:OCMOCK_ANY];
+  [[[downloader expect] andForwardToRealObject] downloadImageWithURL:[OCMArg isNotNil] shouldRetry:OCMOCK_ANY callbackQueue:OCMOCK_ANY downloadProgress:OCMOCK_ANY completion:OCMOCK_ANY];
   [node enterInterfaceState:ASInterfaceStatePreload];
   [downloader verifyWithDelay:5];
 
@@ -138,7 +138,7 @@ - (void)cancelImageDownloadForIdentifier:(id)downloadIdentifier
   // nop
 }
 
-- (id)downloadImageWithURL:(NSURL *)URL callbackQueue:(dispatch_queue_t)callbackQueue downloadProgress:(ASImageDownloaderProgress)downloadProgress completion:(ASImageDownloaderCompletion)completion
+- (id)downloadImageWithURL:(NSURL *)URL shouldRetry:(BOOL)shouldRetry callbackQueue:(dispatch_queue_t)callbackQueue downloadProgress:(ASImageDownloaderProgress)downloadProgress completion:(ASImageDownloaderCompletion)completion
 {
   return @(_currentDownloadID++);
 }
diff --git a/examples_extra/Multiplex/Sample/ScreenNode.m b/examples_extra/Multiplex/Sample/ScreenNode.m
@@ -115,6 +115,7 @@ - (void)multiplexImageNode:(ASMultiplexImageNode *)imageNode didFinishDownloadin
 #pragma mark ASImageDownloaderProtocol.
 
 - (nullable id)downloadImageWithURL:(NSURL *)URL
+                        shouldRetry:(BOOL)shouldRetry
                       callbackQueue:(dispatch_queue_t)callbackQueue
                    downloadProgress:(nullable ASImageDownloaderProgress)downloadProgressBlock
                          completion:(ASImageDownloaderCompletion)completion

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN`
`20`	`20`	`* A shared image downloader which can be used by @c ASNetworkImageNodes and @c ASMultiplexImageNodes.`
`21`	`21`	* The userInfo provided by this downloader is `nil`.
`22`	`22`	`*`
`23`		`- * This is a very basic image downloader. It does not support caching, progressive downloading and likely`
	`23`	`+ * This is a very basic image downloader. It does not support caching, retrying, progressive downloading and likely`
`24`	`24`	`* isn't something you should use in production. If you'd like something production ready, see @c ASPINRemoteImageDownloader`
`25`	`25`	`*`
`26`	`26`	`* @note It is strongly recommended you include PINRemoteImage and use @c ASPINRemoteImageDownloader instead.`