ImageDownloader request fails if same request was previously cancelled

[robmaceachern]

I think this is basically the same issue as https://github.com/AFNetworking/AFNetworking/issues/3324 and AFNetworking/AFNetworking#3325, so I'll reuse @florianschulz's words:

When you start downloading multiple images simultaneously, then immediately cancel these downloads and then start them again, some downloads fail with cancelled status. This seems to be tied to the value of the "maximumActiveDownloads" property of [ImageDownloader], since the number of attempted downloads has to be greater than this value in order to make some of the downloads fail. Increasing this value also solves the issue.

I've ported his test case with a minor tweak: the ImageDownloader.maximumActiveDownloads is configured to always be less than the count of URLs we're testing.

/// Tests if a set of download requests can be started, then cancelled, and then successfully downloaded
func testThatItCanDownloadAndCancelAndDownloadAgain() {
    let expectation = expectationWithDescription("all downloads should complete")
    var successCounter = 0

    let imageURLs = [
        NSURL(string: "https://static.pexels.com/photos/1562/italian-landscape-mountains-nature.jpg")!,
        NSURL(string: "https://static.pexels.com/photos/397/italian-landscape-mountains-nature.jpg")!,
        NSURL(string: "https://static.pexels.com/photos/2698/dawn-landscape-mountains-nature.jpg")!,
        NSURL(string: "https://static.pexels.com/photos/2946/dawn-nature-sunset-trees.jpg")!,
        NSURL(string: "https://static.pexels.com/photos/5021/nature-sunset-person-woman.jpg")!
    ]

    let downloader = ImageDownloader(maximumActiveDownloads: imageURLs.count - 1)

    for url in imageURLs {
        let receipt = downloader.downloadImage(URLRequest: NSURLRequest(URL: url), completion: nil)
        downloader.cancelRequestForRequestReceipt(receipt!)
    }

    for url in imageURLs {
        downloader.downloadImage(URLRequest: NSURLRequest(URL: url), completion: { (response) in
            switch response.result {
            case .Success(_):
                NSLog("success")
                successCounter = successCounter + 1
                if successCounter == imageURLs.count {
                    expectation.fulfill()
                }
            case .Failure(let error):
                NSLog("failure: \(error)")
                expectation.fulfill()
            }
        })
    }

    waitForExpectationsWithTimeout(timeout * Double(imageURLs.count), handler: nil)
    XCTAssertEqual(successCounter, imageURLs.count, "At least one restarted request did not succeed")
}

This fails using Xcode 7.3.1 testing against iOS 9.3 with AlamofireImage 2.4.0. I'm running into this problem when using the UIImageView extension on a collection view: certain collection view reloads or changes trigger this case which leaves some cells without an image.

I think the problem is that it is currently possible to append an operation to a responseHandler that has been cancelled. eg in responseHandler.operations.append(id: receiptID, filter: filter, completion: completion).

I just found a better test case in AFNetworking that ensures the original requests cancel properly too: https://github.com/AFNetworking/AFNetworking/blob/master/Tests/Tests/AFImageDownloaderTests.m#L369

[cnoon]

Thanks for the detailed write-up here @robmaceachern...much appreciated! 🍻

I've read up on those AFN issues you referenced and I'm sure there is a bug here in AFI. I'll sync with @kcharwood to make sure we get the same fixes in AFI that went into AFN. Once I get into it I'll update the ticket.

Thanks again for reporting!

[tapz]

Shouldn't AlamofireImage continue the download when the download is cancelled and then requested again? I think that's what other image caches do.

[cnoon]

Thanks for putting together this awesome write-up @robmaceachern! Also, huge shout out to our own @kcharwood for tracking everything down in AFN. Huge time saver to a pretty complicated issue. I've managed to use the same approach as @kcharwood to resolve the issue. It's very similar to the same patch we've put in place in the UIImageView extension when we switch out the url while a request is already in-flight.

I've added a failing (now passing) test and fixed the core issue in 5505066 and pushed it into master. This patch should be released here shortly in the AFI 2.4.2 release.

Thanks again to everyone involved here! 🍻

[cnoon]

Agreed @tapz...and that's exactly what it does if the request has already been attached to the URL session. However, if it hasn't, we drop the request altogether and don't ever start it to make way for the new requests coming in.