diff --git a/CHANGELOG.md b/CHANGELOG.md index 874117d429..37e6d9a235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * Added PrivacyInfo.xcprivacy support. ([#4573](https://github.com/mapbox/mapbox-navigation-ios/pull/4573)) * Removed `NavigationEventsManager.init(activeNavigationDataSource:passiveNavigationDataSource:accessToken:mobileEventsManager:)` in favor of `NavigationEventsManager.init(activeNavigationDataSource:passiveNavigationDataSource:accessToken:)`. ([#4572](https://github.com/mapbox/mapbox-navigation-ios/pull/4572)) +* Fixed a rare issue that could lead to memory corruption under specific conditions. This was resolved by replacing the internal image downloader with brand new actor-based implementation. ([#4588](https://github.com/mapbox/mapbox-navigation-ios/pull/4588)) ## v2.17.0 diff --git a/Sources/MapboxNavigation/ImageDownload.swift b/Sources/MapboxNavigation/ImageDownload.swift deleted file mode 100644 index 12470cbbc0..0000000000 --- a/Sources/MapboxNavigation/ImageDownload.swift +++ /dev/null @@ -1,192 +0,0 @@ -import UIKit - -enum DownloadError: Error { - case serverError - case clientError - case noImageData -} - -protocol ImageDownload: URLSessionDataDelegate { - init(request: URLRequest, in session: URLSession) - func addCompletion(_ completion: @escaping CachedResponseCompletionHandler) - var isFinished: Bool { get } -} - -final class ImageDownloadOperation: Operation, ImageDownload { - @objc(ImageDownloadOperationState) - private enum State: Int { - case ready - case executing - case finished - } - - private let stateLock: NSLock = .init() - private var _state: State = .ready - - @objc - private dynamic var state: State { - get { - stateLock.lock(); defer { - stateLock.unlock() - } - return _state - } - set { - willChangeValue(forKey: #keyPath(state)) - stateLock.lock() - _state = newValue - stateLock.unlock() - didChangeValue(forKey: #keyPath(state)) - } - } - - final override var isReady: Bool { - return state == .ready && super.isReady - } - - final override var isExecuting: Bool { - return state == .executing - } - - final override var isFinished: Bool { - return state == .finished - } - - override var isConcurrent: Bool { - return true - } - - // MARK: - NSObject - - @objc - private dynamic class func keyPathsForValuesAffectingIsReady() -> Set { - return [#keyPath(state)] - } - - @objc - private dynamic class func keyPathsForValuesAffectingIsExecuting() -> Set { - return [#keyPath(state)] - } - - @objc private dynamic class func keyPathsForValuesAffectingIsFinished() -> Set { - return [#keyPath(state)] - } - - private let request: URLRequest - private let session: URLSession - - private var dataTask: URLSessionDataTask? - private var incomingData: Data? - private var completionBlocks: [CachedResponseCompletionHandler] = .init() - private let lock: NSLock = .init() - - required init(request: URLRequest, in session: URLSession) { - self.request = request - self.session = session - } - - func addCompletion(_ completion: @escaping CachedResponseCompletionHandler) { - withLock { - completionBlocks.append(completion) - } - } - - override func cancel() { - withLock { - super.cancel() - if let dataTask = dataTask { - dataTask.cancel() - incomingData = nil - self.dataTask = nil - } - state = .finished - } - } - - override func start() { - withLock { - guard !isCancelled && state != .finished else { - state = .finished; - return - } - - state = .executing - let dataTask = session.dataTask(with: self.request) - dataTask.resume() - - self.dataTask = dataTask - } - } - - // MARK: URLSessionDataDelegate - - func urlSession(_ session: URLSession, - dataTask: URLSessionDataTask, - didReceive response: URLResponse, - completionHandler: @escaping (URLSession.ResponseDisposition) -> Void) { - guard let response: HTTPURLResponse = response as? HTTPURLResponse else { - completionHandler(.cancel) - return - } - if response.statusCode < 400 { - withLock { - incomingData = Data() - } - completionHandler(.allow) - } else { - completionHandler(.cancel) - } - } - - func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) { - withLock { - incomingData?.append(data) - } - } - - func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) { - let (incomingData, completions) = withLock { () -> (Data?, [CachedResponseCompletionHandler]) in - let returnData = (self.incomingData, self.completionBlocks) - self.completionBlocks.removeAll() - return returnData - } - - let completionData: (CachedURLResponse?, Error?) - - if error != nil { - if let urlResponse = task.response as? HTTPURLResponse, - urlResponse.statusCode >= 400 { - completionData = (nil, DownloadError.serverError) - - } - else { - completionData = (nil, DownloadError.clientError) - } - } - else { - if let data = incomingData, let urlResponse = task.response { - completionData = (CachedURLResponse(response: urlResponse, data: data), nil) - } - else { - completionData = (nil, nil) - } - } - - // The motivation is to call completions outside the lock to reduce the likehood of a deadlock. - for completion in completions { - completion(completionData.0, completionData.1) - } - - withLock { - dataTask = nil - } - state = .finished - } - - private func withLock(_ perform: () throws -> ReturnValue) rethrows -> ReturnValue { - lock.lock(); defer { - lock.unlock() - } - return try perform() - } -} diff --git a/Sources/MapboxNavigation/ImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader.swift deleted file mode 100644 index 3f84829a74..0000000000 --- a/Sources/MapboxNavigation/ImageDownloader.swift +++ /dev/null @@ -1,119 +0,0 @@ -import Foundation -import MapboxDirections -import UIKit - -typealias CachedResponseCompletionHandler = (CachedURLResponse?, Error?) -> Void -typealias ImageDownloadCompletionHandler = (DownloadError?) -> Void - -protocol ReentrantImageDownloader { - func download(with url: URL, completion: CachedResponseCompletionHandler?) -> Void - func activeOperation(with url: URL) -> ImageDownload? - func setOperationType(_ operationType: ImageDownload.Type?) -} - -class ImageDownloader: NSObject, ReentrantImageDownloader, URLSessionDataDelegate { - private let sessionConfiguration: URLSessionConfiguration - - private let urlSession: URLSession - private let downloadQueue: OperationQueue - private let accessQueue: DispatchQueue - - private var operationType: ImageDownload.Type - private var operations: [URL: ImageDownload] = [:] - - init(sessionConfiguration: URLSessionConfiguration = .default, - operationType: ImageDownload.Type = ImageDownloadOperation.self) { - self.sessionConfiguration = sessionConfiguration - self.operationType = operationType - - self.downloadQueue = OperationQueue() - self.downloadQueue.name = Bundle.mapboxNavigation.bundleIdentifier! + ".ImageDownloader" - self.accessQueue = DispatchQueue(label: Bundle.mapboxNavigation.bundleIdentifier! + ".ImageDownloaderInternal") - - let urlSessionDelegateProxy = URLSessionDelegateProxy() - urlSession = URLSession(configuration: sessionConfiguration, delegate: urlSessionDelegateProxy, delegateQueue: nil) - super.init() - - urlSessionDelegateProxy.delegate = self - } - - deinit { - self.downloadQueue.cancelAllOperations() - urlSession.invalidateAndCancel() - } - - func download(with url: URL, completion: CachedResponseCompletionHandler?) { - accessQueue.sync { - let request = URLRequest(url) - var operation: ImageDownload - if let activeOperation = self.unsafeActiveOperation(with: url) { - operation = activeOperation - } else { - operation = self.operationType.init(request: request, in: self.urlSession) - self.operations[url] = operation - if let operation = operation as? Operation { - self.downloadQueue.addOperation(operation) - } - } - if let completion = completion { - operation.addCompletion(completion) - } - } - } - - func activeOperation(with url: URL) -> ImageDownload? { - return accessQueue.sync { unsafeActiveOperation(with: url) } - } - - private func unsafeActiveOperation(with url: URL) -> ImageDownload? { - guard let operation = operations[url], !operation.isFinished else { - return nil - } - return operation - } - - func setOperationType(_ operationType: ImageDownload.Type?) { - accessQueue.sync { - if let operationType = operationType { - self.operationType = operationType - } else { - self.operationType = ImageDownloadOperation.self - } - } - } - - // MARK: URLSessionDataDelegate - - func urlSession(_ session: URLSession, - dataTask: URLSessionDataTask, - didReceive response: URLResponse, - completionHandler: @escaping (URLSession.ResponseDisposition) -> Void) { - guard let response: HTTPURLResponse = response as? HTTPURLResponse, - let url = response.url, - let operation: ImageDownload = activeOperation(with: url) else { - completionHandler(.cancel) - return - } - - operation.urlSession?(session, dataTask: dataTask, didReceive: response, completionHandler: completionHandler) - } - - func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) { - guard let url = dataTask.originalRequest?.url, - let operation: ImageDownload = activeOperation(with: url) else { - return - } - operation.urlSession?(session, dataTask: dataTask, didReceive: data) - } - - func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) { - guard let url = task.originalRequest?.url, - let operation: ImageDownload = activeOperation(with: url) else { - return - } - operation.urlSession?(session, task: task, didCompleteWithError: error) - accessQueue.sync { - self.operations[url] = nil - } - } -} diff --git a/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift new file mode 100644 index 0000000000..9cb1523f07 --- /dev/null +++ b/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift @@ -0,0 +1,47 @@ +import Foundation + +enum DownloadError: Error { + case serverError + case clientError + case noImageData +} + +@available(iOS 13.0, *) +actor ImageDownloader: ImageDownloaderProtocol { + private let urlSession: URLSession + + private var inflightRequests: [URL: Task] = [:] + + init(configuration: URLSessionConfiguration? = nil) { + let defaultConfiguration = URLSessionConfiguration.default + defaultConfiguration.urlCache = URLCache(memoryCapacity: 5 * 1024 * 1024, diskCapacity: 20 * 1024 * 1024) + self.urlSession = URLSession(configuration: configuration ?? defaultConfiguration) + } + + nonisolated func download(with url: URL, completion: @escaping (Result) -> Void) { + Task { + do { + let response = try await self.fetch(url) + completion(.success(response)) + } catch { + completion(.failure(error)) + } + } + } + + private func fetch(_ url: URL) async throws -> CachedURLResponse { + if let inflightTask = inflightRequests[url] { + return try await inflightTask.value + } + + let downloadTask = Task { + let (data, response) = try await urlSession.data(from: url) + return CachedURLResponse(response: response, data: data) + } + + inflightRequests[url] = downloadTask + defer { inflightRequests[url] = nil } + + return try await downloadTask.value + } +} diff --git a/Sources/MapboxNavigation/ImageDownloader/ImageDownloaderProtocol.swift b/Sources/MapboxNavigation/ImageDownloader/ImageDownloaderProtocol.swift new file mode 100644 index 0000000000..786b155600 --- /dev/null +++ b/Sources/MapboxNavigation/ImageDownloader/ImageDownloaderProtocol.swift @@ -0,0 +1,5 @@ +import Foundation + +protocol ImageDownloaderProtocol { + func download(with url: URL, completion: @escaping (Result) -> Void) +} diff --git a/Sources/MapboxNavigation/ImageDownloader/LegacyImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader/LegacyImageDownloader.swift new file mode 100644 index 0000000000..466a658004 --- /dev/null +++ b/Sources/MapboxNavigation/ImageDownloader/LegacyImageDownloader.swift @@ -0,0 +1,22 @@ +import Foundation + +@available(iOS, deprecated: 13.0, message: "Use ImageDownloader instead.") +final class LegacyImageDownloader: ImageDownloaderProtocol { + private let urlSession: URLSession + + init(configuration: URLSessionConfiguration? = nil) { + let defaultConfiguration = URLSessionConfiguration.default + defaultConfiguration.urlCache = URLCache(memoryCapacity: 5 * 1024 * 1024, diskCapacity: 20 * 1024 * 1024, diskPath: nil) + self.urlSession = URLSession(configuration: configuration ?? defaultConfiguration) + } + + func download(with url: URL, completion: @escaping (Result) -> Void) { + urlSession.dataTask(with: URLRequest(url)) { data, response, error in + if let response, let data { + completion(.success(CachedURLResponse(response: response, data: data))) + } else if let error { + completion(.failure(error)) + } + }.resume() + } +} diff --git a/Sources/MapboxNavigation/ImageRepository.swift b/Sources/MapboxNavigation/ImageRepository.swift index c10c89aa3c..1a8a16e9f6 100644 --- a/Sources/MapboxNavigation/ImageRepository.swift +++ b/Sources/MapboxNavigation/ImageRepository.swift @@ -1,30 +1,29 @@ import UIKit class ImageRepository { - public var sessionConfiguration: URLSessionConfiguration { - didSet { - imageDownloader = ImageDownloader(sessionConfiguration: sessionConfiguration) - } - } - - public static let shared = ImageRepository.init() + static let shared = ImageRepository() let imageCache: BimodalImageCache - fileprivate(set) var imageDownloader: ReentrantImageDownloader - private let requestTimeOut: TimeInterval = 10 + fileprivate(set) var imageDownloader: ImageDownloaderProtocol var useDiskCache: Bool - required init(withDownloader downloader: ReentrantImageDownloader? = nil, + required init(withDownloader downloader: ImageDownloaderProtocol? = nil, cache: BimodalImageCache? = nil, useDisk: Bool = true) { - sessionConfiguration = URLSessionConfiguration.default - sessionConfiguration.timeoutIntervalForRequest = self.requestTimeOut - imageDownloader = downloader ?? ImageDownloader(sessionConfiguration: sessionConfiguration) + imageDownloader = downloader ?? Self.defaultImageDownloader imageCache = cache ?? ImageCache() useDiskCache = useDisk } + static let defaultImageDownloader: ImageDownloaderProtocol = { + if #available(iOS 13.0, *) { + return ImageDownloader() + } else { + return LegacyImageDownloader() + } + }() + func resetImageCache(_ completion: CompletionHandler?) { imageCache.clearMemory() imageCache.clearDisk(completion: completion) @@ -44,19 +43,14 @@ class ImageRepository { return } - let _ = imageDownloader.download(with: imageURL, completion: { [weak self] (cachedResponse, error) in + imageDownloader.download(with: imageURL, completion: { [weak self] result in guard let strongSelf = self, - let data = cachedResponse?.data, - let image = UIImage(data: data, scale: UIScreen.main.scale) else { + case let .success(cachedResponse) = result, + let image = UIImage(data: cachedResponse.data, scale: UIScreen.main.scale) else { completion(nil) return } - guard error == nil else { - completion(image) - return - } - strongSelf.imageCache.store(image, forKey: cacheKey, toDisk: strongSelf.useDiskCache, completion: { completion(image) }) diff --git a/Sources/MapboxNavigation/SpriteRepository.swift b/Sources/MapboxNavigation/SpriteRepository.swift index 7e3385e0d2..27582e98f8 100644 --- a/Sources/MapboxNavigation/SpriteRepository.swift +++ b/Sources/MapboxNavigation/SpriteRepository.swift @@ -3,33 +3,25 @@ import MapboxMaps import MapboxCoreNavigation import MapboxDirections +typealias ImageDownloadCompletionHandler = (DownloadError?) -> Void + class SpriteRepository { let baseURL: URL = URL(string: "https://api.mapbox.com/styles/v1")! private let defaultStyleURI: StyleURI = .navigationDay - private let requestTimeOut: TimeInterval = 10 private(set) var userInterfaceIdiomStyles = [UIUserInterfaceIdiom: StyleURI]() - private(set) var imageDownloader: ReentrantImageDownloader + private(set) var imageDownloader: ImageDownloaderProtocol let requestCache: URLCaching let derivedCache: BimodalImageCache - var sessionConfiguration: URLSessionConfiguration { - didSet { - imageDownloader = ImageDownloader(sessionConfiguration: sessionConfiguration) - } - } - static let shared = SpriteRepository.init() - init(imageDownloader: ReentrantImageDownloader? = nil, + init(imageDownloader: ImageDownloaderProtocol? = nil, requestCache: URLCaching = URLDataCache(), derivedCache: BimodalImageCache = ImageCache()) { - self.sessionConfiguration = URLSessionConfiguration.default - self.sessionConfiguration.timeoutIntervalForRequest = self.requestTimeOut self.requestCache = requestCache self.derivedCache = derivedCache - - self.imageDownloader = imageDownloader ?? ImageDownloader(sessionConfiguration: sessionConfiguration) + self.imageDownloader = imageDownloader ?? ImageRepository.defaultImageDownloader } func updateStyle(styleURI: StyleURI?, @@ -135,8 +127,8 @@ class SpriteRepository { } func downloadInfo(_ infoURL: URL, completion: @escaping (Data?) -> Void) { - imageDownloader.download(with: infoURL, completion: { [weak self] (cachedResponse, error) in - guard let strongSelf = self, let cachedResponse = cachedResponse else { + imageDownloader.download(with: infoURL, completion: { [weak self] result in + guard let strongSelf = self, case let .success(cachedResponse) = result else { completion(nil) return } @@ -147,9 +139,9 @@ class SpriteRepository { } func downloadImage(imageURL: URL, completion: @escaping (UIImage?) -> Void) { - imageDownloader.download(with: imageURL, completion: { [weak self] (cachedResponse, error) in + imageDownloader.download(with: imageURL, completion: { [weak self] result in guard let strongSelf = self, - let cachedResponse = cachedResponse, + case let .success(cachedResponse) = result, let image = UIImage(data: cachedResponse.data, scale: VisualInstruction.Component.scale) else { completion(nil) return diff --git a/Tests/MapboxNavigationTests/Helper/ImageDownloaderSpy.swift b/Tests/MapboxNavigationTests/Helper/ImageDownloaderSpy.swift new file mode 100644 index 0000000000..5a1b3eed0e --- /dev/null +++ b/Tests/MapboxNavigationTests/Helper/ImageDownloaderSpy.swift @@ -0,0 +1,15 @@ +import UIKit +import MapboxDirections +@testable import MapboxNavigation + +final class ImageDownloaderSpy: ImageDownloaderProtocol { + private var urlToCompletion = [URL: (Result) -> Void]() + + func download(with url: URL, completion: @escaping (Result) -> Void) { + urlToCompletion[url] = completion + } + + func fireCompletion(for url: URL, result: Result) { + self.urlToCompletion[url]?(result) + } +} diff --git a/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift b/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift index cb7a6d3f6f..306c61eb97 100644 --- a/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift +++ b/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift @@ -2,24 +2,18 @@ import UIKit import MapboxDirections @testable import MapboxNavigation -final class ReentrantImageDownloaderSpy: ReentrantImageDownloader { +final class ReentrantImageDownloaderSpy: ImageDownloaderProtocol { var passedDownloadUrl: URL? - var passedOperationType: ImageDownload.Type? var returnedDownloadResults = [URL: Data]() - var returnedOperation: ImageDownload? - func download(with url: URL, completion: CachedResponseCompletionHandler?) -> Void { + func download(with url: URL, completion: @escaping (Result) -> Void) { passedDownloadUrl = url let response = cachedResponse(with: returnedDownloadResults[url], url: url) - completion?(response, response == nil ? DirectionsError.noData : nil) - } - - func activeOperation(with url: URL) -> ImageDownload? { - return returnedOperation - } - - func setOperationType(_ operationType: ImageDownload.Type?) { - passedOperationType = operationType + if let response { + completion(.success(response)) + } else { + completion(.failure(DirectionsError.noData)) + } } private func cachedResponse(with data: Data?, url: URL) -> CachedURLResponse? { diff --git a/Tests/MapboxNavigationTests/ImageDownloadOperationSpy.swift b/Tests/MapboxNavigationTests/ImageDownloadOperationSpy.swift deleted file mode 100644 index 856561c57f..0000000000 --- a/Tests/MapboxNavigationTests/ImageDownloadOperationSpy.swift +++ /dev/null @@ -1,50 +0,0 @@ -import Foundation -import UIKit -@testable import MapboxNavigation - -/** - * This class can be used as a replacement for the `ImageDownloader`'s default download operation class, for spying on url download requests as well as returning canned responses ad hoc. - */ -class ImageDownloadOperationSpy: Operation, ImageDownload { - private static var operations: [URL: ImageDownloadOperationSpy] = [:] - - private(set) var request: URLRequest? - weak private var session: URLSession? - - private var completionBlocks: Array = [] - - required init(request: URLRequest, in session: URLSession) { - self.request = request - self.session = session - - super.init() - - ImageDownloadOperationSpy.operations[request.url!] = self - } - - static func reset() { - operations.removeAll() - } - - /** - * Retrieve an operation spy instance for the given URL, which can then be used to inspect and/or execute completion handlers - */ - static func operationForURL(_ URL: URL) -> ImageDownloadOperationSpy? { - return operations[URL] - } - - func addCompletion(_ completion: @escaping CachedResponseCompletionHandler) { - let wrappedCompletion = { (cachedResponse: CachedURLResponse?, error: Error?) in - completion(cachedResponse, error) - // Sadly we need to tick the run loop here to deal with the fact that the underlying implementations hop between queues. This has a similar effect to using XCTestCase's async expectations. - RunLoop.current.run(until: Date()) - } - self.completionBlocks.append(wrappedCompletion) - } - - func fireAllCompletions(_ cachedResponse: CachedURLResponse?, error: Error?) { - completionBlocks.forEach { completion in - completion(cachedResponse, error) - } - } -} diff --git a/Tests/MapboxNavigationTests/ImageDownloaderTests.swift b/Tests/MapboxNavigationTests/ImageDownloaderTests.swift index 8c1891ff4e..6b27d2d461 100644 --- a/Tests/MapboxNavigationTests/ImageDownloaderTests.swift +++ b/Tests/MapboxNavigationTests/ImageDownloaderTests.swift @@ -2,6 +2,7 @@ import XCTest @testable import MapboxNavigation import TestHelper +@available(iOS 13.0, *) class ImageDownloaderTests: TestCase { lazy var sessionConfig: URLSessionConfiguration = { let config = URLSessionConfiguration.default @@ -9,7 +10,7 @@ class ImageDownloaderTests: TestCase { return config }() - var downloader: ReentrantImageDownloader! + var downloader: ImageDownloaderProtocol! let imageURL = URL(string: "https://github.com/mapbox/mapbox-navigation-ios/blob/main/docs/img/navigation.png")! @@ -22,7 +23,7 @@ class ImageDownloaderTests: TestCase { let imageData = ShieldImage.i280.image.pngData()! ImageLoadingURLProtocolSpy.registerData(imageData, forURL: imageURL) - downloader = ImageDownloader(sessionConfiguration: sessionConfig) + downloader = ImageDownloader(configuration: sessionConfig) } override func tearDown() { @@ -33,25 +34,22 @@ class ImageDownloaderTests: TestCase { } func testDownloadingAnImage() { - var dataReturned: Data? - var errorReturned: Error? + var resultReturned: Result? let semaphore = DispatchSemaphore(value: 0) - downloader.download(with: imageURL) { (cachedResponse, error) in - dataReturned = cachedResponse?.data - errorReturned = error + downloader.download(with: imageURL) { result in + resultReturned = result semaphore.signal() } let semaphoreResult = semaphore.wait(timeout: XCTestCase.NavigationTests.timeout) XCTAssert(semaphoreResult == .success, "Semaphore timed out") - guard let data = dataReturned else { - XCTFail("Failed to download requesr") + guard case let .success(cachedResponse) = resultReturned else { + XCTFail("Failed to download request") return } - XCTAssertNil(errorReturned) - let imageReturned = UIImage(data: data, scale: UIScreen.main.scale) + let imageReturned = UIImage(data: cachedResponse.data, scale: UIScreen.main.scale) XCTAssertNotNil(imageReturned) XCTAssertTrue(imageReturned!.isKind(of: UIImage.self)) } @@ -59,138 +57,77 @@ class ImageDownloaderTests: TestCase { func testDownloadingImageWhileAlreadyInProgressAddsCallbacksWithoutAddingAnotherRequest() { var firstCallbackCalled = false var secondCallbackCalled = false - var operation: ImageDownload // URL loading is delayed in order to simulate conditions under which multiple requests for the same asset would be made ImageLoadingURLProtocolSpy.delayImageLoading() - downloader.download(with: imageURL) { (cachedResponse, error) in + downloader.download(with: imageURL) { _ in firstCallbackCalled = true } - operation = downloader.activeOperation(with: imageURL)! - downloader.download(with: imageURL) { (cachedResponse, error) in + downloader.download(with: imageURL) { _ in secondCallbackCalled = true } ImageLoadingURLProtocolSpy.resumeImageLoading() - XCTAssertTrue(operation === downloader.activeOperation(with: imageURL)!, - "Expected \(String(describing: operation)) to be identical to \(String(describing: downloader.activeOperation(with: imageURL)))") + runUntil({ firstCallbackCalled && secondCallbackCalled }) - var spinCount = 0 - - runUntil({ - spinCount += 1 - return operation.isFinished - }) - - print("Succeeded after evaluating condition \(spinCount) times.") - - XCTAssertTrue(firstCallbackCalled) - XCTAssertTrue(secondCallbackCalled) + XCTAssertTrue(firstCallbackCalled && secondCallbackCalled) + XCTAssertEqual(ImageLoadingURLProtocolSpy.pastRequests.count, 1) + XCTAssertEqual(ImageLoadingURLProtocolSpy.activeRequests.count, 0) } func testDownloadingImageAgainAfterFirstDownloadCompletes() { var callbackCalled = false - var spinCount = 0 - downloader.download(with: imageURL) { (cachedResponse, error) in + downloader.download(with: imageURL) { _ in callbackCalled = true } - var operation = downloader.activeOperation(with: imageURL)! - runUntil({ - spinCount += 1 - return operation.isFinished - }) - - print("Succeeded after evaluating first condition \(spinCount) times.") + runUntil({ callbackCalled }) XCTAssertTrue(callbackCalled) callbackCalled = false - spinCount = 0 - - downloader.download(with: imageURL) { (cachedResponse, error) in + downloader.download(with: imageURL) { _ in callbackCalled = true } - operation = downloader.activeOperation(with: imageURL)! - runUntil({ - spinCount += 1 - return operation.isFinished - }) - - print("Succeeded after evaluating second condition \(spinCount) times.") + runUntil({ callbackCalled }) XCTAssertTrue(callbackCalled) } func testDownloadImageWithIncorrectUrl() { - var dataReturned: Data? - var errorReturned: Error? + var resultReturned: Result? let imageDownloaded = expectation(description: "Image Downloaded") let incorrectUrl = URL(fileURLWithPath: "/incorrect_url") - downloader.download(with: incorrectUrl) { (cachedResponse, error) in - dataReturned = cachedResponse?.data - errorReturned = error + downloader.download(with: incorrectUrl) { result in + resultReturned = result imageDownloaded.fulfill() } waitForExpectations(timeout: 10, handler: nil) - XCTAssertNil(dataReturned) - XCTAssertNotNil(errorReturned) + XCTAssertNotNil(resultReturned) + XCTAssertThrowsError(try resultReturned!.get()) } func testDownloadWith400StatusCode() { - var dataReturned: Data? - var errorReturned: Error? + var resultReturned: Result? let imageDownloaded = expectation(description: "Image Downloaded") let faultyUrl = URL(string: "https://www.mapbox.com")! ImageLoadingURLProtocolSpy.registerHttpStatusCodeError(404, for: faultyUrl) - downloader.download(with: faultyUrl) { (cachedResponse, error) in - dataReturned = cachedResponse?.data - errorReturned = error + downloader.download(with: faultyUrl) { result in + resultReturned = result imageDownloaded.fulfill() } - let operation = (downloader.activeOperation(with: faultyUrl) as! ImageDownloadOperation) waitForExpectations(timeout: 10, handler: nil) - XCTAssertTrue(operation.isFinished) - XCTAssertFalse(operation.isExecuting) - XCTAssertNil(dataReturned) - XCTAssertNotNil(errorReturned) - guard let downloadError = errorReturned as? DownloadError else { - XCTFail("Incorrect error returned"); return - } - XCTAssertEqual(downloadError, DownloadError.serverError) - } - - func testDownloadWithImmidiateCancel() { - let incorrectUrl = URL(fileURLWithPath: "/incorrect_url") - downloader.download(with: incorrectUrl, completion: nil) - let operation = (downloader.activeOperation(with: incorrectUrl) as! ImageDownloadOperation) - operation.cancel() - - XCTAssertFalse(operation.isReady) - XCTAssertFalse(operation.isExecuting) - XCTAssertTrue(operation.isCancelled) - XCTAssertTrue(operation.isFinished) - } - - func testDownloadWithImmidiateCancelFromAnotherThread() { - let incorrectUrl = URL(fileURLWithPath: "/incorrect_url") - downloader.download(with: incorrectUrl, completion: nil) - let operation = (downloader.activeOperation(with: incorrectUrl) as! ImageDownloadOperation) - DispatchQueue.global().sync { - operation.cancel() + guard case let .success(cachedResponse) = resultReturned, let urlResponse = cachedResponse.response as? HTTPURLResponse else { + XCTFail(); return } - - XCTAssertFalse(operation.isReady) - XCTAssertFalse(operation.isExecuting) - XCTAssertTrue(operation.isCancelled) - XCTAssertTrue(operation.isFinished) + XCTAssertEqual(urlResponse.statusCode, 404) } func testThreadSafetyStressTests() { @@ -221,10 +158,10 @@ class ImageDownloaderTests: TestCase { for imageUrl in imageUrls { concurrentOvercommitQueue.async { - self.downloader.download(with: imageUrl) { cachedResponse, error in + self.downloader.download(with: imageUrl) { result in var image: UIImage? = nil - if let data = cachedResponse?.data { - image = UIImage(data: data, scale: UIScreen.main.scale) + if case let .success(cachedResponse) = result { + image = UIImage(data: cachedResponse.data, scale: UIScreen.main.scale) } addDownloadedImage(image, for: imageUrl) allImagesDownloaded.fulfill() @@ -232,19 +169,11 @@ class ImageDownloaderTests: TestCase { } } - for imageUrl in imageUrls { - concurrentOvercommitQueue.async { - /// `activeOperation(with:)` should be thread safe - _ = self.downloader.activeOperation(with: imageUrl) - } - } - waitForExpectations(timeout: 10, handler: nil) XCTAssertEqual(downloadedImages.values.compactMap({$0}).count, imageUrls.count) } - @available(iOS 13.0, *) func testPerformance() { measure(metrics: [ XCTCPUMetric(), diff --git a/Tests/MapboxNavigationTests/ImageLoadingURLProtocolSpy.swift b/Tests/MapboxNavigationTests/ImageLoadingURLProtocolSpy.swift index a90fa60758..0941094a4f 100644 --- a/Tests/MapboxNavigationTests/ImageLoadingURLProtocolSpy.swift +++ b/Tests/MapboxNavigationTests/ImageLoadingURLProtocolSpy.swift @@ -11,9 +11,9 @@ final class ImageLoadingURLProtocolSpy: URLProtocol { } private static let lock: NSLock = .init() - private static var responseData: [URL: Result] = [:] - private static var activeRequests: [URL: URLRequest] = [:] - private static var pastRequests: [URL: URLRequest] = [:] + private(set) static var responseData: [URL: Result] = [:] + private(set) static var activeRequests: [URL: URLRequest] = [:] + private(set) static var pastRequests: [URL: URLRequest] = [:] private static let imageLoadingSemaphore = DispatchSemaphore(value: 1) private var loadingStopped: Bool = false diff --git a/Tests/MapboxNavigationTests/ImageRepositoryTests.swift b/Tests/MapboxNavigationTests/ImageRepositoryTests.swift index 4238f09b20..4afdd3887b 100644 --- a/Tests/MapboxNavigationTests/ImageRepositoryTests.swift +++ b/Tests/MapboxNavigationTests/ImageRepositoryTests.swift @@ -4,12 +4,11 @@ import TestHelper class ImageRepositoryTests: TestCase { lazy var repository: ImageRepository = { - let repo = ImageRepository.shared let config = URLSessionConfiguration.default config.protocolClasses = [ImageLoadingURLProtocolSpy.self] - repo.sessionConfiguration = config + let downloader = LegacyImageDownloader(configuration: config) - return repo + return ImageRepository(withDownloader: downloader) }() override func setUp() { diff --git a/Tests/MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift b/Tests/MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift index 09995ea26b..dfeb6f3fe3 100644 --- a/Tests/MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift +++ b/Tests/MapboxNavigationTests/InstructionsBannerViewIntegrationTests.swift @@ -31,15 +31,12 @@ class InstructionsBannerViewIntegrationTests: InstructionBannerTest { cacheSprite() reverseDelegate = TextReversingDelegate() silentDelegate = DefaultBehaviorDelegate() - spriteRepository.imageDownloader.setOperationType(ImageDownloadOperationSpy.self) } override func tearDown() { super.tearDown() clearDiskCache() - spriteRepository.imageDownloader.setOperationType(nil) - ImageDownloadOperationSpy.reset() } func testCustomVisualInstructionDelegate() { @@ -290,11 +287,11 @@ class InstructionsBannerViewIntegrationTests: InstructionBannerTest { private func simulateDownloadingShieldForComponent(_ component: VisualInstruction.Component) { guard case let VisualInstruction.Component.image(image: imageRepresentation, alternativeText: _) = component, let imageURL = imageRepresentation.imageURL(scale: VisualInstruction.Component.scale, format: .png) else { return } - let operation: ImageDownloadOperationSpy = ImageDownloadOperationSpy.operationForURL(imageURL)! let data = ShieldImage.i280.image.pngData()! let response = URLResponse(url: imageURL, mimeType: nil, expectedContentLength: data.count, textEncodingName: nil) - operation.fireAllCompletions(CachedURLResponse(response: response, data: data), error: nil) - + (spriteRepository.imageDownloader as! ImageDownloaderSpy).fireCompletion( + for: imageURL, result: .success(CachedURLResponse(response: response, data: data)) + ) XCTAssertNotNil(spriteRepository.getLegacyShield(with: imageRepresentation)) } } diff --git a/Tests/MapboxNavigationTests/InstructionsBannerViewSnapshotTests.swift b/Tests/MapboxNavigationTests/InstructionsBannerViewSnapshotTests.swift index 7cee9a99c9..9fa1d3de3b 100644 --- a/Tests/MapboxNavigationTests/InstructionsBannerViewSnapshotTests.swift +++ b/Tests/MapboxNavigationTests/InstructionsBannerViewSnapshotTests.swift @@ -499,7 +499,8 @@ class InstructionBannerTest: TestCase { override func setUp() { super.setUp() - spriteRepository = SpriteRepository(requestCache: URLCacheSpy(), + spriteRepository = SpriteRepository(imageDownloader: ImageDownloaderSpy(), + requestCache: URLCacheSpy(), derivedCache: BimodalImageCacheSpy()) } diff --git a/Tests/MapboxNavigationTests/NavigationViewControllerTests.swift b/Tests/MapboxNavigationTests/NavigationViewControllerTests.swift index 84faf35d45..138f9ce9ea 100644 --- a/Tests/MapboxNavigationTests/NavigationViewControllerTests.swift +++ b/Tests/MapboxNavigationTests/NavigationViewControllerTests.swift @@ -491,9 +491,6 @@ class NavigationViewControllerTests: TestCase { let dayStyleURI: StyleURI = .navigationDay let spriteRepository = SpriteRepository.shared - let config = URLSessionConfiguration.default - config.timeoutIntervalForRequest = 1.0 - spriteRepository.sessionConfiguration = config let styleLoadedExpectation = XCTestExpectation(description: "Style updated expectation.") spriteRepository.updateStyle(styleURI: nightStyleURI) { _ in