Skip to content

Commit

Permalink
Bugfix - Request Cancellation Race Conditions in ImageDownloader (#372)
Browse files Browse the repository at this point in the history
* Updated Alamofire submodule and pod dependency to 5.0.0-beta.7

* Fixed several issues related to async task creation in ImageDownloader
  • Loading branch information
cnoon authored Aug 28, 2019
1 parent 7f4bc4a commit 24ab6b9
Show file tree
Hide file tree
Showing 6 changed files with 237 additions and 28 deletions.
2 changes: 1 addition & 1 deletion AlamofireImage.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@ Pod::Spec.new do |s|

s.swift_version = '5.0'

s.dependency 'Alamofire', '~> 5.0.0-beta.6'
s.dependency 'Alamofire', '~> 5.0.0-beta.7'
end
16 changes: 16 additions & 0 deletions AlamofireImage.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,9 @@
4C86C1121DA0B3400032ECC3 /* UIImageViewTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4C62D30F1B96C1500011B036 /* UIImageViewTests.swift */; };
4C86C1131DA0B3440032ECC3 /* UIImage+AlamofireImageTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4C0893EB1B936A7A005125D9 /* UIImage+AlamofireImageTests.swift */; };
4C96A4781AAE9488008AE0B6 /* ImageDownloader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4C96A4771AAE9488008AE0B6 /* ImageDownloader.swift */; };
4CBE8FB72316008300782A2E /* ImageDownloaderStressTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CBE8FB62316008300782A2E /* ImageDownloaderStressTests.swift */; };
4CBE8FB82316008300782A2E /* ImageDownloaderStressTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CBE8FB62316008300782A2E /* ImageDownloaderStressTests.swift */; };
4CBE8FB92316008300782A2E /* ImageDownloaderStressTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CBE8FB62316008300782A2E /* ImageDownloaderStressTests.swift */; };
4CD5BCEA1D7F9D1E0055E232 /* AFIError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CD5BCE91D7F9D1E0055E232 /* AFIError.swift */; };
4CD5BCEB1D7F9D1E0055E232 /* AFIError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CD5BCE91D7F9D1E0055E232 /* AFIError.swift */; };
4CD5BCEC1D7F9D1E0055E232 /* AFIError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CD5BCE91D7F9D1E0055E232 /* AFIError.swift */; };
Expand Down Expand Up @@ -745,6 +748,7 @@
4C9043821AABBFC5001B4E60 /* AlamofireImage iOS Tests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "AlamofireImage iOS Tests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; };
4C96A4771AAE9488008AE0B6 /* ImageDownloader.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ImageDownloader.swift; sourceTree = "<group>"; };
4CB2B2F31C0270C500B442EA /* Info-tvOS.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = "Info-tvOS.plist"; sourceTree = "<group>"; };
4CBE8FB62316008300782A2E /* ImageDownloaderStressTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageDownloaderStressTests.swift; sourceTree = "<group>"; };
4CD5BCE91D7F9D1E0055E232 /* AFIError.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AFIError.swift; sourceTree = "<group>"; };
4CD5BCF41D7FBE380055E232 /* AFError+AlamofireImageTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "AFError+AlamofireImageTests.swift"; sourceTree = "<group>"; };
4CE611321AABC24E00D35044 /* AlamofireImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = AlamofireImage.framework; sourceTree = BUILT_PRODUCTS_DIR; };
Expand Down Expand Up @@ -1236,6 +1240,14 @@
name = Products;
sourceTree = "<group>";
};
4CBE8FB52316006100782A2E /* Stress Tests */ = {
isa = PBXGroup;
children = (
4CBE8FB62316008300782A2E /* ImageDownloaderStressTests.swift */,
);
name = "Stress Tests";
sourceTree = "<group>";
};
4CD5BCF21D7FBE110055E232 /* Tests */ = {
isa = PBXGroup;
children = (
Expand All @@ -1248,6 +1260,7 @@
4C0893F41B937404005125D9 /* UIImageTests.swift */,
4C62D30F1B96C1500011B036 /* UIImageViewTests.swift */,
4C0893EA1B936A4D005125D9 /* Extensions */,
4CBE8FB52316006100782A2E /* Stress Tests */,
);
name = Tests;
sourceTree = "<group>";
Expand Down Expand Up @@ -2064,6 +2077,7 @@
4C16B3AE1BA93ADB00A66EF0 /* UIImage+AlamofireImageTests.swift in Sources */,
4C16B3AA1BA93ADB00A66EF0 /* ImageFilterTests.swift in Sources */,
4C16B3A71BA93ADB00A66EF0 /* BaseTestCase.swift in Sources */,
4CBE8FB92316008300782A2E /* ImageDownloaderStressTests.swift in Sources */,
4C16B3AD1BA93ADB00A66EF0 /* UIImageViewTests.swift in Sources */,
4C16B3A91BA93ADB00A66EF0 /* ImageDownloaderTests.swift in Sources */,
4C16B3AC1BA93ADB00A66EF0 /* UIImageTests.swift in Sources */,
Expand Down Expand Up @@ -2114,6 +2128,7 @@
4C5872CA1B93DEAC00407E58 /* ImageFilterTests.swift in Sources */,
4CEBB5401B93C622001391DE /* ImageCacheTests.swift in Sources */,
4C0893EC1B936A7A005125D9 /* UIImage+AlamofireImageTests.swift in Sources */,
4CBE8FB72316008300782A2E /* ImageDownloaderStressTests.swift in Sources */,
4C1624851AABE8E600A0385D /* BaseTestCase.swift in Sources */,
4C0893F51B937404005125D9 /* UIImageTests.swift in Sources */,
4C5874861B93F81800407E58 /* ImageDownloaderTests.swift in Sources */,
Expand Down Expand Up @@ -2148,6 +2163,7 @@
4C1624861AABE8E600A0385D /* BaseTestCase.swift in Sources */,
4CEBB5411B93C622001391DE /* ImageCacheTests.swift in Sources */,
4C86C1121DA0B3400032ECC3 /* UIImageViewTests.swift in Sources */,
4CBE8FB82316008300782A2E /* ImageDownloaderStressTests.swift in Sources */,
4C5874871B93F81800407E58 /* ImageDownloaderTests.swift in Sources */,
4C86C1131DA0B3440032ECC3 /* UIImage+AlamofireImageTests.swift in Sources */,
4C0897EC1B93BC0D005125D9 /* RequestTests.swift in Sources */,
Expand Down
2 changes: 1 addition & 1 deletion Carthage/Checkouts/Alamofire
Submodule Alamofire updated 277 files
38 changes: 16 additions & 22 deletions Source/ImageDownloader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -322,19 +322,18 @@ open class ImageDownloader {
request.response(
queue: self.responseQueue,
responseSerializer: imageResponseSerializer,
completionHandler: { [weak self] response in
guard let strongSelf = self, let request = response.request else { return }

completionHandler: { response in
defer {
strongSelf.safelyDecrementActiveRequestCount()
strongSelf.safelyStartNextRequestIfNecessary()
self.safelyDecrementActiveRequestCount()
self.safelyStartNextRequestIfNecessary()
}

// Early out if the request has changed out from under us
let handler = strongSelf.safelyFetchResponseHandler(withURLIdentifier: urlID)
guard handler?.handlerID == handlerID else { return }

guard let responseHandler = strongSelf.safelyRemoveResponseHandler(withURLIdentifier: urlID) else {
guard
let handler = self.safelyFetchResponseHandler(withURLIdentifier: urlID),
handler.handlerID == handlerID,
let responseHandler = self.safelyRemoveResponseHandler(withURLIdentifier: urlID)
else {
return
}

Expand All @@ -356,7 +355,9 @@ open class ImageDownloader {
filteredImage = image
}

strongSelf.imageCache?.add(filteredImage, for: request, withIdentifier: filter?.identifier)
if let request = response.request {
self.imageCache?.add(filteredImage, for: request, withIdentifier: filter?.identifier)
}

DispatchQueue.main.async {
let response = DataResponse<Image>(
Expand Down Expand Up @@ -476,7 +477,7 @@ open class ImageDownloader {
DispatchQueue.main.async { operation.completion?(response) }
}

if responseHandler.operations.isEmpty && requestReceipt.request.task?.state == .suspended {
if responseHandler.operations.isEmpty {
requestReceipt.request.cancel()
self.responseHandlers.removeValue(forKey: urlID)
}
Expand Down Expand Up @@ -509,22 +510,15 @@ open class ImageDownloader {
synchronizationQueue.sync {
guard self.isActiveRequestCountBelowMaximumLimit() else { return }

let states: Set<Request.State> = [.initialized, .suspended]
guard let request = self.dequeue() else { return }

while !self.queuedRequests.isEmpty {
if let request = self.dequeue(), states.contains(request.state) {
self.start(request)
break
}
}
self.start(request)
}
}

func safelyDecrementActiveRequestCount() {
self.synchronizationQueue.sync {
if self.activeRequestCount > 0 {
self.activeRequestCount -= 1
}
synchronizationQueue.sync {
self.activeRequestCount -= 1
}
}

Expand Down
199 changes: 199 additions & 0 deletions Tests/ImageDownloaderStressTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
//
// ImageDownloaderStressTests.swift
//
// Copyright (c) 2019 Alamofire Software Foundation (http://alamofire.org/)
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//

@testable import Alamofire
@testable import AlamofireImage
import Foundation
import XCTest

class ImageDownloaderStressTestCase: BaseTestCase {
let imageCount = 10
var kittenCache: Set<String> = []

// MARK: - Setup and Teardown

override func setUp() {
super.setUp()
kittenCache.removeAll()
}

// MARK: - Tests - Common Use Cases

func testThatItCanDownloadManyImagesInParallel() {
// Given
let imageRequests = (1...imageCount).map { _ in randomKittenURLRequest() }
let imageDownloader = ImageDownloader(configuration: .ephemeral)

let expect = expectation(description: "all requests should complete")
expect.expectedFulfillmentCount = imageRequests.count

var receipts: [RequestReceipt] = []
var responses: [DataResponse<Image>] = []

// When
for imageRequest in imageRequests {
let receipt = imageDownloader.download(imageRequest) { response in
responses.append(response)
expect.fulfill()
}

receipt.flatMap { receipts.append($0) }
}

waitForExpectations(timeout: 10, handler: nil)

// Then
XCTAssertEqual(receipts.count, imageCount)
XCTAssertEqual(responses.count, imageCount)
responses.forEach { XCTAssertTrue($0.result.isSuccess) }
}

func testThatItCanDownloadManyImagesInParallelWhileCancellingRequests() {
// Given
let cancelledImageCount = 4
let imageRequests = (1...imageCount).map { _ in randomKittenURLRequest() }
let imageDownloader = ImageDownloader(configuration: .ephemeral)

let expect = expectation(description: "all requests should complete")
expect.expectedFulfillmentCount = imageRequests.count

var receipts: [RequestReceipt] = []
var responses: [DataResponse<Image>] = []

// When
for imageRequest in imageRequests {
let receipt = imageDownloader.download(imageRequest) { response in
responses.append(response)
expect.fulfill()
}

receipt.flatMap { receipts.append($0) }
}

receipts.suffix(cancelledImageCount).forEach { imageDownloader.cancelRequest(with: $0) }

waitForExpectations(timeout: 10, handler: nil)

// Then
XCTAssertEqual(receipts.count, imageCount)
XCTAssertEqual(responses.count, imageCount)

let successCount = responses.reduce(0) { count, response in response.result.isSuccess ? count + 1 : count }
let failureCount = responses.reduce(0) { count, response in response.result.isFailure ? count + 1 : count }

XCTAssertEqual(successCount, imageCount - cancelledImageCount)
XCTAssertEqual(failureCount, cancelledImageCount)
}

// MARK: - Tests - Uncommon Use Cases (External Abuse)

func testThatItCanDownloadManyImagesInParallelWhileResumingRequestsExternally() {
// Given
let imageRequests = (1...imageCount).map { _ in randomKittenURLRequest() }
let imageDownloader = ImageDownloader(configuration: .ephemeral)

let expect = expectation(description: "all requests should complete")
expect.expectedFulfillmentCount = imageRequests.count

var receipts: [RequestReceipt] = []
var responses: [DataResponse<Image>] = []

// When
for imageRequest in imageRequests {
let receipt = imageDownloader.download(imageRequest) { response in
responses.append(response)
expect.fulfill()
}

receipt.flatMap { receipts.append($0) }
}

receipts.suffix(4).forEach { $0.request.resume() }

waitForExpectations(timeout: 10, handler: nil)

// Then
XCTAssertEqual(receipts.count, imageCount)
XCTAssertEqual(responses.count, imageCount)
responses.forEach { XCTAssertTrue($0.result.isSuccess) }
}

func testThatItCanDownloadManyImagesInParallelWhileCancellingRequestsExternally() {
// Given
let cancelledImageCount = 4
let imageRequests = (1...imageCount).map { _ in randomKittenURLRequest() }
let imageDownloader = ImageDownloader(configuration: .ephemeral)

let expect = expectation(description: "all requests should complete")
expect.expectedFulfillmentCount = imageRequests.count

var receipts: [RequestReceipt] = []
var responses: [DataResponse<Image>] = []

// When
for imageRequest in imageRequests {
let receipt = imageDownloader.download(imageRequest) { response in
responses.append(response)
expect.fulfill()
}

receipt.flatMap { receipts.append($0) }
}

receipts.suffix(cancelledImageCount).forEach { $0.request.cancel() }

waitForExpectations(timeout: 10, handler: nil)

// Then
XCTAssertEqual(receipts.count, imageCount)
XCTAssertEqual(responses.count, imageCount)

let successCount = responses.reduce(0) { count, response in response.result.isSuccess ? count + 1 : count }
let failureCount = responses.reduce(0) { count, response in response.result.isFailure ? count + 1 : count }

XCTAssertEqual(successCount, imageCount - cancelledImageCount)
XCTAssertEqual(failureCount, cancelledImageCount)
}

private func randomKittenURLRequest() -> URLRequest {
let urlString = uniqueKittenURLString()
let url = URL(string: urlString)!

return URLRequest(url: url)
}

private func uniqueKittenURLString() -> String {
let width = Int.random(in: 100...400)
let height = Int.random(in: 100...400)

let urlString = "https://placekitten.com/\(width)/\(height)"

if kittenCache.contains(urlString) {
return uniqueKittenURLString()
} else {
kittenCache.insert(urlString)
return urlString
}
}
}
8 changes: 4 additions & 4 deletions Tests/ImageDownloaderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -187,16 +187,16 @@ class ImageDownloaderTestCase: BaseTestCase {
let urlRequest2 = try! URLRequest(url: "https://httpbin.org/image/png", method: .get)

let expectation = self.expectation(description: "both downloads should succeed")
var completedDownloads = 0
expectation.expectedFulfillmentCount = 2

var completedDownloads = 0
var results: [AFResult<Image>] = []

// When
downloader.download([urlRequest1, urlRequest2], filter: nil) { closureResponse in
results.append(closureResponse.result)

completedDownloads += 1
if completedDownloads == 2 { expectation.fulfill() }
expectation.fulfill()
}

let activeRequestCount = downloader.activeRequestCount
Expand Down Expand Up @@ -488,7 +488,7 @@ class ImageDownloaderTestCase: BaseTestCase {
let downloader = ImageDownloader()
let urlRequest = try! URLRequest(url: "https://httpbin.org/image/jpeg", method: .get)

let expectation = self.expectation(description: "download request should succeed")
let expectation = self.expectation(description: "download request should cancel")

var response: DataResponse<Image>?

Expand Down

0 comments on commit 24ab6b9

Please # to comment.