Merge pull request #2999 from dreampiggy/bugfix_prefetcher_cancel_hung

Fix the issue that NSOperation conforms to `SDWebImageOperation` check failed. And fix the SDAsyncBlockOperation cancel logic

SDWebImage.xcodeproj/project.pbxproj

@@ -112,6 +112,8 @@
 		327054D6206CD8B3006EA328 /* SDImageAPNGCoder.h in Headers */ = {isa = PBXBuildFile; fileRef = 327054D2206CD8B3006EA328 /* SDImageAPNGCoder.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		327054DA206CD8B3006EA328 /* SDImageAPNGCoder.m in Sources */ = {isa = PBXBuildFile; fileRef = 327054D3206CD8B3006EA328 /* SDImageAPNGCoder.m */; };
 		327054DC206CD8B3006EA328 /* SDImageAPNGCoder.m in Sources */ = {isa = PBXBuildFile; fileRef = 327054D3206CD8B3006EA328 /* SDImageAPNGCoder.m */; };
+		327F2E83245AE1650075F846 /* SDWebImageOperation.m in Sources */ = {isa = PBXBuildFile; fileRef = 327F2E82245AE1650075F846 /* SDWebImageOperation.m */; };
+		327F2E84245AE1650075F846 /* SDWebImageOperation.m in Sources */ = {isa = PBXBuildFile; fileRef = 327F2E82245AE1650075F846 /* SDWebImageOperation.m */; };
 		328BB69E2081FED200760D6C /* SDWebImageCacheKeyFilter.h in Headers */ = {isa = PBXBuildFile; fileRef = 328BB69A2081FED200760D6C /* SDWebImageCacheKeyFilter.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		328BB6A22081FED200760D6C /* SDWebImageCacheKeyFilter.m in Sources */ = {isa = PBXBuildFile; fileRef = 328BB69B2081FED200760D6C /* SDWebImageCacheKeyFilter.m */; };
 		328BB6A42081FED200760D6C /* SDWebImageCacheKeyFilter.m in Sources */ = {isa = PBXBuildFile; fileRef = 328BB69B2081FED200760D6C /* SDWebImageCacheKeyFilter.m */; };
@@ -437,6 +439,7 @@
 		326E2F32236F1D58006F847F /* SDDeviceHelper.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SDDeviceHelper.m; sourceTree = "<group>"; };
 		327054D2206CD8B3006EA328 /* SDImageAPNGCoder.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SDImageAPNGCoder.h; path = Core/SDImageAPNGCoder.h; sourceTree = "<group>"; };
 		327054D3206CD8B3006EA328 /* SDImageAPNGCoder.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = SDImageAPNGCoder.m; path = Core/SDImageAPNGCoder.m; sourceTree = "<group>"; };
+		327F2E82245AE1650075F846 /* SDWebImageOperation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = SDWebImageOperation.m; path = Core/SDWebImageOperation.m; sourceTree = "<group>"; };
 		328BB69A2081FED200760D6C /* SDWebImageCacheKeyFilter.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SDWebImageCacheKeyFilter.h; path = Core/SDWebImageCacheKeyFilter.h; sourceTree = "<group>"; };
 		328BB69B2081FED200760D6C /* SDWebImageCacheKeyFilter.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = SDWebImageCacheKeyFilter.m; path = Core/SDWebImageCacheKeyFilter.m; sourceTree = "<group>"; };
 		328BB6A82081FEE500760D6C /* SDWebImageCacheSerializer.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SDWebImageCacheSerializer.h; path = Core/SDWebImageCacheSerializer.h; sourceTree = "<group>"; };
@@ -846,6 +849,7 @@
 				320CAE132086F50500CFFC80 /* SDWebImageError.h */,
 				320CAE142086F50500CFFC80 /* SDWebImageError.m */,
 				530E49E71646388E002868E7 /* SDWebImageOperation.h */,
+				327F2E82245AE1650075F846 /* SDWebImageOperation.m */,
 				324DF4B2200A14DC008A84CC /* SDWebImageDefine.h */,
 				324DF4B3200A14DC008A84CC /* SDWebImageDefine.m */,
 				325312C6200F09910046BF1E /* SDWebImageTransition.h */,
@@ -1196,6 +1200,7 @@
 				4A2CAE201AB4BB6C00B6BC39 /* SDImageCache.m in Sources */,
 				4369C2801D9807EC007E863A /* UIView+WebCache.m in Sources */,
 				329A18611FFF5DFD008C9A2F /* UIImage+Metadata.m in Sources */,
+				327F2E84245AE1650075F846 /* SDWebImageOperation.m in Sources */,
 				328BB6B22081FEE500760D6C /* SDWebImageCacheSerializer.m in Sources */,
 				325C4611223394D8004CAE11 /* SDImageCachesManagerOperation.m in Sources */,
 			);
@@ -1270,6 +1275,7 @@
 				ABBE71A818C43B4D00B75E91 /* UIImageView+HighlightedWebCache.m in Sources */,
 				4369C27E1D9807EC007E863A /* UIView+WebCache.m in Sources */,
 				329A185F1FFF5DFD008C9A2F /* UIImage+Metadata.m in Sources */,
+				327F2E83245AE1650075F846 /* SDWebImageOperation.m in Sources */,
 				328BB6B02081FEE500760D6C /* SDWebImageCacheSerializer.m in Sources */,
 				325C4610223394D8004CAE11 /* SDImageCachesManagerOperation.m in Sources */,
 			);

