Merge pull request SDWebImage#3477 from dreampiggy/bugfix/ensure_downloader_callback_atomic

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

Author: dreampiggy

Examples/SDWebImage Demo/MasterViewController.m

@@ -74,7 +74,7 @@ - (instancetype)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibB
                     @"https://raw.githubusercontent.com/icons8/flat-color-icons/master/pdf/stack_of_photos.pdf",
                     @"https://nr-platform.s3.amazonaws.com/uploads/platform/published_extension/branding_icon/275/AmazonS3.png",
                     @"https://res.cloudinary.com/dwpjzbyux/raw/upload/v1666474070/RawDemo/raw_vebed5.NEF",
-                    @"http://via.placeholder.com/200x200.jpg",
+                    @"https://via.placeholder.com/200x200.jpg",
                     nil];
 
         for (int i=1; i<25; i++) {

SDWebImage/Core/SDAnimatedImagePlayer.m

@@ -90,6 +90,7 @@ - (NSOperationQueue *)fetchQueue {
     if (!_fetchQueue) {
         _fetchQueue = [[NSOperationQueue alloc] init];
         _fetchQueue.maxConcurrentOperationCount = 1;
+        _fetchQueue.name = @"com.hackemist.SDAnimatedImagePlayer.fetchQueue";
     }
     return _fetchQueue;
 }

SDWebImage/Core/SDImageCache.m

@@ -63,7 +63,7 @@ @interface SDImageCache ()
 @property (nonatomic, strong, readwrite, nonnull) id<SDDiskCache> diskCache;
 @property (nonatomic, copy, readwrite, nonnull) SDImageCacheConfig *config;
 @property (nonatomic, copy, readwrite, nonnull) NSString *diskCachePath;
-@property (nonatomic, strong, nullable) dispatch_queue_t ioQueue;
+@property (nonatomic, strong, nonnull) dispatch_queue_t ioQueue;
 
 @end
 
@@ -118,7 +118,7 @@ - (nonnull instancetype)initWithNamespace:(nonnull NSString *)ns
 
         // Create IO queue
         dispatch_queue_attr_t ioQueueAttributes = _config.ioQueueAttributes;
-        _ioQueue = dispatch_queue_create("com.hackemist.SDImageCache", ioQueueAttributes);
+        _ioQueue = dispatch_queue_create("com.hackemist.SDImageCache.ioQueue", ioQueueAttributes);
         NSAssert(_ioQueue, @"The IO queue should not be nil. Your configured `ioQueueAttributes` may be wrong");
 
         // Init the memory cache
@@ -508,10 +508,6 @@ - (nullable NSData *)diskImageDataBySearchingAllPathsForKey:(nullable NSString *
 
 - (nullable UIImage *)diskImageForKey:(nullable NSString *)key {
     NSData *data = [self diskImageDataForKey:key];
-    return [self diskImageForKey:key data:data];
-}
-
-- (nullable UIImage *)diskImageForKey:(nullable NSString *)key data:(nullable NSData *)data {
     return [self diskImageForKey:key data:data options:0 context:nil];
 }
 

SDWebImage/Core/SDWebImageDownloader.m

@@ -13,13 +13,30 @@
 #import "SDWebImageCacheKeyFilter.h"
 #import "SDImageCacheDefine.h"
 #import "SDInternalMacros.h"
+#import "objc/runtime.h"
 
 NSNotificationName const SDWebImageDownloadStartNotification = @"SDWebImageDownloadStartNotification";
 NSNotificationName const SDWebImageDownloadReceiveResponseNotification = @"SDWebImageDownloadReceiveResponseNotification";
 NSNotificationName const SDWebImageDownloadStopNotification = @"SDWebImageDownloadStopNotification";
 NSNotificationName const SDWebImageDownloadFinishNotification = @"SDWebImageDownloadFinishNotification";
 
 static void * SDWebImageDownloaderContext = &SDWebImageDownloaderContext;
+static void * SDWebImageDownloaderOperationKey = &SDWebImageDownloaderOperationKey;
+
+BOOL SDWebImageDownloaderOperationGetCompleted(id<SDWebImageDownloaderOperation> operation) {
+    NSCParameterAssert(operation);
+    NSNumber *value = objc_getAssociatedObject(operation, SDWebImageDownloaderOperationKey);
+    if (value) {
+        return value.boolValue;
+    } else {
+        return NO;
+    }
+}
+
+void SDWebImageDownloaderOperationSetCompleted(id<SDWebImageDownloaderOperation> operation, BOOL isCompleted) {
+    NSCParameterAssert(operation);
+    objc_setAssociatedObject(operation, SDWebImageDownloaderOperationKey, @(isCompleted), OBJC_ASSOCIATION_RETAIN);
+}
 
 @interface SDWebImageDownloadToken ()
 
@@ -99,7 +116,7 @@ - (instancetype)initWithConfig:(SDWebImageDownloaderConfig *)config {
         [_config addObserver:self forKeyPath:NSStringFromSelector(@selector(maxConcurrentDownloads)) options:0 context:SDWebImageDownloaderContext];
         _downloadQueue = [NSOperationQueue new];
         _downloadQueue.maxConcurrentOperationCount = _config.maxConcurrentDownloads;
-        _downloadQueue.name = @"com.hackemist.SDWebImageDownloader";
+        _downloadQueue.name = @"com.hackemist.SDWebImageDownloader.downloadQueue";
         _URLOperations = [NSMutableDictionary new];
         NSMutableDictionary<NSString *, NSString *> *headerDictionary = [NSMutableDictionary dictionary];
         NSString *userAgent = nil;
@@ -219,7 +236,8 @@ - (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) {
+    BOOL shouldNotReuseOperation = !operation || operation.isFinished || operation.isCancelled || SDWebImageDownloaderOperationGetCompleted(operation);
+    if (shouldNotReuseOperation) {
         operation = [self createDownloaderOperationWithUrl:url options:options context:context];
         if (!operation) {
             SD_UNLOCK(_operationsLock);
@@ -499,6 +517,10 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
 
     // Identify the operation that runs this task and pass it the delegate method
     NSOperation<SDWebImageDownloaderOperation> *dataOperation = [self operationWithTask:task];
+    if (dataOperation) {
+        // Mark the downloader operation `isCompleted = YES`, no longer re-use this operation when new request comes in
+        SDWebImageDownloaderOperationSetCompleted(dataOperation, true);
+    }
     if ([dataOperation respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]) {
         [dataOperation URLSession:session task:task didCompleteWithError:error];
     }

SDWebImage/Core/SDWebImageDownloaderOperation.m

@@ -14,6 +14,8 @@
 #import "SDImageCacheDefine.h"
 #import "SDCallbackQueue.h"
 
+BOOL SDWebImageDownloaderOperationGetCompleted(id<SDWebImageDownloaderOperation> operation); // Private currently, mark open if needed
+
 // A handler to represent individual request
 @interface SDWebImageDownloaderOperationToken : NSObject
 
@@ -110,8 +112,9 @@ - (nonnull instancetype)initWithRequest:(nullable NSURLRequest *)request
         _finished = NO;
         _expectedSize = 0;
         _unownedSession = session;
-        _coderQueue = [NSOperationQueue new];
+        _coderQueue = [[NSOperationQueue alloc] init];
         _coderQueue.maxConcurrentOperationCount = 1;
+        _coderQueue.name = @"com.hackemist.SDWebImageDownloaderOperation.coderQueue";
         _imageMap = [[NSMapTable alloc] initWithKeyOptions:NSPointerFunctionsStrongMemory valueOptions:NSPointerFunctionsWeakMemory capacity:1];
 #if SD_UIKIT
         _backgroundTaskId = UIBackgroundTaskInvalid;
@@ -231,13 +234,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;
@@ -471,22 +471,22 @@ - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)data
         if (self.coderQueue.operationCount == 0) {
             // NSOperation have autoreleasepool, don't need to create extra one
             @weakify(self);
-            NSOperation *operation = [NSBlockOperation blockOperationWithBlock:^{
+            [self.coderQueue addOperationWithBlock:^{
                 @strongify(self);
                 if (!self) {
                     return;
                 }
-                UIImage *image = SDImageLoaderDecodeProgressiveImageData(imageData, self.request.URL, NO, self, [[self class] imageOptionsFromDownloaderOptions:self.options], self.context);
-                if (self.isFinished) {
+                // When cancelled or transfer finished (`didCompleteWithError`), cancel the progress callback, only completed block is called and enough
+                if (self.isCancelled || SDWebImageDownloaderOperationGetCompleted(self)) {
                     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];
                 }
             }];
-            [self.coderQueue addOperation:operation];
         }
     }
 
@@ -564,16 +564,8 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
                     // 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:^{
+                        [self.coderQueue addOperationWithBlock:^{
                             @strongify(self);
                             if (!self) {
                                 return;
@@ -612,11 +604,22 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
                                 [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_block_t doneBlock = ^{
+                        @strongify(self);
+                        if (!self) {
+                            return;
+                        }
+                        [self done];
+                    };
+                    if (@available(iOS 13, tvOS 13, macOS 10.15, watchOS 6, *)) {
+                        // seems faster than `addOperationWithBlock`
+                        [self.coderQueue addBarrierBlock:doneBlock];
+                    } else {
+                        // serial queue, this does the same effect in semantics
+                        [self.coderQueue addOperationWithBlock:doneBlock];
+                    }
                 }
             } else {
                 [self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorBadImageData userInfo:@{NSLocalizedDescriptionKey : @"Image data is nil"}]];

Tests/Tests/SDTestCase.m

@@ -11,9 +11,9 @@
 
 const int64_t kAsyncTestTimeout = 5;
 const int64_t kMinDelayNanosecond = NSEC_PER_MSEC * 100; // 0.1s
-NSString *const kTestJPEGURL = @"http://via.placeholder.com/50x50.jpg";
+NSString *const kTestJPEGURL = @"https://via.placeholder.com/50x50.jpg";
 NSString *const kTestProgressiveJPEGURL = @"https://raw.githubusercontent.com/ibireme/YYImage/master/Demo/YYImageDemo/mew_progressive.jpg";
-NSString *const kTestPNGURL = @"http://via.placeholder.com/50x50.png";
+NSString *const kTestPNGURL = @"https://via.placeholder.com/50x50.png";
 NSString *const kTestGIFURL = @"https://media.giphy.com/media/UEsrLdv7ugRTq/giphy.gif";
 NSString *const kTestAPNGPURL = @"https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png";
 

Tests/Tests/SDWebImageDownloaderTests.m

@@ -174,7 +174,7 @@ - (void)test10That404CaseCallsCompletionWithError {
 - (void)test11ThatCancelWorks {
     XCTestExpectation *expectation = [self expectationWithDescription:@"Cancel"];
 
-    NSURL *imageURL = [NSURL URLWithString:@"http://via.placeholder.com/1000x1000.png"];
+    NSURL *imageURL = [NSURL URLWithString:@"https://via.placeholder.com/1000x1000.png"];
     SDWebImageDownloadToken *token = [[SDWebImageDownloader sharedDownloader]
                                       downloadImageWithURL:imageURL options:0 progress:nil completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, BOOL finished) {
                                           expect(error).notTo.beNil();
@@ -680,7 +680,7 @@ - (void)test26DownloadURLSessionMetrics {
             expect(metric.fetchStartDate).notTo.beNil();
             expect(metric.connectStartDate).notTo.beNil();
             expect(metric.connectEndDate).notTo.beNil();
-            expect(metric.networkProtocolName).equal(@"http/1.1");
+            expect(metric.networkProtocolName).equal(@"h2");
             expect(metric.resourceFetchType).equal(NSURLSessionTaskMetricsResourceFetchTypeNetworkLoad);
             expect(metric.isProxyConnection).beFalsy();
             expect(metric.isReusedConnection).beFalsy();
@@ -779,7 +779,7 @@ - (void)test30ThatDifferentThumbnailLoadShouldCallbackDifferentSize {
     // We move the logic into SDWebImageDownloaderOperation, which decode each callback's thumbnail size with different decoding pipeline, and callback independently
     // Note the progressiveLoad does not support this and always callback first size
 
-    NSURL *url = [NSURL URLWithString:@"http://via.placeholder.com/501x501.png"];
+    NSURL *url = [NSURL URLWithString:@"https://via.placeholder.com/501x501.png"];
     NSString *fullSizeKey = [SDWebImageManager.sharedManager cacheKeyForURL:url];
     [SDImageCache.sharedImageCache removeImageFromDiskForKey:fullSizeKey];
     for (int i = 490; i < 500; i++) {
@@ -799,8 +799,39 @@ - (void)test30ThatDifferentThumbnailLoadShouldCallbackDifferentSize {
     [self waitForExpectationsWithTimeout:kAsyncTestTimeout * 5 handler:nil];
 }
 
+- (void)test31ThatMultipleRequestForSameURLNeverSkipCallback {
+    // See #3475
+    // When multiple download request for same URL, the SDWebImageDownloader will try to `Re-use` URLSessionTask to avoid duplicate actual network request
+    // However, if caller submit too frequently in another queue, we should stop attaching more callback once the URLSessionTask `didCompleteWithError:` is called
+    NSURL *url = [NSURL fileURLWithPath:[self testPNGPath]];
+    NSMutableArray<XCTestExpectation *> *expectations = [NSMutableArray arrayWithCapacity:100];
+    __block void (^recursiveBlock)(int);
+    void (^mainBlock)(int) = ^(int i) {
+        if (i > 200) return;
+        NSString *desc = [NSString stringWithFormat:@"Local url with index %d not callback!", i];
+        XCTestExpectation *expectation = [self expectationWithDescription:desc];
+        [expectations addObject:expectation];
+        // Delay 0.01s ~ 0.99s for each download request, simulate the real-world call site
+        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(i * 10000000ull)), dispatch_get_main_queue(), ^{
+            [SDWebImageDownloader.sharedDownloader downloadImageWithURL:url completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, BOOL finished) {
+                if (image) {
+                    NSLog(@"Local url callback with index: %d", i);
+                    [expectation fulfill];
+                } else {
+                    XCTFail(@"Something went wrong: %@", error.description);
+                }
+            }];
+        });
+        recursiveBlock(i+1);
+    };
+    recursiveBlock = mainBlock;
+    recursiveBlock(0);
+    
+    [self waitForExpectations:expectations timeout:kAsyncTestTimeout * 2];
+}
+
 #pragma mark - SDWebImageLoader
-- (void)test30CustomImageLoaderWorks {
+- (void)testCustomImageLoaderWorks {
     XCTestExpectation *expectation = [self expectationWithDescription:@"Custom image not works"];
     SDWebImageTestLoader *loader = [[SDWebImageTestLoader alloc] init];
     NSURL *imageURL = [NSURL URLWithString:kTestJPEGURL];
@@ -820,7 +851,7 @@ - (void)test30CustomImageLoaderWorks {
     [self waitForExpectationsWithCommonTimeout];
 }
 
-- (void)test31ThatLoadersManagerWorks {
+- (void)testThatLoadersManagerWorks {
     XCTestExpectation *expectation = [self expectationWithDescription:@"Loaders manager not works"];
     SDWebImageTestLoader *loader = [[SDWebImageTestLoader alloc] init];
     SDImageLoadersManager *manager = [[SDImageLoadersManager alloc] init];