Ensure the DownloaderOperation callback the completion in atomic and never miss one

The downloader will now check and ignore a `transferedDataFinished` operation (which is callbacking its own completion), because it's not safe in multi-thread environment, create new network request instead

Author: dreampiggy

SDWebImage/Core/SDWebImageDownloader.m

@@ -219,7 +219,7 @@ - (nullable SDWebImageDownloadToken *)downloadImageWithURL:(nullable NSURL *)url
     SDImageCoderOptions *decodeOptions = SDGetDecodeOptionsFromContext(context, [self.class imageOptionsFromDownloaderOptions:options], cacheKey);
     NSOperation<SDWebImageDownloaderOperation> *operation = [self.URLOperations objectForKey:url];
     // There is a case that the operation may be marked as finished or cancelled, but not been removed from `self.URLOperations`.
-    if (!operation || operation.isFinished || operation.isCancelled) {
+    if (!operation || operation.isFinished || operation.isCancelled || operation.isTransferFinished) {
         operation = [self createDownloaderOperationWithUrl:url options:options context:context];
         if (!operation) {
             SD_UNLOCK(_operationsLock);

SDWebImage/Core/SDWebImageDownloaderOperation.h

@@ -35,6 +35,8 @@
 
 - (BOOL)cancel:(nullable id)token;
 
+@property (assign, readonly) BOOL isTransferFinished;
+
 @property (strong, nonatomic, readonly, nullable) NSURLRequest *request;
 @property (strong, nonatomic, readonly, nullable) NSURLResponse *response;
 
@@ -56,6 +58,10 @@
  */
 @interface SDWebImageDownloaderOperation : NSOperation <SDWebImageDownloaderOperation>
 
+/// Whether the operation's network data transfer is finished. This is used by downloader to decide whether to call `addHandlersForProgress:`, or create a new operation instead.
+/// @note You must implements this or this will cause downloader attach new handlers for a already finished operation, may cause some callback missing.
+@property (assign, readonly) BOOL isTransferFinished;
+
 /**
  * The request used by the operation's task.
  */

SDWebImage/Core/SDWebImageDownloaderOperation.m

@@ -73,7 +73,8 @@ @interface SDWebImageDownloaderOperation ()
 
 @property (strong, nonatomic, readwrite, nullable) NSURLSessionTaskMetrics *metrics API_AVAILABLE(macosx(10.12), ios(10.0), watchos(3.0), tvos(10.0));
 
-@property (strong, nonatomic, nonnull) NSOperationQueue *coderQueue; // the serial operation queue to do image decoding
+@property (strong, nonatomic, nonnull) dispatch_queue_t coderQueue; // the serial operation queue to do image decoding
+@property (assign, readwrite) BOOL isTransferFinished; // Whether current operation's network transfer is finished (actually, `didCompleteWithError` already been called)
 
 @property (strong, nonatomic, nonnull) NSMapTable<SDImageCoderOptions *, UIImage *> *imageMap; // each variant of image is weak-referenced to avoid too many re-decode during downloading
 #if SD_UIKIT
@@ -110,8 +111,7 @@ - (nonnull instancetype)initWithRequest:(nullable NSURLRequest *)request
         _finished = NO;
         _expectedSize = 0;
         _unownedSession = session;
-        _coderQueue = [NSOperationQueue new];
-        _coderQueue.maxConcurrentOperationCount = 1;
+        _coderQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderOperation", DISPATCH_QUEUE_SERIAL);
         _imageMap = [[NSMapTable alloc] initWithKeyOptions:NSPointerFunctionsStrongMemory valueOptions:NSPointerFunctionsWeakMemory capacity:1];
 #if SD_UIKIT
         _backgroundTaskId = UIBackgroundTaskInvalid;
@@ -231,13 +231,10 @@ - (void)start {
     if (self.dataTask) {
         if (self.options & SDWebImageDownloaderHighPriority) {
             self.dataTask.priority = NSURLSessionTaskPriorityHigh;
-            self.coderQueue.qualityOfService = NSQualityOfServiceUserInteractive;
         } else if (self.options & SDWebImageDownloaderLowPriority) {
             self.dataTask.priority = NSURLSessionTaskPriorityLow;
-            self.coderQueue.qualityOfService = NSQualityOfServiceBackground;
         } else {
             self.dataTask.priority = NSURLSessionTaskPriorityDefault;
-            self.coderQueue.qualityOfService = NSQualityOfServiceDefault;
         }
         [self.dataTask resume];
         NSArray<SDWebImageDownloaderOperationToken *> *tokens;
@@ -468,26 +465,23 @@ - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)data
         NSData *imageData = self.imageData;
 
         // keep maximum one progressive decode process during download
-        if (self.coderQueue.operationCount == 0) {
-            // NSOperation have autoreleasepool, don't need to create extra one
-            @weakify(self);
-            NSOperation *operation = [NSBlockOperation blockOperationWithBlock:^{
-                @strongify(self);
-                if (!self) {
-                    return;
-                }
-                UIImage *image = SDImageLoaderDecodeProgressiveImageData(imageData, self.request.URL, NO, self, [[self class] imageOptionsFromDownloaderOptions:self.options], self.context);
-                if (self.isFinished) {
-                    return;
-                }
-                if (image) {
-                    // We do not keep the progressive decoding image even when `finished`=YES. Because they are for view rendering but not take full function from downloader options. And some coders implementation may not keep consistent between progressive decoding and normal decoding.
-                    
-                    [self callCompletionBlocksWithImage:image imageData:nil error:nil finished:NO];
-                }
-            }];
-            [self.coderQueue addOperation:operation];
-        }
+        @weakify(self);
+        dispatch_async(self.coderQueue, ^{
+            @strongify(self);
+            if (!self) {
+                return;
+            }
+            // When cancelled or transfer finished (`didCompleteWithError`), cancel the progress callback, only completed block is called and enough
+            if (self.isCancelled || self.isTransferFinished) {
+                return;
+            }
+            UIImage *image = SDImageLoaderDecodeProgressiveImageData(imageData, self.request.URL, NO, self, [[self class] imageOptionsFromDownloaderOptions:self.options], self.context);
+            if (image) {
+                // We do not keep the progressive decoding image even when `finished`=YES. Because they are for view rendering but not take full function from downloader options. And some coders implementation may not keep consistent between progressive decoding and normal decoding.
+                
+                [self callCompletionBlocksWithImage:image imageData:nil error:nil finished:NO];
+            }
+        });
     }
 
     for (SDWebImageDownloaderOperationToken *token in tokens) {
@@ -518,6 +512,7 @@ - (void)URLSession:(NSURLSession *)session
 - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didCompleteWithError:(NSError *)error {
     // If we already cancel the operation or anything mark the operation finished, don't callback twice
     if (self.isFinished) return;
+    self.isTransferFinished = YES;
 
     NSArray<SDWebImageDownloaderOperationToken *> *tokens;
     @synchronized (self) {
@@ -561,19 +556,9 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
                     [self callCompletionBlocksWithError:self.responseError];
                     [self done];
                 } else {
-                    // decode the image in coder queue, cancel all previous decoding process
-                    [self.coderQueue cancelAllOperations];
                     @weakify(self);
-                    // call done after all different variant completed block was dispatched
-                    NSOperation *doneOperation = [NSBlockOperation blockOperationWithBlock:^{
-                        @strongify(self);
-                        if (!self) {
-                            return;
-                        }
-                        [self done];
-                    }];
                     for (SDWebImageDownloaderOperationToken *token in tokens) {
-                        NSOperation *operation = [NSBlockOperation blockOperationWithBlock:^{
+                        dispatch_async(self.coderQueue, ^{
                             @strongify(self);
                             if (!self) {
                                 return;
@@ -611,12 +596,19 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
                             } else {
                                 [self callCompletionBlockWithToken:token image:image imageData:imageData error:nil finished:YES];
                             }
-                        }];
-                        [doneOperation addDependency:operation];
-                        [self.coderQueue addOperation:operation];
+                        });
                     }
                     // call [self done] after all completed block was dispatched
-                    [self.coderQueue addOperation:doneOperation];
+                    dispatch_barrier_async(self.coderQueue, ^{
+                        @strongify(self);
+                        if (!self) {
+                            return;
+                        }
+                        [self done];
+                    });
+                    dispatch_async(self.coderQueue, ^{
+                        [self done];
+                    });
                 }
             } else {
                 [self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorBadImageData userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}]];