Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fixed image downloading cancellation race condition #3325

Merged
merged 3 commits into from Feb 15, 2016

Conversation

kcharwood
Copy link
Contributor

Addresses #3324

@codecov-io
Copy link

Current coverage is 74.48%

Merging #3325 into master will increase coverage by +0.37% as of 7b5487c

@@            master   #3325   diff @@
======================================
  Files            5       5       
  Stmts         3002    3002       
  Branches         0       0       
  Methods                          
======================================
+ Hit           2225    2236    +11
  Partial          0       0       
+ Missed         777     766    -11

Review entire Coverage Diff as of 7b5487c

Powered by Codecov. Updated on successful CI builds.

@florianschulz
Copy link

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");
}

@kcharwood
Copy link
Contributor Author

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.

@kcharwood kcharwood force-pushed the bug/fix_cancellation_race_condition branch from e1931e0 to b351251 Compare February 1, 2016 18:22
@kcharwood
Copy link
Contributor Author

Thanks @florianschulz

I've updated the unit test to be a bit better, and improved my patch. Let me know your thoughts.

@florianschulz
Copy link

Great job! It's working now for me. ✌️
Thanks again for addressing the issue so quickly.

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.

@kcharwood
Copy link
Contributor Author

Thanks for the tip. Just switched over to gravatar.

@kcharwood
Copy link
Contributor Author

Tests failed due to a certificate expiring. Need to get that one fixed first.

@kcharwood kcharwood force-pushed the bug/fix_cancellation_race_condition branch from c709c8a to 124811e Compare February 8, 2016 16:56
kcharwood added a commit that referenced this pull request Feb 15, 2016
…condition

Fixed image downloading cancellation race condition
@kcharwood kcharwood merged commit 32547b0 into master Feb 15, 2016
@kcharwood kcharwood deleted the bug/fix_cancellation_race_condition branch February 15, 2016 14:43
crazytonyli added a commit to crazytonyli/AFNetworking that referenced this pull request Oct 19, 2016
crazytonyli added a commit to crazytonyli/AFNetworking that referenced this pull request Nov 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants