mirrors
/
SDWebImage
mirror of https://github.com/SDWebImage/SDWebImage.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2692 from dreampiggy/bugfix_user_cancel_guarantee_callback 

Ensure we always callback user's completion block even when cancelled with `SDWebImageErrorCancelled` error code
This commit is contained in:
DreamPiggy 2019-08-03 18:49:23 +08:00 committed by GitHub
parent 54b6d9d3f8 caae8f171f
commit c372e345f6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 119 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
SDWebImage/Core/SDImageCache.m
View File

@ -416,7 +416,9 @@
(!image && options & SDImageCacheQueryDiskDataSync));
void(^queryDiskBlock)(void) = ^{
if (operation.isCancelled) {
// do not call the completion if cancelled
if (doneBlock) {
doneBlock(nil, nil, SDImageCacheTypeNone);
}
return;
}

26
SDWebImage/Core/SDWebImageDownloader.m
View File

@ -204,6 +204,7 @@ static void * SDWebImageDownloaderContext = &SDWebImageDownloaderContext;
}
SD_LOCK(self.operationsLock);
id downloadOperationCancelToken;
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) {
@ -230,20 +231,25 @@ static void * SDWebImageDownloaderContext = &SDWebImageDownloaderContext;
// Add operation to operation queue only after all configuration done according to Apple's doc.
// `addOperation:` does not synchronously execute the `operation.completionBlock` so this will not cause deadlock.
[self.downloadQueue addOperation:operation];
}
else if (!operation.isExecuting) {
if (options & SDWebImageDownloaderHighPriority) {
operation.queuePriority = NSOperationQueuePriorityHigh;
} else if (options & SDWebImageDownloaderLowPriority) {
operation.queuePriority = NSOperationQueuePriorityLow;
} else {
operation.queuePriority = NSOperationQueuePriorityNormal;
downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock];
} else {
// When we reuse the download operation to attach more callbacks, there may be thread safe issue because the getter of callbacks may in another queue (decoding queue or delegate queue)
// So we lock the operation here, and in `SDWebImageDownloaderOperation`, we use `@synchonzied (self)`, to ensure the thread safe between these two classes.
@synchronized (operation) {
downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock];
}
if (!operation.isExecuting) {
if (options & SDWebImageDownloaderHighPriority) {
operation.queuePriority = NSOperationQueuePriorityHigh;
} else if (options & SDWebImageDownloaderLowPriority) {
operation.queuePriority = NSOperationQueuePriorityLow;
} else {
operation.queuePriority = NSOperationQueuePriorityNormal;
}
}
}
SD_UNLOCK(self.operationsLock);
id downloadOperationCancelToken = [operation addHandlersForProgress:progressBlock completed:completedBlock];
SDWebImageDownloadToken *token = [[SDWebImageDownloadToken alloc] initWithDownloadOperation:operation];
token.url = url;
token.request = operation.request;

53
SDWebImage/Core/SDWebImageDownloaderOperation.m
View File

