Skip to content

Commit

Permalink
Merge pull request onevcat#698 from onevcat/fix/refactor-tests
Browse files Browse the repository at this point in the history 
Cancel semaphore to solve cancel timing race
  • Loading branch information
@onevcat
onevcat authored Jun 2, 2017
2 parents 5d541c6 + 327d499 commit 2e985b8
Show file tree
Hide file tree
Showing 10 changed files with 168 additions and 161 deletions.
51 changes: 37 additions & 14 deletions Sources/ImageDownloader.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ open class ImageDownloader {

var downloadTaskCount = 0
var downloadTask: RetrieveImageDownloadTask?
var cancelSemaphore: DispatchSemaphore?
}

// MARK: - Public property
Expand Down Expand Up @@ -219,6 +220,7 @@ open class ImageDownloader {
// MARK: - Internal property
let barrierQueue: DispatchQueue
let processQueue: DispatchQueue
let cancelQueue: DispatchQueue

typealias CallbackPair = (progressBlock: ImageDownloaderProgressBlock?, completionHandler: ImageDownloaderCompletionHandler?)

Expand All @@ -242,6 +244,7 @@ open class ImageDownloader {

barrierQueue = DispatchQueue(label: "com.onevcat.Kingfisher.ImageDownloader.Barrier.\(name)", attributes: .concurrent)
processQueue = DispatchQueue(label: "com.onevcat.Kingfisher.ImageDownloader.Process.\(name)", attributes: .concurrent)
cancelQueue = DispatchQueue(label: "com.onevcat.Kingfisher.ImageDownloader.Cancel.\(name)")

sessionHandler = ImageDownloaderSessionHandler()

Expand All @@ -256,7 +259,7 @@ open class ImageDownloader {

func fetchLoad(for url: URL) -> ImageFetchLoad? {
var fetchLoad: ImageFetchLoad?
barrierQueue.sync { fetchLoad = fetchLoads[url] }
barrierQueue.sync(flags: .barrier) { fetchLoad = fetchLoads[url] }
return fetchLoad
}

Expand Down Expand Up @@ -312,7 +315,7 @@ open class ImageDownloader {

dataTask.priority = options?.downloadPriority ?? URLSessionTask.defaultPriority
dataTask.resume()
delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request)
self.delegate?.imageDownloader(self, willDownloadImageForURL: url, with: request)

// Hold self while the task is executing.
self.sessionHandler.downloadHolder = self
Expand All @@ -332,24 +335,39 @@ open class ImageDownloader {
extension ImageDownloader {

// A single key may have multiple callbacks. Only download once.
func setup(progressBlock: ImageDownloaderProgressBlock?, with completionHandler: ImageDownloaderCompletionHandler?, for url: URL, options: KingfisherOptionsInfo?, started: ((URLSession, ImageFetchLoad) -> Void)) {
func setup(progressBlock: ImageDownloaderProgressBlock?, with completionHandler: ImageDownloaderCompletionHandler?, for url: URL, options: KingfisherOptionsInfo?, started: @escaping ((URLSession, ImageFetchLoad) -> Void)) {

barrierQueue.sync(flags: .barrier) {
let loadObjectForURL = fetchLoads[url] ?? ImageFetchLoad()
let callbackPair = (progressBlock: progressBlock, completionHandler: completionHandler)

loadObjectForURL.contents.append((callbackPair, options ?? KingfisherEmptyOptionsInfo))

fetchLoads[url] = loadObjectForURL

if let session = session {
started(session, loadObjectForURL)
func prepareFetchLoad() {
barrierQueue.sync(flags: .barrier) {
let loadObjectForURL = fetchLoads[url] ?? ImageFetchLoad()
let callbackPair = (progressBlock: progressBlock, completionHandler: completionHandler)

loadObjectForURL.contents.append((callbackPair, options ?? KingfisherEmptyOptionsInfo))

fetchLoads[url] = loadObjectForURL

if let session = session {
started(session, loadObjectForURL)
}
}
}

if let fetchLoad = fetchLoad(for: url), fetchLoad.downloadTaskCount == 0 {
if fetchLoad.cancelSemaphore == nil {
fetchLoad.cancelSemaphore = DispatchSemaphore(value: 0)
}
cancelQueue.async {
_ = fetchLoad.cancelSemaphore?.wait(timeout: .distantFuture)
fetchLoad.cancelSemaphore = nil
prepareFetchLoad()
}
} else {
prepareFetchLoad()
}
}

func cancelDownloadingTask(_ task: RetrieveImageDownloadTask) {
barrierQueue.sync {
barrierQueue.sync(flags: .barrier) {
if let URL = task.internalTask.originalRequest?.url, let imageFetchLoad = self.fetchLoads[URL] {
imageFetchLoad.downloadTaskCount -= 1
if imageFetchLoad.downloadTaskCount == 0 {
Expand Down Expand Up @@ -464,6 +482,11 @@ class ImageDownloaderSessionHandler: NSObject, URLSessionDataDelegate, Authentic
// We need to clean the fetch load first, before actually calling completion handler.
cleanFetchLoad(for: url)

var leftSignal: Int
repeat {
leftSignal = fetchLoad.cancelSemaphore?.signal() ?? 0
} while leftSignal != 0

for content in fetchLoad.contents {
content.options.callbackDispatchQueue.safeAsync {
content.callback.completionHandler?(nil, error as NSError, url, nil)
Expand Down
3 changes: 1 addition & 2 deletions Sources/ImageView+Kingfisher.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,21 @@ extension Kingfisher where Base: ImageView {
},
completionHandler: {[weak base] image, error, cacheType, imageURL in
DispatchQueue.main.safeAsync {
maybeIndicator?.stopAnimatingView()
guard let strongBase = base, imageURL == self.webURL else {
completionHandler?(image, error, cacheType, imageURL)
return
}

self.setImageTask(nil)
guard let image = image else {
maybeIndicator?.stopAnimatingView()
completionHandler?(nil, error, cacheType, imageURL)
return
}

guard let transitionItem = options.lastMatchIgnoringAssociatedValue(.transition(.none)),
case .transition(let transition) = transitionItem, ( options.forceTransition || cacheType == .none) else
{
maybeIndicator?.stopAnimatingView()
strongBase.image = image
completionHandler?(image, error, cacheType, imageURL)
return
Expand Down
36 changes: 22 additions & 14 deletions Sources/Indicator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,34 +86,42 @@ extension Indicator {

// MARK: - ActivityIndicator
// Displays a NSProgressIndicator / UIActivityIndicatorView
struct ActivityIndicator: Indicator {
class ActivityIndicator: Indicator {

#if os(macOS)
private let activityIndicatorView: NSProgressIndicator
#else
private let activityIndicatorView: UIActivityIndicatorView
#endif
private var animatingCount = 0

var view: IndicatorView {
return activityIndicatorView
}

func startAnimatingView() {
#if os(macOS)
activityIndicatorView.startAnimation(nil)
#else
activityIndicatorView.startAnimating()
#endif
activityIndicatorView.isHidden = false
animatingCount += 1
// Alrady animating
if animatingCount == 1 {
#if os(macOS)
activityIndicatorView.startAnimation(nil)
#else
activityIndicatorView.startAnimating()
#endif
activityIndicatorView.isHidden = false
}
}

func stopAnimatingView() {
#if os(macOS)
activityIndicatorView.stopAnimation(nil)
#else
activityIndicatorView.stopAnimating()
#endif
activityIndicatorView.isHidden = true
animatingCount = max(animatingCount - 1, 0)
if animatingCount == 0 {
#if os(macOS)
activityIndicatorView.stopAnimation(nil)
#else
activityIndicatorView.stopAnimating()
#endif
activityIndicatorView.isHidden = true
}
}

init() {
Expand All @@ -135,7 +143,7 @@ struct ActivityIndicator: Indicator {

// MARK: - ImageIndicator
// Displays an ImageView. Supports gif
struct ImageIndicator: Indicator {
class ImageIndicator: Indicator {
private let animatedImageIndicatorView: ImageView

var view: IndicatorView {
Expand Down
4 changes: 1 addition & 3 deletions Tests/KingfisherTests/ImageCacheTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,6 @@ class ImageCacheTests: XCTestCase {
group.leave()
}

group.notify(queue: DispatchQueue.main) {
completionHandler()
}
group.notify(queue: .main, execute: completionHandler)
}
}
54 changes: 41 additions & 13 deletions Tests/KingfisherTests/ImageDownloaderTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,7 @@ class ImageDownloaderTests: XCTestCase {
}
}

group.notify(queue: DispatchQueue.main) { () -> Void in
expectation.fulfill()
}

group.notify(queue: .main, execute: expectation.fulfill)
waitForExpectations(timeout: 5, handler: nil)
}

Expand All @@ -117,10 +114,7 @@ class ImageDownloaderTests: XCTestCase {
}
}

group.notify(queue: DispatchQueue.main) { () -> Void in
expectation.fulfill()
}

group.notify(queue: .main, execute: expectation.fulfill)
waitForExpectations(timeout: 5, handler: nil)
}

Expand Down Expand Up @@ -261,27 +255,61 @@ class ImageDownloaderTests: XCTestCase {
let url = URL(string: URLString)!

var progressBlockIsCalled = false
var completionBlockIsCalled = false

let downloadTask = downloader.downloadImage(with: url, progressBlock: { (receivedSize, totalSize) -> () in
progressBlockIsCalled = true
}) { (image, error, imageURL, originalData) -> () in
XCTAssertNotNil(error)
XCTAssertEqual(error!.code, NSURLErrorCancelled)
completionBlockIsCalled = true
XCTAssert(progressBlockIsCalled == false, "ProgressBlock should not be called since it is canceled.")

expectation.fulfill()
}

XCTAssertNotNil(downloadTask)

downloadTask!.cancel()
_ = stub!.go()

DispatchQueue.main.asyncAfter(deadline: DispatchTime.now() + Double(Int64(Double(NSEC_PER_SEC) * 0.09)) / Double(NSEC_PER_SEC)) { () -> Void in
expectation.fulfill()
waitForExpectations(timeout: 5, handler: nil)
}

// Issue 532 https://github.com/onevcat/Kingfisher/issues/532#issuecomment-305644311
func testCancelThenRestartSameDownload() {
let expectation = self.expectation(description: "wait for downloading")

let URLString = testKeys[0]
let stub = stubRequest("GET", URLString).andReturn(200)?.withBody(testImageData)?.delay()
let url = URL(string: URLString)!

var progressBlockIsCalled = false

let group = DispatchGroup()

group.enter()
let downloadTask = downloader.downloadImage(with: url, progressBlock: { (receivedSize, totalSize) -> () in
progressBlockIsCalled = true
}) { (image, error, imageURL, originalData) -> () in
XCTAssertNotNil(error)
XCTAssertEqual(error!.code, NSURLErrorCancelled)
XCTAssert(progressBlockIsCalled == false, "ProgressBlock should not be called since it is canceled.")
XCTAssert(completionBlockIsCalled == true, "CompletionBlock should be called with error.")
group.leave()
}

XCTAssertNotNil(downloadTask)

downloadTask!.cancel()
_ = stub!.go()

group.enter()
downloader.downloadImage(with: url, progressBlock: { (receivedSize, totalSize) -> () in
progressBlockIsCalled = true
}) { (image, error, imageURL, originalData) -> () in
XCTAssertNotNil(image)
group.leave()
}

group.notify(queue: .main, execute: expectation.fulfill)
waitForExpectations(timeout: 5, handler: nil)
}

Expand Down
9 changes: 2 additions & 7 deletions Tests/KingfisherTests/ImagePrefetcherTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,23 +102,18 @@ class ImagePrefetcherTests: XCTestCase {
progressBlock: { (skippedResources, failedResources, completedResources) -> () in
},
completionHandler: {(skippedResources, failedResources, completedResources) -> () in
expectation.fulfill()
XCTAssertEqual(skippedResources.count, 0, "There should be no items skipped.")
XCTAssertEqual(failedResources.count, urls.count, "The failed count should be the same with started downloads due to cancellation.")
XCTAssertEqual(completedResources.count, 0, "None resources prefetching should complete.")
expectation.fulfill()
})

prefetcher.maxConcurrentDownloads = maxConcurrentCount

prefetcher.start()
prefetcher.stop()

let delayTime = DispatchTime.now() + Double(Int64(0.1 * Double(NSEC_PER_SEC))) / Double(NSEC_PER_SEC)

DispatchQueue.main.asyncAfter(deadline: delayTime) {
responses.forEach { _ = $0!.go() }
}

delay(0.1) { responses.forEach { _ = $0!.go() } }
waitForExpectations(timeout: 5, handler: nil)
}

Expand Down
Loading

0 comments on commit 2e985b8

Please sign in to comment.