From f3f3edc017ddfc3bd16c0253d7571ee0be7598d9 Mon Sep 17 00:00:00 2001 From: Alex Azarov Date: Tue, 23 Jan 2024 18:20:28 +0100 Subject: [PATCH 1/4] Replace legacy image downloader with new actor-based implementation --- Sources/MapboxNavigation/ImageDownload.swift | 192 ------------------ .../MapboxNavigation/ImageDownloader.swift | 119 ----------- .../ImageDownloader/ImageDownloader.swift | 5 + .../LegacyImageDownloader.swift | 22 ++ .../ModernImageDownloader.swift | 47 +++++ .../MapboxNavigation/ImageRepository.swift | 36 ++-- .../MapboxNavigation/SpriteRepository.swift | 26 +-- .../Helper/ImageDownloaderSpy.swift | 15 ++ .../Helper/ReentrantImageDownloaderSpy.swift | 20 +- .../ImageDownloadOperationSpy.swift | 50 ----- .../ImageDownloaderTests.swift | 137 +++---------- .../ImageLoadingURLProtocolSpy.swift | 6 +- .../ImageRepositoryTests.swift | 5 +- ...structionsBannerViewIntegrationTests.swift | 9 +- .../InstructionsBannerViewSnapshotTests.swift | 3 +- .../NavigationViewControllerTests.swift | 3 - 16 files changed, 163 insertions(+), 532 deletions(-) delete mode 100644 Sources/MapboxNavigation/ImageDownload.swift delete mode 100644 Sources/MapboxNavigation/ImageDownloader.swift create mode 100644 Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift create mode 100644 Sources/MapboxNavigation/ImageDownloader/LegacyImageDownloader.swift create mode 100644 Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.swift create mode 100644 Tests/MapboxNavigationTests/Helper/ImageDownloaderSpy.swift delete mode 100644 Tests/MapboxNavigationTests/ImageDownloadOperationSpy.swift 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..786b155600 --- /dev/null +++ b/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.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/ImageDownloader/ModernImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.swift new file mode 100644 index 0000000000..0e2483eda2 --- /dev/null +++ b/Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.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 + let response = try await downloadTask.value + inflightRequests[url] = nil + + return response + } +} diff --git a/Sources/MapboxNavigation/ImageRepository.swift b/Sources/MapboxNavigation/ImageRepository.swift index c10c89aa3c..c5bafe25dd 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 + let _ = 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 From 6f556203399832b546bc7a65268a41457b2979ca Mon Sep 17 00:00:00 2001 From: Alex Azarov Date: Wed, 24 Jan 2024 15:14:41 +0100 Subject: [PATCH 2/4] Remove failed tasks from inflightRequests array --- .../ImageDownloader/ImageDownloader.swift | 46 +++++++++++++++++- .../ImageDownloaderProtocol.swift | 5 ++ .../ModernImageDownloader.swift | 47 ------------------- 3 files changed, 49 insertions(+), 49 deletions(-) create mode 100644 Sources/MapboxNavigation/ImageDownloader/ImageDownloaderProtocol.swift delete mode 100644 Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.swift diff --git a/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift index 786b155600..9cb1523f07 100644 --- a/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift +++ b/Sources/MapboxNavigation/ImageDownloader/ImageDownloader.swift @@ -1,5 +1,47 @@ import Foundation -protocol ImageDownloaderProtocol { - func download(with url: URL, completion: @escaping (Result) -> Void) +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/ModernImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.swift deleted file mode 100644 index 0e2483eda2..0000000000 --- a/Sources/MapboxNavigation/ImageDownloader/ModernImageDownloader.swift +++ /dev/null @@ -1,47 +0,0 @@ -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 - let response = try await downloadTask.value - inflightRequests[url] = nil - - return response - } -} From df251cc5e984d5cd1519d5130615ee6330493f88 Mon Sep 17 00:00:00 2001 From: Alex Azarov Date: Wed, 24 Jan 2024 15:55:40 +0100 Subject: [PATCH 3/4] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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 From 2e0b7fdf50e4ca4c1fbe4d0646980881c9a6910a Mon Sep 17 00:00:00 2001 From: Alex Azarov Date: Wed, 24 Jan 2024 15:58:12 +0100 Subject: [PATCH 4/4] small refactoring --- Sources/MapboxNavigation/ImageRepository.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/MapboxNavigation/ImageRepository.swift b/Sources/MapboxNavigation/ImageRepository.swift index c5bafe25dd..1a8a16e9f6 100644 --- a/Sources/MapboxNavigation/ImageRepository.swift +++ b/Sources/MapboxNavigation/ImageRepository.swift @@ -43,7 +43,7 @@ class ImageRepository { return } - let _ = imageDownloader.download(with: imageURL, completion: { [weak self] result in + imageDownloader.download(with: imageURL, completion: { [weak self] result in guard let strongSelf = self, case let .success(cachedResponse) = result, let image = UIImage(data: cachedResponse.data, scale: UIScreen.main.scale) else {