SDWebImage/Core/SDWebImageOperation.m

@@ -0,0 +1,14 @@
+/*
+ * This file is part of the SDWebImage package.
+ * (c) Olivier Poitrey <rs@dailymotion.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+#import "SDWebImageOperation.h"
+
+/// NSOperation conform to `SDWebImageOperation`.
+@implementation NSOperation (SDWebImageOperation)
+
+@end

SDWebImage/Core/SDWebImagePrefetcher.m

@@ -20,6 +20,10 @@
     atomic_flag  _isAllFinished;
     
     unsigned long _totalCount;
+    
+    // Used to ensure NSPointerArray thread safe
+    dispatch_semaphore_t _prefetchOperationsLock;
+    dispatch_semaphore_t _loadOperationsLock;
 }
 
 @property (nonatomic, copy, readwrite) NSArray<NSURL *> *urls;
@@ -106,7 +110,6 @@
 }
 
 - (void)startPrefetchWithToken:(SDWebImagePrefetchToken * _Nonnull)token {
-    NSPointerArray *operations = token.loadOperations;
     for (NSURL *url in token.urls) {
         @autoreleasepool {
             @weakify(self);
@@ -142,13 +145,13 @@
                     [asyncOperation complete];
                 }];
                 NSAssert(operation != nil, @"Operation should not be nil, [SDWebImageManager loadImageWithURL:options:context:progress:completed:] break prefetch logic");
-                @synchronized (token) {
-                    [operations addPointer:(__bridge void *)operation];
-                }
+                SD_LOCK(token->_loadOperationsLock);
+                [token.loadOperations addPointer:(__bridge void *)operation];
+                SD_UNLOCK(token->_loadOperationsLock);
             }];
-            @synchronized (token) {
-                [token.prefetchOperations addPointer:(__bridge void *)prefetchOperation];
-            }
+            SD_LOCK(token->_prefetchOperationsLock);
+            [token.prefetchOperations addPointer:(__bridge void *)prefetchOperation];
+            SD_UNLOCK(token->_prefetchOperationsLock);
             [self.prefetchQueue addOperation:prefetchOperation];
         }
     }
@@ -262,24 +265,38 @@
 
 @implementation SDWebImagePrefetchToken
 
+- (instancetype)init {
+    self = [super init];
+    if (self) {
+        _prefetchOperationsLock = dispatch_semaphore_create(1);
+        _loadOperationsLock = dispatch_semaphore_create(1);
+    }
+    return self;
+}
+
 - (void)cancel {
-    @synchronized (self) {
-        [self.prefetchOperations compact];
-        for (id operation in self.prefetchOperations) {
-            if ([operation conformsToProtocol:@protocol(SDWebImageOperation)]) {
-                [operation cancel];
-            }
+    SD_LOCK(_prefetchOperationsLock);
+    [self.prefetchOperations compact];
+    for (id operation in self.prefetchOperations) {
+        id<SDWebImageOperation> strongOperation = operation;
+        if (strongOperation) {
+            [strongOperation cancel];
         }
-        self.prefetchOperations.count = 0;
-        
-        [self.loadOperations compact];
-        for (id operation in self.loadOperations) {
-            if ([operation conformsToProtocol:@protocol(SDWebImageOperation)]) {
-                [operation cancel];
-            }
+    }
+    self.prefetchOperations.count = 0;
+    SD_UNLOCK(_prefetchOperationsLock);
+    
+    SD_LOCK(_loadOperationsLock);
+    [self.loadOperations compact];
+    for (id operation in self.loadOperations) {
+        id<SDWebImageOperation> strongOperation = operation;
+        if (strongOperation) {
+            [strongOperation cancel];
         }
-        self.loadOperations.count = 0;
     }
+    self.loadOperations.count = 0;
+    SD_UNLOCK(_loadOperationsLock);
+    
     self.completionBlock = nil;
     self.progressBlock = nil;
     [self.prefetcher removeRunningToken:self];

SDWebImage/Private/SDAsyncBlockOperation.m

@@ -35,33 +35,58 @@
 }
 
 - (void)start {
-    if (self.isCancelled) {
-        return;
-    }
-    
-    [self willChangeValueForKey:@"isExecuting"];
-    self.executing = YES;
-    [self didChangeValueForKey:@"isExecuting"];
-    
-    if (self.executionBlock) {
-        self.executionBlock(self);
-    } else {
-        [self complete];
+    @synchronized (self) {
+        if (self.isCancelled) {
+            self.finished = YES;
+            return;
+        }
+        
+        self.finished = NO;
+        self.executing = YES;
+        
+        if (self.executionBlock) {
+            self.executionBlock(self);
+        } else {
+            self.executing = NO;
+            self.finished = YES;
+        }
     }
 }
 
 - (void)cancel {
-    [super cancel];
-    [self complete];
+    @synchronized (self) {
+        [super cancel];
+        if (self.isExecuting) {
+            self.executing = NO;
+            self.finished = YES;
+        }
+    }
 }
 
+ 
 - (void)complete {
-    [self willChangeValueForKey:@"isExecuting"];
+    @synchronized (self) {
+        if (self.isExecuting) {
+            self.finished = YES;
+            self.executing = NO;
+        }
+    }
+ }
+
+- (void)setFinished:(BOOL)finished {
     [self willChangeValueForKey:@"isFinished"];
-    self.executing = NO;
-    self.finished = YES;
-    [self didChangeValueForKey:@"isExecuting"];
+    _finished = finished;
     [self didChangeValueForKey:@"isFinished"];
 }
 
+- (void)setExecuting:(BOOL)executing {
+    [self willChangeValueForKey:@"isExecuting"];
+    _executing = executing;
+    [self didChangeValueForKey:@"isExecuting"];
+}
+
+- (BOOL)isConcurrent {
+    return YES;
+}
+
 @end

Tests/Tests/SDWebImageDownloaderTests.m

@@ -95,6 +95,10 @@
     operation = token.downloadOperation;
     expect([operation class]).to.equal([SDWebImageTestDownloadOperation class]);
     
+    // Assert the NSOperation conforms to `SDWebImageOperation`
+    expect([NSOperation.class conformsToProtocol:@protocol(SDWebImageOperation)]).beTruthy();
+    expect([operation conformsToProtocol:@protocol(SDWebImageOperation)]).beTruthy();
+    
     // back to the original value
     downloader.config.operationClass = nil;
     token = [downloader downloadImageWithURL:imageURL3 options:0 progress:nil completed:nil];