some refactoring and additional logging

mapbox · Jan 22, 2024 · 7caca3a · 7caca3a
1 parent a432412
commit 7caca3a
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 12 deletions.
diff --git a/Sources/MapboxNavigation/ImageDownload.swift b/Sources/MapboxNavigation/ImageDownload.swift
@@ -105,6 +105,7 @@ final class ImageDownloadOperation: Operation, ImageDownload {
 
     override func start() {
         withLock {
+            print("[imagedownloader] operation started for url: \(self.request.url!.absoluteString)")
             guard !isCancelled && state != .finished else {
                 state = .finished;
                 return
@@ -172,6 +173,7 @@ final class ImageDownloadOperation: Operation, ImageDownload {
             }
         }
 
+        assert(completions.count > 0, "ImageDownloadOperation should have at least one completion")
         // 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)

diff --git a/Sources/MapboxNavigation/ImageDownloader.swift b/Sources/MapboxNavigation/ImageDownloader.swift
@@ -6,7 +6,7 @@ typealias CachedResponseCompletionHandler = (CachedURLResponse?, Error?) -> Void
 typealias ImageDownloadCompletionHandler = (DownloadError?) -> Void
 
 protocol ReentrantImageDownloader {
-    func download(with url: URL, completion: CachedResponseCompletionHandler?) -> Void
+    func download(with url: URL, completion: @escaping CachedResponseCompletionHandler) -> Void
     func activeOperation(with url: URL) -> ImageDownload?
     func setOperationType(_ operationType: ImageDownload.Type?)
 }
@@ -43,22 +43,21 @@ class ImageDownloader: NSObject, ReentrantImageDownloader, URLSessionDataDelegat
         urlSession.invalidateAndCancel()
     }
 
-    func download(with url: URL, completion: CachedResponseCompletionHandler?) {
+    func download(with url: URL, completion: @escaping CachedResponseCompletionHandler) {
+        print("$$$ asked to download an image with url: \(url.absoluteString)")
         accessQueue.sync {
             let request = URLRequest(url)
-            var operation: ImageDownload
             if let activeOperation = self.unsafeActiveOperation(with: url) {
-                operation = activeOperation
+                activeOperation.addCompletion(completion)
             } else {
-                operation = self.operationType.init(request: request, in: self.urlSession)
+                let operation = self.operationType.init(request: request, in: self.urlSession)
+                operation.addCompletion(completion)
                 self.operations[url] = operation
                 if let operation = operation as? Operation {
+                    print("[imagedownloader] added operation for url: \(url.absoluteString)")
                     self.downloadQueue.addOperation(operation)
                 }
             }
-            if let completion = completion {
-                operation.addCompletion(completion)
-            }
         }
     }
 

diff --git a/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift b/Tests/MapboxNavigationTests/Helper/ReentrantImageDownloaderSpy.swift
@@ -8,10 +8,10 @@ final class ReentrantImageDownloaderSpy: ReentrantImageDownloader {
     var returnedDownloadResults = [URL: Data]()
     var returnedOperation: ImageDownload?
 
-    func download(with url: URL, completion: CachedResponseCompletionHandler?) -> Void {
+    func download(with url: URL, completion: @escaping CachedResponseCompletionHandler) -> Void {
         passedDownloadUrl = url
         let response = cachedResponse(with: returnedDownloadResults[url], url: url)
-        completion?(response, response == nil ? DirectionsError.noData : nil)
+        completion(response, response == nil ? DirectionsError.noData : nil)
     }
 
     func activeOperation(with url: URL) -> ImageDownload? {

diff --git a/Tests/MapboxNavigationTests/ImageDownloaderTests.swift b/Tests/MapboxNavigationTests/ImageDownloaderTests.swift
@@ -169,7 +169,7 @@ class ImageDownloaderTests: TestCase {
 
     func testDownloadWithImmidiateCancel() {
         let incorrectUrl = URL(fileURLWithPath: "/incorrect_url")
-        downloader.download(with: incorrectUrl, completion: nil)
+        downloader.download(with: incorrectUrl, completion: {_,_ in })
         let operation = (downloader.activeOperation(with: incorrectUrl) as! ImageDownloadOperation)
         operation.cancel()
 
@@ -181,7 +181,7 @@ class ImageDownloaderTests: TestCase {
 
     func testDownloadWithImmidiateCancelFromAnotherThread() {
         let incorrectUrl = URL(fileURLWithPath: "/incorrect_url")
-        downloader.download(with: incorrectUrl, completion: nil)
+        downloader.download(with: incorrectUrl, completion: {_,_ in })
         let operation = (downloader.activeOperation(with: incorrectUrl) as! ImageDownloadOperation)
         DispatchQueue.global().sync {
             operation.cancel()