From 902c6191263cb0f2caad9cbf337cd944214f98ce Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 26 Sep 2019 17:52:22 +0800 Subject: [PATCH 1/2] Added the test case `test15DownloaderLIFOExecutionOrder` to fix the LIFO order. See #2823 --- Tests/Tests/SDWebImageDownloaderTests.m | 87 +++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/Tests/Tests/SDWebImageDownloaderTests.m b/Tests/Tests/SDWebImageDownloaderTests.m index f1dbb11f..9f626b0c 100644 --- a/Tests/Tests/SDWebImageDownloaderTests.m +++ b/Tests/Tests/SDWebImageDownloaderTests.m @@ -26,6 +26,8 @@ @interface SDWebImageDownloaderTests : SDTestCase +@property (nonatomic, strong) NSMutableArray *executionOrderURLs; + @end @implementation SDWebImageDownloaderTests @@ -250,6 +252,91 @@ [self waitForExpectationsWithCommonTimeout]; } +- (void)test15DownloaderLIFOExecutionOrder { + SDWebImageDownloaderConfig *config = [[SDWebImageDownloaderConfig alloc] init]; + config.executionOrder = SDWebImageDownloaderLIFOExecutionOrder; // Last In First Out + config.maxConcurrentDownloads = 1; // 1 + SDWebImageDownloader *downloader = [[SDWebImageDownloader alloc] initWithConfig:config]; + self.executionOrderURLs = [NSMutableArray array]; + + // Input order: 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 (wait for 7 finished and immediately) -> 8 -> 9 -> 10 -> 11 -> 12 -> 13 -> 14 + // Expected result: 1 (first one has no dependency) -> 7 -> 14 -> 13 -> 12 -> 11 -> 10 -> 9 -> 8 -> 6 -> 5 -> 4 -> 3 -> 2 + int waitIndex = 7; + int maxIndex = 14; + NSMutableArray *expectations = [NSMutableArray array]; + for (int i = 1; i <= maxIndex; i++) { + XCTestExpectation *expectation = [self expectationWithDescription:[NSString stringWithFormat:@"URL %d order wrong", i]]; + [expectations addObject:expectation]; + } + + for (int i = 1; i <= waitIndex; i++) { + [self createLIFOOperationWithDownloader:downloader expectation:expectations[i-1] index:i]; + } + [[NSNotificationCenter defaultCenter] addObserverForName:SDWebImageDownloadFinishNotification object:nil queue:nil usingBlock:^(NSNotification * _Nonnull note) { + SDWebImageDownloaderOperation *operation = note.object; + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", waitIndex]]; + if (![operation.request.URL isEqual:url]) { + return; + } + for (int i = waitIndex + 1; i <= maxIndex; i++) { + [self createLIFOOperationWithDownloader:downloader expectation:expectations[i-1] index:i]; + } + }]; + + [self waitForExpectationsWithTimeout:kAsyncTestTimeout * maxIndex handler:nil]; +} + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wshadow" +- (void)createLIFOOperationWithDownloader:(SDWebImageDownloader *)downloader expectation:(XCTestExpectation *)expectation index:(int)index { + int waitIndex = 7; + int maxIndex = 14; + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", index]]; + [self.executionOrderURLs addObject:url]; + [downloader downloadImageWithURL:url options:0 progress:^(NSInteger receivedSize, NSInteger expectedSize, NSURL * _Nullable targetURL) { + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", waitIndex]]; + if ([self.executionOrderURLs containsObject:url]) { + return; + } + + } completed:^(UIImage * _Nullable image, NSData * _Nullable data, NSError * _Nullable error, BOOL finished) { + printf("URL%d finished\n", index); + NSMutableArray *pendingArray = [NSMutableArray array]; + if (index == 1) { + // 1 + for (int j = 1; j <= waitIndex; j++) { + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", j]]; + [pendingArray addObject:url]; + } + } else if (index == waitIndex) { + // 7 + for (int j = 2; j <= maxIndex; j++) { + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", j]]; + [pendingArray addObject:url]; + } + } else if (index > waitIndex) { + // 8-14 + for (int j = 2; j <= index; j++) { + if (j == waitIndex) continue; + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", j]]; + [pendingArray addObject:url]; + } + } else if (index < waitIndex) { + // 2-6 + for (int j = 2; j <= index; j++) { + if (j == waitIndex) continue; + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", j]]; + [pendingArray addObject:url]; + } + } + expect(self.executionOrderURLs).equal(pendingArray); + NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://via.placeholder.com/1000x%d.png", index]]; + [self.executionOrderURLs removeObject:url]; + [expectation fulfill]; + }]; +} +#pragma clang diagnostic pop + - (void)test17ThatMinimumProgressIntervalWorks { XCTestExpectation *expectation = [self expectationWithDescription:@"Minimum progress interval"]; SDWebImageDownloaderConfig *config = SDWebImageDownloaderConfig.defaultDownloaderConfig; From 76a6beb029d6deb28604c4feea5fc97da7ff6706 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 26 Sep 2019 22:47:09 +0800 Subject: [PATCH 2/2] Fix the LIFO execute order issue, we should make all previous operation dependent the new operation, Foundation will take care of dependency graph and calculate the order --- SDWebImage/Core/SDWebImageDownloader.m | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/SDWebImage/Core/SDWebImageDownloader.m b/SDWebImage/Core/SDWebImageDownloader.m index 4d8a0e69..1b9fd119 100644 --- a/SDWebImage/Core/SDWebImageDownloader.m +++ b/SDWebImage/Core/SDWebImageDownloader.m @@ -38,7 +38,6 @@ static void * SDWebImageDownloaderContext = &SDWebImageDownloaderContext; @interface SDWebImageDownloader () @property (strong, nonatomic, nonnull) NSOperationQueue *downloadQueue; -@property (weak, nonatomic, nullable) NSOperation *lastAddedOperation; @property (strong, nonatomic, nonnull) NSMutableDictionary *> *URLOperations; @property (strong, nonatomic, nullable) NSMutableDictionary *HTTPHeaders; @property (strong, nonatomic, nonnull) dispatch_semaphore_t HTTPHeadersLock; // A lock to keep the access to `HTTPHeaders` thread-safe @@ -321,9 +320,12 @@ static void * SDWebImageDownloaderContext = &SDWebImageDownloaderContext; } if (self.config.executionOrder == SDWebImageDownloaderLIFOExecutionOrder) { - // Emulate LIFO execution order by systematically adding new operations as last operation's dependency - [self.lastAddedOperation addDependency:operation]; - self.lastAddedOperation = operation; + // Emulate LIFO execution order by systematically, each previous adding operation can dependency the new operation + // This can gurantee the new operation to be execulated firstly, even if when some operations finished, meanwhile you appending new operations + // Just make last added operation dependents new operation can not solve this problem. See test case #test15DownloaderLIFOExecutionOrder + for (NSOperation *pendingOperation in self.downloadQueue.operations) { + [pendingOperation addDependency:operation]; + } } return operation;