-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fixed image downloading cancellation race condition #3325
Conversation
Current coverage is
|
Thanks for addressing this issue that fast! 👍🏻 Unfortunately the unit test example I posted within the issue #3324 is wrong. The successCount should equal the number of requests. I also removed the image URLs from the test since they were fairly large downloads. The unit test now fails again, because now neither the failure block nor the success block get called anymore of the requests whose indices exceed the value in "maximumActiveDownloads". The fixed unit test: - (void)testThatItCanDownloadAndCancelAndDownloadAgain {
AFImageDownloader *downloader = [[AFImageDownloader alloc] initWithSessionManager:self.downloader.sessionManager
downloadPrioritization:self.downloader.downloadPrioritizaton
maximumActiveDownloads:1
imageCache:self.downloader.imageCache];
XCTestExpectation *expectation = [self expectationWithDescription:@"no download should fail"];
__block NSUInteger successCount = 0;
NSArray *imageURLRequests = @[self.pngRequest, self.jpegRequest];
for (NSURLRequest *imageURLRequest in imageURLRequests) {
AFImageDownloadReceipt *receipt = [downloader downloadImageForURLRequest:imageURLRequest success:nil failure:nil];
[downloader cancelTaskForImageDownloadReceipt:receipt];
}
for (NSURLRequest *imageURLRequest in imageURLRequests) {
[downloader downloadImageForURLRequest:imageURLRequest success:^(NSURLRequest * _Nonnull request, NSHTTPURLResponse * _Nullable response, UIImage * _Nonnull responseObject) {
successCount++;
if (successCount == [imageURLRequests count]) {
[expectation fulfill];
}
} failure:^(NSURLRequest * _Nonnull request, NSHTTPURLResponse * _Nullable response, NSError * _Nonnull error) {
[expectation fulfill];
}];
}
[self waitForExpectationsWithCommonTimeoutUsingHandler:nil];
XCTAssertTrue(successCount == [imageURLRequests count], @"all downloads should succeed");
} |
You are right. I'll update that and then dig further. The root of the issue here is cancelling a request and kicking it off again before the original one has a to chance to cancel, and only if that request to be cancelled is in the suspended state. |
e1931e0
to
b351251
Compare
Thanks @florianschulz I've updated the unit test to be a bit better, and improved my patch. Let me know your thoughts. |
Great job! It's working now for me. ✌️ Just as a suggestion, I would still replace the image URLs in the test before merging it into master, since due to their download size, there is a chance of getting an expectation timeout under bad network conditions. It is also possible that the images will be removed from the server in the future. I couldn't find a good source for reliable image downloads, so I modified the test (as suggested above) to download only two images from httpbin and setting the maximumActiveDownloads to 1. |
Thanks for the tip. Just switched over to gravatar. |
Tests failed due to a certificate expiring. Need to get that one fixed first. |
c709c8a
to
124811e
Compare
Addresses #3324
Fixed issue where test could still fail if cancelled in a certain order
…condition Fixed image downloading cancellation race condition
This line is added in [PR AFNetworking#3325](https://github.com/AFNetworking/AFNetworking/pull/3325/files#diff-9d9536229ce0eada1e165adb747fc722R232), I reckon it's supposed to use `strongSelf` rather than `self`.
This line is added in [PR AFNetworking#3325](https://github.com/AFNetworking/AFNetworking/pull/3325/files#diff-9d9536229ce0eada1e165adb747fc722R232), I reckon it's supposed to use `strongSelf` rather than `self`.
Addresses #3324