From 4173f9f3c1ac8a7ef2de40666bd32cb735f1ef55 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 30 Apr 2020 18:50:18 +0800 Subject: [PATCH 1/5] Fix the issue that NSOperation conforms to `SDWebImageOperation` check failed. And fix the SDAsyncBlockOperation cancel logic --- SDWebImage.xcodeproj/project.pbxproj | 6 ++++++ SDWebImage/Core/SDWebImageOperation.m | 14 ++++++++++++++ SDWebImage/Core/SDWebImagePrefetcher.m | 10 ++++++---- SDWebImage/Private/SDAsyncBlockOperation.m | 4 +++- 4 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 SDWebImage/Core/SDWebImageOperation.m diff --git a/SDWebImage.xcodeproj/project.pbxproj b/SDWebImage.xcodeproj/project.pbxproj index bbb554c1..3e2fefd6 100644 --- a/SDWebImage.xcodeproj/project.pbxproj +++ b/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 = ""; }; 327054D2206CD8B3006EA328 /* SDImageAPNGCoder.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SDImageAPNGCoder.h; path = Core/SDImageAPNGCoder.h; sourceTree = ""; }; 327054D3206CD8B3006EA328 /* SDImageAPNGCoder.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = SDImageAPNGCoder.m; path = Core/SDImageAPNGCoder.m; sourceTree = ""; }; + 327F2E82245AE1650075F846 /* SDWebImageOperation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = SDWebImageOperation.m; path = Core/SDWebImageOperation.m; sourceTree = ""; }; 328BB69A2081FED200760D6C /* SDWebImageCacheKeyFilter.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SDWebImageCacheKeyFilter.h; path = Core/SDWebImageCacheKeyFilter.h; sourceTree = ""; }; 328BB69B2081FED200760D6C /* SDWebImageCacheKeyFilter.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = SDWebImageCacheKeyFilter.m; path = Core/SDWebImageCacheKeyFilter.m; sourceTree = ""; }; 328BB6A82081FEE500760D6C /* SDWebImageCacheSerializer.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SDWebImageCacheSerializer.h; path = Core/SDWebImageCacheSerializer.h; sourceTree = ""; }; @@ -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 */, ); diff --git a/SDWebImage/Core/SDWebImageOperation.m b/SDWebImage/Core/SDWebImageOperation.m new file mode 100644 index 00000000..0d6e880d --- /dev/null +++ b/SDWebImage/Core/SDWebImageOperation.m @@ -0,0 +1,14 @@ +/* + * This file is part of the SDWebImage package. + * (c) Olivier Poitrey + * + * 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 diff --git a/SDWebImage/Core/SDWebImagePrefetcher.m b/SDWebImage/Core/SDWebImagePrefetcher.m index efcb5c73..30b65ace 100644 --- a/SDWebImage/Core/SDWebImagePrefetcher.m +++ b/SDWebImage/Core/SDWebImagePrefetcher.m @@ -266,16 +266,18 @@ @synchronized (self) { [self.prefetchOperations compact]; for (id operation in self.prefetchOperations) { - if ([operation conformsToProtocol:@protocol(SDWebImageOperation)]) { - [operation cancel]; + id 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]; + id strongOperation = operation; + if (strongOperation) { + [strongOperation cancel]; } } self.loadOperations.count = 0; diff --git a/SDWebImage/Private/SDAsyncBlockOperation.m b/SDWebImage/Private/SDAsyncBlockOperation.m index 8862ef8e..4aefb4f5 100644 --- a/SDWebImage/Private/SDAsyncBlockOperation.m +++ b/SDWebImage/Private/SDAsyncBlockOperation.m @@ -52,7 +52,9 @@ - (void)cancel { [super cancel]; - [self complete]; + if (self.isExecuting) { + [self complete]; + } } - (void)complete { From 974c3ff4adac26fda8c5fab3a091d7f72ff7376e Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 30 Apr 2020 18:54:12 +0800 Subject: [PATCH 2/5] Update the test case to ensure thi silly problem will not occur again :) --- Tests/Tests/SDWebImageDownloaderTests.m | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Tests/Tests/SDWebImageDownloaderTests.m b/Tests/Tests/SDWebImageDownloaderTests.m index 4cd877c1..6da292e5 100644 --- a/Tests/Tests/SDWebImageDownloaderTests.m +++ b/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]; From f51992cfd619f5cfffe8c0a298bb29c897193bb5 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 30 Apr 2020 19:19:55 +0800 Subject: [PATCH 3/5] Mark the `SDAsyncBlockOperation` safe using the lock and correct override method --- SDWebImage/Private/SDAsyncBlockOperation.m | 58 +++++++++++++++------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/SDWebImage/Private/SDAsyncBlockOperation.m b/SDWebImage/Private/SDAsyncBlockOperation.m index 4aefb4f5..754a2ff5 100644 --- a/SDWebImage/Private/SDAsyncBlockOperation.m +++ b/SDWebImage/Private/SDAsyncBlockOperation.m @@ -35,35 +35,55 @@ } - (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 willChangeValueForKey:@"isExecuting"]; + self.executing = YES; + [self didChangeValueForKey:@"isExecuting"]; + + if (self.executionBlock) { + self.executionBlock(self); + } else { + self.executing = NO; + self.finished = YES; + } } } - (void)cancel { - [super cancel]; - if (self.isExecuting) { - [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.executing = NO; + self.finished = YES; + } + } + } + +- (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"]; +} + @end From 54771ceb22451ab188c923ef191cee59373bf8cd Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Wed, 6 May 2020 10:32:35 +0800 Subject: [PATCH 4/5] Fix the wrong subclass which does not override `isConcurrent` method --- SDWebImage/Private/SDAsyncBlockOperation.m | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/SDWebImage/Private/SDAsyncBlockOperation.m b/SDWebImage/Private/SDAsyncBlockOperation.m index 754a2ff5..0b2fefec 100644 --- a/SDWebImage/Private/SDAsyncBlockOperation.m +++ b/SDWebImage/Private/SDAsyncBlockOperation.m @@ -41,9 +41,8 @@ return; } - [self willChangeValueForKey:@"isExecuting"]; + self.finished = NO; self.executing = YES; - [self didChangeValueForKey:@"isExecuting"]; if (self.executionBlock) { self.executionBlock(self); @@ -68,8 +67,8 @@ - (void)complete { @synchronized (self) { if (self.isExecuting) { - self.executing = NO; self.finished = YES; + self.executing = NO; } } } @@ -86,4 +85,8 @@ [self didChangeValueForKey:@"isExecuting"]; } +- (BOOL)isConcurrent { + return YES; +} + @end From fe730cc7e414d30b4a34d99e2fc41b46585d9f5c Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Wed, 6 May 2020 16:13:32 +0800 Subject: [PATCH 5/5] Fix the deadlock by using the two different semaphore, the `@synchronized` can not represent two pointer array lock and may be entered twice --- SDWebImage/Core/SDWebImagePrefetcher.m | 67 ++++++++++++++++---------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/SDWebImage/Core/SDWebImagePrefetcher.m b/SDWebImage/Core/SDWebImagePrefetcher.m index 30b65ace..5c0f37f7 100644 --- a/SDWebImage/Core/SDWebImagePrefetcher.m +++ b/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 *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,26 +265,38 @@ @implementation SDWebImagePrefetchToken -- (void)cancel { - @synchronized (self) { - [self.prefetchOperations compact]; - for (id operation in self.prefetchOperations) { - id strongOperation = operation; - if (strongOperation) { - [strongOperation cancel]; - } - } - self.prefetchOperations.count = 0; - - [self.loadOperations compact]; - for (id operation in self.loadOperations) { - id strongOperation = operation; - if (strongOperation) { - [strongOperation cancel]; - } - } - self.loadOperations.count = 0; +- (instancetype)init { + self = [super init]; + if (self) { + _prefetchOperationsLock = dispatch_semaphore_create(1); + _loadOperationsLock = dispatch_semaphore_create(1); } + return self; +} + +- (void)cancel { + SD_LOCK(_prefetchOperationsLock); + [self.prefetchOperations compact]; + for (id operation in self.prefetchOperations) { + id strongOperation = operation; + if (strongOperation) { + [strongOperation cancel]; + } + } + self.prefetchOperations.count = 0; + SD_UNLOCK(_prefetchOperationsLock); + + SD_LOCK(_loadOperationsLock); + [self.loadOperations compact]; + for (id operation in self.loadOperations) { + id strongOperation = operation; + if (strongOperation) { + [strongOperation cancel]; + } + } + self.loadOperations.count = 0; + SD_UNLOCK(_loadOperationsLock); + self.completionBlock = nil; self.progressBlock = nil; [self.prefetcher removeRunningToken:self];