From 10847fd5492b2048f30848a6c404c13ab2e1c6ad Mon Sep 17 00:00:00 2001 From: Jamil Date: Thu, 16 Jan 2025 07:26:03 -0800 Subject: [PATCH] fix(apple): Use Task.detached when starting from MainActor (#7766) When starting a Task, by default it's launched with the same priority as the calling code. In the UI these are run on the `MainActor` with highest priority by default. If the worker thread running the Task closure gets blocked, it will cause the UI to hang. To fix this, we use `Task.detached` which runs the closure without a specific priority, which is lower than the UI thread. Furthermore, `weak self` is used to prevent retain cycles if the parent thread `deinit`s. This was causing an issue primarily when making IPC calls because those will sometimes hang until the XPC service is launched for the first time. --------- Signed-off-by: Jamil --- .../Managers/VPNConfigurationManager.swift | 10 ++--- .../Sources/FirezoneKit/Stores/Store.swift | 41 +++++++++---------- .../FirezoneKit/Views/FirstTimeView.swift | 4 +- .../Sources/FirezoneKit/Views/MenuBar.swift | 20 +++++---- .../FirezoneKit/Views/SettingsView.swift | 31 +++++++------- 5 files changed, 53 insertions(+), 53 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift index cb11847de..831b5c064 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Managers/VPNConfigurationManager.swift @@ -174,7 +174,7 @@ public class VPNConfigurationManager { } } - func loadFromPreferences(vpnStateUpdateHandler: @escaping (NEVPNStatus, Settings?, String?) -> Void) async throws { + func loadFromPreferences(vpnStateUpdateHandler: @escaping @MainActor (NEVPNStatus, Settings?, String?) -> Void) async throws { // loadAllFromPreferences() returns list of VPN configurations created by our main app's bundle ID. // Since our bundle ID can change (by us), find the one that's current and ignore the others. let managers = try await NETunnelProviderManager.loadAllFromPreferences() @@ -206,7 +206,7 @@ public class VPNConfigurationManager { Telemetry.accountSlug = providerConfiguration[VPNConfigurationManagerKeys.accountSlug] // Share what we found with our caller - vpnStateUpdateHandler(status, settings, actorName) + await vpnStateUpdateHandler(status, settings, actorName) // Stop looking for our tunnel break @@ -216,7 +216,7 @@ public class VPNConfigurationManager { // If no tunnel configuration was found, update state to // prompt user to create one. if manager == nil { - vpnStateUpdateHandler(.invalid, nil, nil) + await vpnStateUpdateHandler(.invalid, nil, nil) } // Hook up status updates @@ -475,7 +475,7 @@ public class VPNConfigurationManager { // Subscribe to system notifications about our VPN status changing // and let our handler know about them. - private func subscribeToVPNStatusUpdates(handler: @escaping (NEVPNStatus, Settings?, String?) -> Void) { + private func subscribeToVPNStatusUpdates(handler: @escaping @MainActor (NEVPNStatus, Settings?, String?) -> Void) { Log.log("\(#function)") for task in tunnelObservingTasks { @@ -500,7 +500,7 @@ public class VPNConfigurationManager { resourcesListCache = ResourceList.loading } - handler(session.status, nil, nil) + await handler(session.status, nil, nil) } } ) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift index c7e23e722..d057f1a49 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/Store.swift @@ -70,19 +70,17 @@ public final class Store: ObservableObject { func bindToVPNConfigurationUpdates() async throws { // Load our existing VPN configuration and set an update handler try await self.vpnConfigurationManager.loadFromPreferences( - vpnStateUpdateHandler: { [weak self] status, settings, actorName in + vpnStateUpdateHandler: { @MainActor [weak self] status, settings, actorName in guard let self else { return } - DispatchQueue.main.async { - self.status = status + self.status = status - if let settings { - self.settings = settings - } + if let settings { + self.settings = settings + } - if let actorName { - self.actorName = actorName - } + if let actorName { + self.actorName = actorName } if status == .disconnected { @@ -98,7 +96,9 @@ public final class Store: ObservableObject { /// reason and alert the user if it was due to receiving a 401 from the /// portal. private func maybeShowSignedOutAlert() { - Task { + Task.detached { [weak self] in + guard let self else { return } + do { if let savedValue = try await self.vpnConfigurationManager.consumeStopReason(), let rawValue = Int(savedValue), @@ -106,7 +106,7 @@ public final class Store: ObservableObject { case .authenticationCanceled = reason { #if os(macOS) - self.sessionNotification.showSignedOutAlertmacOS() + await self.sessionNotification.showSignedOutAlertmacOS() #endif } } catch { @@ -190,7 +190,7 @@ public final class Store: ObservableObject { func signIn(authResponse: AuthResponse) async throws { // Save actorName - DispatchQueue.main.async { self.actorName = authResponse.actorName } + await MainActor.run { self.actorName = authResponse.actorName } try await self.vpnConfigurationManager.saveSettings(settings) try await self.vpnConfigurationManager.saveAuthResponse(authResponse) @@ -212,9 +212,8 @@ public final class Store: ObservableObject { self.vpnConfigurationManager.fetchResources(callback: callback) let intervalInSeconds: TimeInterval = 1 let timer = Timer(timeInterval: intervalInSeconds, repeats: true) { [weak self] _ in - Task { @MainActor in - guard let self else { return } - self.vpnConfigurationManager.fetchResources(callback: callback) + Task.detached { + await self?.vpnConfigurationManager.fetchResources(callback: callback) } } RunLoop.main.add(timer, forMode: .common) @@ -226,11 +225,13 @@ public final class Store: ObservableObject { resourcesTimer = nil } - func save(_ newSettings: Settings) async throws { - Task { + func save(_ newSettings: Settings) { + Task.detached { [weak self] in + guard let self else { return } + do { try await self.vpnConfigurationManager.saveSettings(newSettings) - DispatchQueue.main.async { self.settings = newSettings } + await DispatchQueue.main.async { self.settings = newSettings } } catch { Log.error(error) } @@ -241,8 +242,6 @@ public final class Store: ObservableObject { self.vpnConfigurationManager.toggleInternetResource(enabled: enabled) var newSettings = settings newSettings.internetResourceEnabled = self.vpnConfigurationManager.internetResourceEnabled - Task { - try await save(newSettings) - } + save(newSettings) } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/FirstTimeView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/FirstTimeView.swift index d23bd9f17..eca2d78ab 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/FirstTimeView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/FirstTimeView.swift @@ -36,9 +36,7 @@ struct FirstTimeView: View { .buttonStyle(.borderedProminent) .controlSize(.large) Button("Open menu") { - DispatchQueue.main.async { - menuBar.showMenu() - } + menuBar.showMenu() AppViewModel.WindowDefinition.main.window()?.close() } .buttonStyle(.borderedProminent) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index 9b488591c..6db0ab10c 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -265,13 +265,15 @@ public final class MenuBar: NSObject, ObservableObject { } @objc private func signOutButtonTapped() { - Task { - try await model.store.signOut() + Task.detached { [weak self] in + try await self?.model.store.signOut() } } @objc private func grantPermissionMenuItemTapped() { - Task { + Task.detached { [weak self] in + guard let self else { return } + do { // If we get here, it means either system extension got disabled or // our VPN configuration got removed. Since we don't know which, reinstall @@ -314,9 +316,11 @@ public final class MenuBar: NSObject, ObservableObject { } @objc private func quitButtonTapped() { - Task { - model.store.stop() - NSApp.terminate(self) + Task.detached { [weak self] in + guard let self else { return } + + await self.model.store.stop() + await NSApp.terminate(self) } } @@ -357,8 +361,8 @@ public final class MenuBar: NSObject, ObservableObject { guard connectingAnimationTimer == nil else { return } let timer = Timer(timeInterval: 0.25, repeats: true) { [weak self] _ in guard let self = self else { return } - Task { - await self.connectingAnimationShowNextFrame() + Task.detached { [weak self] in + await self?.connectingAnimationShowNextFrame() } } RunLoop.main.add(timer, forMode: .common) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift index d5e79667d..189f6427f 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/SettingsView.swift @@ -47,16 +47,13 @@ public final class SettingsViewModel: ObservableObject { } func saveSettings() { - Task { if [.connected, .connecting, .reasserting].contains(store.status) { - _ = try await store.signOut() + Task.detached { [weak self] in + do { try await self?.store.signOut() } catch { Log.error(error) } + } } - do { - try await store.save(settings) - } catch { - Log.error(error) - } - } + + store.save(settings) } // Calculates the total size of our logs by summing the size of the @@ -493,11 +490,13 @@ public struct SettingsView: View { isProcessing: $isExportingLogs, action: { self.isExportingLogs = true - Task { + Task.detached { let archiveURL = LogExporter.tempFile() try await LogExporter.export(to: archiveURL) - self.logTempZipFileURL = archiveURL - self.isPresentingExportLogShareSheet = true + await MainActor.run { + self.logTempZipFileURL = archiveURL + self.isPresentingExportLogShareSheet = true + } } } ) @@ -606,7 +605,7 @@ public struct SettingsView: View { return } - Task { + Task.detached { do { try await LogExporter.export( to: destinationURL, @@ -619,15 +618,15 @@ public struct SettingsView: View { } catch { Log.error(error) - let alert = NSAlert() - alert.messageText = "Error exporting logs: \(error.localizedDescription)" - alert.alertStyle = .critical + let alert = await NSAlert() await MainActor.run { + alert.messageText = "Error exporting logs: \(error.localizedDescription)" + alert.alertStyle = .critical let _ = alert.runModal() } } - self.isExportingLogs = false + await MainActor.run { self.isExportingLogs = false } } } }