From e8384ea5b0f86c46b9c317801a2b48e312ea312b Mon Sep 17 00:00:00 2001 From: Jamil Date: Mon, 10 Feb 2025 14:38:05 -0800 Subject: [PATCH] refactor(apple): Make IPC calls async, bubbling errors (#8062) `fetchResources` is an IPC call, and we can use `withCheckedThrowingContinuation` like the others to yield while we wait for the provider to respond. The particular sentry issue related to this isn't because we are necessarily blocking the task thread, rather, I suspect it's when applying the fetched Resources to the UI that we're slow. There isn't much we can do about this, but this PR will only help. Because we're using a timer that fires off a closure to do this, we still use a `callback` inside the timer to actually set the Resources on the main `Store`, which updates the UI. Unfortunately refactoring these IPC calls lead to somewhat of a ball of yarn, so the best way to summarize the spirit of this PR is: - Ensure IPC calls use `withCheckedThrowingContinuation` where possible - Thusly, marking these functions `async throws` - Bubble these errors up the view where we can ultimately decide what to do with them - Keep VPN state management and conditional logic based on `NEVPNStatus` in the vpnConfigurationManager --- .../Managers/VPNConfigurationManager.swift | 126 +++++++++--------- .../Sources/FirezoneKit/Stores/Store.swift | 49 +++---- .../Sources/FirezoneKit/Views/MenuBar.swift | 23 ++-- .../FirezoneKit/Views/ResourceView.swift | 8 +- .../FirezoneKit/Views/SessionView.swift | 2 +- .../FirezoneKit/Views/SettingsView.swift | 14 +- .../FirezoneKit/Views/iOSNavigationView.swift | 4 +- 7 files changed, 111 insertions(+), 115 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift index ae25cc80b..6dfaf99b2 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift @@ -18,8 +18,9 @@ enum VPNConfigurationManagerError: Error { case managerNotInitialized case cannotLoad case decodeIPCDataFailed - case invalidStatusChange + case invalidNotification case noIPCData + case invalidStatus(NEVPNStatus) var localizedDescription: String { switch self { @@ -27,12 +28,14 @@ enum VPNConfigurationManagerError: Error { return "Manager doesn't seem initialized." case .decodeIPCDataFailed: return "Decoding IPC data failed." - case .invalidStatusChange: + case .invalidNotification: return "NEVPNStatusDidChange notification doesn't seem to be valid." case .cannotLoad: return "Could not load VPN configurations!" case .noIPCData: return "No IPC data returned from the XPC connection!" + case .invalidStatus(let status): + return "The IPC operation couldn't complete because the VPN status is \(status)." } } } @@ -292,72 +295,71 @@ public class VPNConfigurationManager { options.merge(["id": id as NSObject]) { _, new in new } } - try session()?.startTunnel(options: options) + try session().startTunnel(options: options) } - func stop(clearToken: Bool = false) { - if clearToken { - do { - try session()?.sendProviderMessage(encoder.encode(TunnelMessage.signOut)) { _ in - self.session()?.stopTunnel() - } - } catch { - Log.error(error) - } - } else { - session()?.stopTunnel() - } + func signOut() throws { + try session([.connected, .connecting, .reasserting]).stopTunnel() + try session().sendProviderMessage(encoder.encode(TunnelMessage.signOut)) } - func updateInternetResourceState() { - guard session()?.status == .connected else { return } - - try? session()?.sendProviderMessage(encoder.encode(TunnelMessage.internetResourceEnabled(internetResourceEnabled))) + func stop() throws { + try session([.connected, .connecting, .reasserting]).stopTunnel() } - func toggleInternetResource(enabled: Bool) { + func updateInternetResourceState() throws { + try session([.connected]).sendProviderMessage( + encoder.encode(TunnelMessage.internetResourceEnabled(internetResourceEnabled))) + } + + func toggleInternetResource(enabled: Bool) throws { internetResourceEnabled = enabled - updateInternetResourceState() + try updateInternetResourceState() } - func fetchResources(callback: @escaping @MainActor (ResourceList) -> Void) { - guard session()?.status == .connected else { return } + func fetchResources() async throws -> ResourceList { + return try await withCheckedThrowingContinuation { continuation in + do { + // Request list of resources from the provider. We send the hash of the resource list we already have. + // If it differs, we'll get the full list in the callback. If not, we'll get nil. + try session([.connected]).sendProviderMessage( + encoder.encode(TunnelMessage.getResourceList(resourceListHash))) { data in - do { - try session()?.sendProviderMessage(encoder.encode(TunnelMessage.getResourceList(resourceListHash))) { data in - if let data = data { + guard let data = data + else { + // No data returned; Resources haven't changed + continuation.resume(returning: self.resourcesListCache) + + return + } + + // Save hash to compare against self.resourceListHash = Data(SHA256.hash(data: data)) + let decoder = JSONDecoder() decoder.keyDecodingStrategy = .convertFromSnakeCase - guard let decoded = try? decoder.decode([Resource].self, from: data) - else { - fatalError("Should be able to decode ResourceList") + do { + let decoded = try decoder.decode([Resource].self, from: data) + self.resourcesListCache = ResourceList.loaded(decoded) + + continuation.resume(returning: self.resourcesListCache) + } catch { + continuation.resume(throwing: error) } - - self.resourcesListCache = ResourceList.loaded(decoded) } - - Task { await MainActor.run { callback(self.resourcesListCache) } } + } catch { + continuation.resume(throwing: error) } - } catch { - Log.error(error) } } func clearLogs() async throws { return try await withCheckedThrowingContinuation { continuation in - guard let session = session() - else { - continuation.resume(throwing: VPNConfigurationManagerError.managerNotInitialized) - - return - } - do { - try session.sendProviderMessage( - encoder.encode(TunnelMessage.clearLogs) - ) { _ in continuation.resume() } + try session().sendProviderMessage(encoder.encode(TunnelMessage.clearLogs)) { _ in + continuation.resume() + } } catch { continuation.resume(throwing: error) } @@ -366,15 +368,9 @@ public class VPNConfigurationManager { func getLogFolderSize() async throws -> Int64 { return try await withCheckedThrowingContinuation { continuation in - guard let session = session() - else { - continuation.resume(throwing: VPNConfigurationManagerError.managerNotInitialized) - - return - } do { - try session.sendProviderMessage( + try session().sendProviderMessage( encoder.encode(TunnelMessage.getLogFolderSize) ) { data in @@ -406,7 +402,7 @@ public class VPNConfigurationManager { func loop() { do { - try session()?.sendProviderMessage( + try session().sendProviderMessage( encoder.encode(TunnelMessage.exportLogs) ) { data in guard let data = data @@ -443,15 +439,8 @@ public class VPNConfigurationManager { func consumeStopReason() async throws -> NEProviderStopReason? { return try await withCheckedThrowingContinuation { continuation in - guard let session = session() - else { - continuation.resume(throwing: VPNConfigurationManagerError.managerNotInitialized) - - return - } - do { - try session.sendProviderMessage( + try session().sendProviderMessage( encoder.encode(TunnelMessage.consumeStopReason) ) { data in @@ -472,8 +461,17 @@ public class VPNConfigurationManager { } } - private func session() -> NETunnelProviderSession? { - return manager?.connection as? NETunnelProviderSession + private func session(_ requiredStatuses: Set = []) throws -> NETunnelProviderSession { + guard let session = manager?.connection as? NETunnelProviderSession + else { + throw VPNConfigurationManagerError.managerNotInitialized + } + + if requiredStatuses.isEmpty || requiredStatuses.contains(session.status) { + return session + } + + throw VPNConfigurationManagerError.invalidStatus(session.status) } // Subscribe to system notifications about our VPN status changing @@ -495,7 +493,7 @@ public class VPNConfigurationManager { ) { guard let session = notification.object as? NETunnelProviderSession else { - Log.error(VPNConfigurationManagerError.invalidStatusChange) + Log.error(VPNConfigurationManagerError.invalidNotification) return } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift index 0a79c04b2..7a3282677 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift @@ -126,25 +126,16 @@ public final class Store: ObservableObject { } private func start(token: String? = nil) throws { - guard status == .disconnected - else { - Log.log("\(#function): Already connected") - return - } - try self.vpnConfigurationManager.start(token: token) } - func stop(clearToken: Bool = false) { - guard [.connected, .connecting, .reasserting].contains(status) - else { return } - - self.vpnConfigurationManager.stop(clearToken: clearToken) + func stop() throws { + try self.vpnConfigurationManager.stop() } func signIn(authResponse: AuthResponse) async throws { // Save actorName - await MainActor.run { self.actorName = authResponse.actorName } + self.actorName = authResponse.actorName try await self.vpnConfigurationManager.saveSettings(settings) try await self.vpnConfigurationManager.saveAuthResponse(authResponse) @@ -153,9 +144,8 @@ public final class Store: ObservableObject { try self.vpnConfigurationManager.start(token: authResponse.token) } - func signOut() async throws { - // Stop tunnel and clear token - stop(clearToken: true) + func signOut() throws { + try self.vpnConfigurationManager.signOut() } // Network Extensions don't have a 2-way binding up to the GUI process, @@ -171,8 +161,13 @@ public final class Store: ObservableObject { // Define the Timer's closure let updateResources: @Sendable (Timer) -> Void = { _ in - Task.detached { [weak self] in - await self?.vpnConfigurationManager.fetchResources(callback: callback) + Task { + do { + let resources = try await self.vpnConfigurationManager.fetchResources() + await callback(resources) + } catch { + Log.error(error) + } } } @@ -193,23 +188,15 @@ public final class Store: ObservableObject { resourcesTimer = nil } - func save(_ newSettings: Settings) { - Task.detached { [weak self] in - guard let self else { return } - - do { - try await self.vpnConfigurationManager.saveSettings(newSettings) - await MainActor.run { self.settings = newSettings } - } catch { - Log.error(error) - } - } + func save(_ newSettings: Settings) async throws { + try await self.vpnConfigurationManager.saveSettings(newSettings) + self.settings = newSettings } - func toggleInternetResource(enabled: Bool) { - self.vpnConfigurationManager.toggleInternetResource(enabled: enabled) + func toggleInternetResource(enabled: Bool) async throws { + try self.vpnConfigurationManager.toggleInternetResource(enabled: enabled) var newSettings = settings newSettings.internetResourceEnabled = self.vpnConfigurationManager.internetResourceEnabled - save(newSettings) + try await save(newSettings) } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index 0e2a581f2..2755733de 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -276,9 +276,7 @@ public final class MenuBar: NSObject, ObservableObject { } @objc private func signOutButtonTapped() { - Task.detached { [weak self] in - try await self?.model.store.signOut() - } + do { try self.model.store.signOut() } catch { Log.error(error) } } @objc private func grantPermissionMenuItemTapped() { @@ -340,11 +338,9 @@ public final class MenuBar: NSObject, ObservableObject { } @objc private func quitButtonTapped() { - Task.detached { [weak self] in - guard let self else { return } - - await self.model.store.stop() - await NSApp.terminate(self) + Task { + do { try self.model.store.stop() } catch { Log.error(error) } + NSApp.terminate(self) } } @@ -809,8 +805,15 @@ public final class MenuBar: NSObject, ObservableObject { } @objc private func internetResourceToggle(_ sender: NSMenuItem) { - self.model.store.toggleInternetResource(enabled: !model.store.internetResourceEnabled()) - sender.title = internetResourceToggleTitle() + Task { + do { + try await self.model.store.toggleInternetResource(enabled: !model.store.internetResourceEnabled()) + } catch { + Log.error(error) + } + + sender.title = internetResourceToggleTitle() + } } @objc private func resourceURLTapped(_ sender: AnyObject?) { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/ResourceView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/ResourceView.swift index ebda3d4c7..ca7802bb8 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/ResourceView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/ResourceView.swift @@ -245,7 +245,13 @@ struct ToggleInternetResourceButton: View { var body: some View { Button( action: { - model.store.toggleInternetResource(enabled: !model.isInternetResourceEnabled()) + Task { + do { + try await model.store.toggleInternetResource(enabled: !model.isInternetResourceEnabled()) + } catch { + Log.error(error) + } + } }, label: { HStack { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift index 564fd41f2..3f27f34ff 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SessionView.swift @@ -49,7 +49,7 @@ public final class SessionViewModel: ObservableObject { if status == .connected { store.beginUpdatingResources { resources in - Task { await MainActor.run { self.resources = resources } } + self.resources = resources } } else { store.endUpdatingResources() diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift index 760ab24ae..532334f75 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift @@ -50,13 +50,17 @@ public final class SettingsViewModel: ObservableObject { } func saveSettings() { - if [.connected, .connecting, .reasserting].contains(store.status) { - Task.detached { [weak self] in - do { try await self?.store.signOut() } catch { Log.error(error) } + Task { + do { + if [.connected, .connecting, .reasserting].contains(store.status) { + try self.store.signOut() } - } - store.save(settings) + try await store.save(settings) + } catch { + Log.error(error) + } + } } // Calculates the total size of our logs by summing the size of the diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift index 8e4204d3d..d32dfe6f6 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/iOSNavigationView.swift @@ -118,9 +118,7 @@ struct iOSNavigationView: View { // swiftlint:disable:this type_n } private func signOutButtonTapped() { - Task { - try await model.store.signOut() - } + do { try model.store.signOut() } catch { Log.error(error) } } } #endif