From bd4decbb15408292b940317284c88c37f302e6f8 Mon Sep 17 00:00:00 2001 From: jahorton Date: Thu, 23 Jul 2020 12:51:06 +0700 Subject: [PATCH 1/3] feat(ios/engine): download queue concurrency --- .../ResourceDownloadQueue.swift | 109 +++++++++++------- 1 file changed, 70 insertions(+), 39 deletions(-) diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift index 523d05ebd8..3e24dc16bf 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift @@ -231,28 +231,33 @@ private class DownloadQueueFrame { } } -// The other half of ResourceDownloadManager, this class is responsible for executing the downloads -// and handling the results. +/** + * The other half of ResourceDownloadManager, this class is responsible for executing the downloads + * and handling the results. Internally manages its own single-threaded `DispatchQueue`, which + * handles all necessary synchronization. + * + * At present, submissions to this class will be evaluated sequentially by its `DispatchQueue`, though + * this may change at a later time. (The 'sequential' aspect is due to legacy design decisions from + * before concurrency was a consideration.) + */ class ResourceDownloadQueue: HTTPDownloadDelegate { enum QueueState: String { case clear - case busy case noConnection var error: Error? { switch(self) { case .clear: return nil - case .busy: - return NSError(domain: "Keyman", code: 0, userInfo: [NSLocalizedDescriptionKey: "No internet connection"]) case .noConnection: - return NSError(domain: "Keyman", code: 0, userInfo: [NSLocalizedDescriptionKey: "Download queue is busy"]) + return NSError(domain: "Keyman", code: 0, userInfo: [NSLocalizedDescriptionKey: "No internet connection"]) } } } private var queueRoot: DownloadQueueFrame private var queueStack: [DownloadQueueFrame] + private var queueThread: DispatchQueue private var downloader: HTTPDownloader? private var reachability: Reachability? @@ -278,6 +283,10 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { queueRoot = DownloadQueueFrame() queueStack = [queueRoot] // queueRoot will always be the bottom frame of the stack. + + // Creates a single-threaded DispatchQueue, facilitating concurrency at this class's + // critical sections. + queueThread = DispatchQueue(label: "com.keyman.ResourceDownloadQueue") } public func hasConnection() -> Bool { @@ -289,11 +298,6 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { return .noConnection } - // At this stage, we now have everything needed to generate download requests. - guard currentBatch == nil else { // Original behavior - only one download operation is permitted at a time. - return .busy - } - return .clear } @@ -315,6 +319,12 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } } + // ---------- CRITICAL SECTION: START ----------- + // IMPORTANT: These functions should only ever be referenced from `queueThread`. + // as they maintain synchronization for the queue's critical + // tracking fields. + // + // As a result, these functions are intentionally and explicitly `private`. private func finalizeCurrentBatch(withError error: Error? = nil) { let frame = queueStack[queueStack.count - 1] @@ -333,7 +343,7 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { frame.index += 1 } } - + private func executeNext() { let frame = queueStack[queueStack.count - 1] @@ -374,15 +384,6 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } } - internal func step() { - executeNext() - } - - // Exposes useful information for runtime mocking. Used by some automated tests. - internal func topLevelNodes() -> [DownloadNode] { - return queueRoot.nodes - } - private func innerExecute(_ node: DownloadNode) { switch(node) { case .simpleBatch(let batch): @@ -410,16 +411,36 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { innerExecute(batch.tasks[0]) } } - + + // ----------- CRITICAL SECTION: END ------------ + + // The next two functions are used as queueThread's "API". + // Internally, they must perform `sync` / `async` operations when + // writing to the critical tracking fields. public func queue(_ node: DownloadNode) { - queueRoot.nodes.append(node) - - // If the queue was empty before this... - if queueRoot.nodes.count == 1 && autoExecute { - executeNext() + // `sync` is particularly important here for some automated testing checks + // when `autoExecute == `false`. + queueThread.sync { + self.queueRoot.nodes.append(node) + } + + queueThread.async { + // If the queue was empty before this... + if self.queueRoot.nodes.count == 1 && self.autoExecute { + self.executeNext() + } } } - + + internal func step() { + queueThread.async { self.executeNext() } + } + + // Exposes useful information for runtime mocking. Used by some automated tests. + internal func topLevelNodes() -> [DownloadNode] { + return queueRoot.nodes + } + // MARK - helper methods for ResourceDownloadManager // TODO: Eliminate this property coompletely. @@ -445,17 +466,25 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { let batch = queue.userInfo[Key.downloadBatch] as! AnyDownloadBatch let packagePath = batch.tasks[0].file + var err: Error? = nil do { try batch.completeWithPackage(fromKMP: packagePath) - // Completing the queue means having completed a batch. We should only move forward in this class's - // queue at this time, once a batch's task queue is complete. - finalizeCurrentBatch() } catch { - finalizeCurrentBatch(withError: error) + err = error } - if autoExecute { - executeNext() + queueThread.async { + if let error = err { + self.finalizeCurrentBatch(withError: error) + } else { + // Completing the queue means having completed a batch. We should only move forward in this class's + // queue at this time, once a batch's task queue is complete. + self.finalizeCurrentBatch() + } + + if self.autoExecute { + self.executeNext() + } } } @@ -463,12 +492,14 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { if case .simpleBatch(let batch) = self.currentBatch { try? batch.completeWithCancellation() } - - // In case we're part of a 'composite' operation, we should still keep the queue moving. - finalizeCurrentBatch() - if autoExecute { - executeNext() + queueThread.async { + // In case we're part of a 'composite' operation, we should still keep the queue moving. + self.finalizeCurrentBatch() + + if self.autoExecute { + self.executeNext() + } } } From 485d261b83f2e15bdd4c04e26eae72702d13b944 Mon Sep 17 00:00:00 2001 From: jahorton Date: Thu, 23 Jul 2020 13:15:15 +0700 Subject: [PATCH 2/3] docs(ios/engine): for critical section fields --- .../Classes/Resource Management/ResourceDownloadQueue.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift index 3e24dc16bf..c76b12996f 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadQueue.swift @@ -255,10 +255,14 @@ class ResourceDownloadQueue: HTTPDownloadDelegate { } } + // ---- CRITICAL SECTION FIELDS ---- + // Any writes to these must be performed on `queueThread`. + // Async reads are permitted. private var queueRoot: DownloadQueueFrame private var queueStack: [DownloadQueueFrame] + // ------------- END --------------- + private var queueThread: DispatchQueue - private var downloader: HTTPDownloader? private var reachability: Reachability? From 88f9bde5eeae835f1d228bdb5c8dae5c8d271dfa Mon Sep 17 00:00:00 2001 From: jahorton Date: Thu, 23 Jul 2020 13:51:31 +0700 Subject: [PATCH 3/3] fix(ios/engine): UI-related notifications -> DispatchQueue.main --- .../ResourceDownloadManager.swift | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift index 058db32a3c..54dcb9f9d8 100644 --- a/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift +++ b/ios/engine/KMEI/KeymanEngine/Classes/Resource Management/ResourceDownloadManager.swift @@ -615,20 +615,26 @@ public class ResourceDownloadManager { // MARK - Notifications internal func resourceDownloadStarted(withKey packageKey: KeymanPackage.Key) { - NotificationCenter.default.post(name: Notifications.packageDownloadStarted, - object: self, - value: packageKey) + DispatchQueue.main.async { + NotificationCenter.default.post(name: Notifications.packageDownloadStarted, + object: self, + value: packageKey) + } } internal func resourceDownloadCompleted(with package: KeymanPackage) { - NotificationCenter.default.post(name: Notifications.packageDownloadCompleted, - object: self, - value: package) + DispatchQueue.main.async { + NotificationCenter.default.post(name: Notifications.packageDownloadCompleted, + object: self, + value: package) + } } internal func resourceDownloadFailed(withKey packageKey: KeymanPackage.Key, with error: Error) { - NotificationCenter.default.post(name: Notifications.packageDownloadFailed, - object: self, - value: PackageDownloadFailedNotification(packageKey: packageKey, error: error)) + DispatchQueue.main.async { + NotificationCenter.default.post(name: Notifications.packageDownloadFailed, + object: self, + value: PackageDownloadFailedNotification(packageKey: packageKey, error: error)) + } } }