@ -47,8 +47,6 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
@property (strong, nonatomic, readwrite, nullable) NSURLSessionTask *dataTask;
@property (strong, nonatomic, nonnull) dispatch_semaphore_t callbacksLock; // a lock to keep the access to `callbackBlocks` thread-safe
@property (strong, nonatomic, nonnull) dispatch_queue_t coderQueue; // the queue to do image decoding
#if SD_UIKIT
@property (assign, nonatomic) UIBackgroundTaskIdentifier backgroundTaskId;
@ -82,7 +80,6 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
_finished = NO;
_expectedSize = 0;
_unownedSession = session;
_callbacksLock = dispatch_semaphore_create(1);
_coderQueue = dispatch_queue_create("com.hackemist.SDWebImageDownloaderOperationCoderQueue", DISPATCH_QUEUE_SERIAL);
#if SD_UIKIT
_backgroundTaskId = UIBackgroundTaskInvalid;
@ -96,31 +93,47 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
SDCallbacksDictionary *callbacks = [NSMutableDictionary new];
if (progressBlock) callbacks[kProgressCallbackKey] = [progressBlock copy];
if (completedBlock) callbacks[kCompletedCallbackKey] = [completedBlock copy];
SD_LOCK(self.callbacksLock);
[self.callbackBlocks addObject:callbacks];
SD_UNLOCK(self.callbacksLock);
@synchronized (self) {
[self.callbackBlocks addObject:callbacks];
}
return callbacks;
}
- (nullable NSArray<id> *)callbacksForKey:(NSString *)key {
SD_LOCK(self.callbacksLock);
NSMutableArray<id> *callbacks = [[self.callbackBlocks valueForKey:key] mutableCopy];
SD_UNLOCK(self.callbacksLock);
NSMutableArray<id> *callbacks;
@synchronized (self) {
callbacks = [[self.callbackBlocks valueForKey:key] mutableCopy];
}
// We need to remove [NSNull null] because there might not always be a progress block for each callback
[callbacks removeObjectIdenticalTo:[NSNull null]];
return [callbacks copy]; // strip mutability here
}
- (BOOL)cancel:(nullable id)token {
if (!token) return NO;
BOOL shouldCancel = NO;
SD_LOCK(self.callbacksLock);
[self.callbackBlocks removeObjectIdenticalTo:token];
if (self.callbackBlocks.count == 0) {
shouldCancel = YES;
@synchronized (self) {
NSMutableArray *tempCallbackBlocks = [self.callbackBlocks mutableCopy];
[tempCallbackBlocks removeObjectIdenticalTo:token];
if (tempCallbackBlocks.count == 0) {
shouldCancel = YES;
}
}
SD_UNLOCK(self.callbacksLock);
if (shouldCancel) {
// Cancel operation running and callback last token's completion block
[self cancel];
} else {
// Only callback this token's completion block
@synchronized (self) {
[self.callbackBlocks removeObjectIdenticalTo:token];
}
SDWebImageDownloaderCompletedBlock completedBlock = [token valueForKey:kCompletedCallbackKey];
dispatch_main_async_safe(^{
if (completedBlock) {
completedBlock(nil, nil, [NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorCancelled userInfo:nil], YES);
}
});
}
return shouldCancel;
}
@ -129,6 +142,8 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
@synchronized (self) {
if (self.isCancelled) {
self.finished = YES;
// Operation cancelled by user before sending the request
[self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorCancelled userInfo:nil]];
[self reset];
return;
}
@ -222,6 +237,8 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
if (self.isExecuting) self.executing = NO;
if (!self.isFinished) self.finished = YES;
}
// Operation cancelled by user before sending the request
[self callCompletionBlocksWithError:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorCancelled userInfo:nil]];
[self reset];
}
@ -233,11 +250,8 @@ typedef NSMutableDictionary<NSString *, id> SDCallbacksDictionary;
}
- (void)reset {
SD_LOCK(self.callbacksLock);
[self.callbackBlocks removeAllObjects];
SD_UNLOCK(self.callbacksLock);
@synchronized (self) {
[self.callbackBlocks removeAllObjects];
self.dataTask = nil;
if (self.ownedSession) {
@ -381,6 +395,9 @@ didReceiveResponse:(NSURLResponse *)response
#pragma mark NSURLSessionTaskDelegate
- (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;
@synchronized(self) {
self.dataTask = nil;
__block typeof(self) strongSelf = self;

3
SDWebImage/Core/SDWebImageError.h
View File

@ -20,5 +20,6 @@ typedef NS_ERROR_ENUM(SDWebImageErrorDomain, SDWebImageError) {
SDWebImageErrorBadImageData = 1001, // The image data can not be decoded to image, or the image data is empty
SDWebImageErrorCacheNotModified = 1002, // The remote location specify that the cached image is not modified, such as the HTTP response 304 code. It's useful for `SDWebImageRefreshCached`
SDWebImageErrorInvalidDownloadOperation = 2000, // The image download operation is invalid, such as nil operation or unexpected error occur when operation initialized
SDWebImageErrorInvalidDownloadStatusCode = 2001, // The image downloda response a invalid status code. You can check the status code in error's userInfo under `SDWebImageErrorDownloadStatusCodeKey`
SDWebImageErrorInvalidDownloadStatusCode = 2001, // The image download response a invalid status code. You can check the status code in error's userInfo under `SDWebImageErrorDownloadStatusCodeKey`
SDWebImageErrorCancelled = 2002, // The image loading operation is cancelled before finished, during either async disk cache query, or waiting before actual network request. For actual network request error, check `NSURLErrorDomain` error domain and code.
};

12
SDWebImage/Core/SDWebImageManager.m
View File

@ -193,6 +193,8 @@ static id<SDImageLoader> _defaultImageLoader;
operation.cacheOperation = [self.imageCache queryImageForKey:key options:options context:context completion:^(UIImage * _Nullable cachedImage, NSData * _Nullable cachedData, SDImageCacheType cacheType) {
@strongify(operation);
if (!operation || operation.isCancelled) {
// Image combined operation cancelled by user
[self callCompletionBlockForOperation:operation completion:completedBlock error:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorCancelled userInfo:nil] url:url];
[self safelyRemoveOperationFromRunning:operation];
return;
}
@ -241,11 +243,13 @@ static id<SDImageLoader> _defaultImageLoader;
operation.loaderOperation = [self.imageLoader requestImageWithURL:url options:options context:context progress:progressBlock completed:^(UIImage *downloadedImage, NSData *downloadedData, NSError *error, BOOL finished) {
@strongify(operation);
if (!operation || operation.isCancelled) {
// Do nothing if the operation was cancelled
// See #699 for more details
// if we would call the completedBlock, there could be a race condition between this block and another completedBlock for the same object, so if this one is called second, we will overwrite the new data
// Image combined operation cancelled by user
[self callCompletionBlockForOperation:operation completion:completedBlock error:[NSError errorWithDomain:SDWebImageErrorDomain code:SDWebImageErrorCancelled userInfo:nil] url:url];
} else if (cachedImage && options & SDWebImageRefreshCached && [error.domain isEqualToString:SDWebImageErrorDomain] && error.code == SDWebImageErrorCacheNotModified) {
// Image refresh hit the NSURLCache cache, do not call the completion block
} else if ([error.domain isEqualToString:SDWebImageErrorDomain] && error.code == SDWebImageErrorCancelled) {
// Download operation cancelled by user before sending the request, don't block failed URL
[self callCompletionBlockForOperation:operation completion:completedBlock error:error url:url];
} else if (error) {
[self callCompletionBlockForOperation:operation completion:completedBlock error:error url:url];
BOOL shouldBlockFailedURL = [self shouldBlockFailedURLWithURL:url error:error];
@ -376,7 +380,7 @@ static id<SDImageLoader> _defaultImageLoader;
finished:(BOOL)finished
url:(nullable NSURL *)url {
dispatch_main_async_safe(^{
if (operation && !operation.isCancelled && completionBlock) {
if (completionBlock) {
completionBlock(image, data, error, cacheType, finished, url);
}
});

19
Tests/Tests/SDWebCacheCategoriesTests.m
View File

@ -203,12 +203,31 @@
- (void)testUIViewCancelCurrentImageLoad {
UIView *imageView = [[UIView alloc] init];
NSURL *originalImageURL = [NSURL URLWithString:kTestJPEGURL];
[SDImageCache.sharedImageCache removeImageFromDiskForKey:kTestJPEGURL];
[SDImageCache.sharedImageCache removeImageFromMemoryForKey:kTestJPEGURL];
[imageView sd_internalSetImageWithURL:originalImageURL placeholderImage:nil options:0 context:nil setImageBlock:nil progress:nil completed:nil];
[imageView sd_cancelCurrentImageLoad];
NSString *operationKey = NSStringFromClass(UIView.class);
expect([imageView sd_imageLoadOperationForKey:operationKey]).beNil();
}
- (void)testUIViewCancelCallbackWithError {
XCTestExpectation *expectation = [self expectationWithDescription:@"UIView internalSetImageWithURL cancel callback error"];
UIView *imageView = [[UIView alloc] init];
NSURL *originalImageURL = [NSURL URLWithString:kTestJPEGURL];
[SDImageCache.sharedImageCache removeImageFromDiskForKey:kTestJPEGURL];
[SDImageCache.sharedImageCache removeImageFromMemoryForKey:kTestJPEGURL];
[imageView sd_internalSetImageWithURL:originalImageURL placeholderImage:nil options:0 context:nil setImageBlock:nil progress:nil completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, SDImageCacheType cacheType, BOOL finished, NSURL * _Nullable imageURL) {
expect(error).notTo.beNil();
expect(error.code).equal(SDWebImageErrorCancelled);
[expectation fulfill];
}];
[imageView sd_cancelCurrentImageLoad];
[self waitForExpectationsWithCommonTimeout];
}
- (void)testUIViewImageProgressKVOWork {
XCTestExpectation *expectation = [self expectationWithDescription:@"UIView imageProgressKVO failed"];
UIView *view = [[UIView alloc] init];

13
Tests/Tests/SDWebImageDownloaderTests.m
View File

@ -161,10 +161,11 @@
- (void)test11ThatCancelWorks {
XCTestExpectation *expectation = [self expectationWithDescription:@"Cancel"];
NSURL *imageURL = [NSURL URLWithString:kTestJPEGURL];
NSURL *imageURL = [NSURL URLWithString:@"http://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) {
XCTFail(@"Should not get here");
expect(error).notTo.beNil();
expect(error.code).equal(SDWebImageErrorCancelled);
}];
expect([SDWebImageDownloader sharedDownloader].currentDownloadCount).to.equal(1);
@ -182,7 +183,7 @@
- (void)test11ThatCancelAllDownloadWorks {
XCTestExpectation *expectation = [self expectationWithDescription:@"CancelAllDownloads"];
NSURL *imageURL = [NSURL URLWithString:kTestJPEGURL];
NSURL *imageURL = [NSURL URLWithString:@"http://via.placeholder.com/1100x1100.png"];
[[SDWebImageDownloader sharedDownloader] downloadImageWithURL:imageURL completed:nil];
expect([SDWebImageDownloader sharedDownloader].currentDownloadCount).to.equal(1);
@ -323,7 +324,8 @@
options:0
progress:nil
completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) {
XCTFail(@"Shouldn't have completed here.");
expect(error).notTo.beNil();
expect(error.code).equal(SDWebImageErrorCancelled);
}];
expect(token1).toNot.beNil();
@ -361,7 +363,8 @@
options:0
progress:nil
completed:^(UIImage *image, NSData *data, NSError *error, BOOL finished) {
XCTFail(@"Shouldn't have completed here.");
expect(error).notTo.beNil();
expect(error.code).equal(SDWebImageErrorCancelled);
}];
expect(token1).toNot.beNil();

5
Tests/Tests/SDWebImageManagerTests.m
View File

@ -62,13 +62,14 @@
}
- (void)test06CancellAll {
XCTestExpectation *expectation = [self expectationWithDescription:@"Cancel"];
XCTestExpectation *expectation = [self expectationWithDescription:@"Cancel should callback with error"];
// need a bigger image here, that is why we don't use kTestJPEGURL
// if the image is too small, it will get downloaded before we can cancel :)
NSURL *url = [NSURL URLWithString:@"https://raw.githubusercontent.com/liyong03/YLGIFImage/master/YLGIFImageDemo/YLGIFImageDemo/joy.gif"];
[[SDWebImageManager sharedManager] loadImageWithURL:url options:0 progress:nil completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, SDImageCacheType cacheType, BOOL finished, NSURL * _Nullable imageURL) {
XCTFail(@"Should not get here");
expect(error).notTo.beNil();
expect(error.code).equal(SDWebImageErrorCancelled);
}];
[[SDWebImageManager sharedManager] cancelAll];

25
Tests/Tests/SDWebImagePrefetcherTests.m
View File

@ -151,6 +151,31 @@
expect(prefetcher.runningTokens.count).equal(0);
}
- (void)test07DownloaderCancelDuringPrefetching {
XCTestExpectation *expectation = [self expectationWithDescription:@"Downloader cancel during prefetch should not hung up"];
NSArray *imageURLs = @[@"http://via.placeholder.com/5000x5000.jpg",
@"http://via.placeholder.com/6000x6000.jpg",
@"http://via.placeholder.com/7000x7000.jpg"];
for (NSString *url in imageURLs) {
[SDImageCache.sharedImageCache removeImageFromDiskForKey:url];
}
SDWebImagePrefetcher *prefetcher = [[SDWebImagePrefetcher alloc] init];
prefetcher.maxConcurrentPrefetchCount = 3;
[prefetcher prefetchURLs:imageURLs progress:nil completed:^(NSUInteger noOfFinishedUrls, NSUInteger noOfSkippedUrls) {
expect(noOfSkippedUrls).equal(3);
[expectation fulfill];
}];
// Cancel all download, should not effect the prefetcher logic or cause hung up
// Prefetch is not sync, so using wait for testing
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kMinDelayNanosecond), dispatch_get_main_queue(), ^{
[SDWebImageDownloader.sharedDownloader cancelAllDownloads];
});
[self waitForExpectationsWithCommonTimeout];
}
- (void)imagePrefetcher:(SDWebImagePrefetcher *)imagePrefetcher didFinishWithTotalCount:(NSUInteger)totalCount skippedCount:(NSUInteger)skippedCount {
expect(imagePrefetcher).to.equal(self.prefetcher);
self.skippedCount = skippedCount;