From a9dfe009b7be399659c8b433d43c72794e60437d Mon Sep 17 00:00:00 2001 From: Jamil Date: Mon, 18 Mar 2024 12:24:59 -0700 Subject: [PATCH] refactor(apple): Remove Client-side retry logic in favor of letting the error take effect (#4196) Currently we retry starting the tunnel based on why it disconnected. This has happened for example if the stored token is invalid and we don't clear it, causing a connect -> disconnect loop that causes the UI to glitch every second. Instead, we just let it fail, and log the error, or maybe even crash. This PR makes that more noisy so (1) we know about it, and (2) the app enters a valid state (signed out) more quickly. Fixes #4138 --- .../Helpers/TunnelShutdownEvent.swift | 11 +----- .../FirezoneKit/Stores/AuthStore.swift | 35 ------------------- .../FirezoneNetworkExtension/Adapter.swift | 8 ++--- 3 files changed, 3 insertions(+), 51 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/TunnelShutdownEvent.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/TunnelShutdownEvent.swift index 57a5b44d6..5e92b0948 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/TunnelShutdownEvent.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/TunnelShutdownEvent.swift @@ -20,8 +20,6 @@ public struct TunnelShutdownEvent: Codable, CustomStringConvertible { case connlibDisconnected case badTunnelConfiguration case tokenNotFound - case networkSettingsApplyFailure - case invalidAdapterState public var description: String { switch self { @@ -30,8 +28,6 @@ public struct TunnelShutdownEvent: Codable, CustomStringConvertible { case .connlibDisconnected: return "connlib disconnected" case .badTunnelConfiguration: return "bad tunnel configuration" case .tokenNotFound: return "token not found" - case .networkSettingsApplyFailure: return "network settings apply failure" - case .invalidAdapterState: return "invalid adapter state" } } @@ -40,13 +36,9 @@ public struct TunnelShutdownEvent: Codable, CustomStringConvertible { case .stopped(let reason): if reason == .userInitiated { return .signoutImmediatelySilently - } else if reason == .userLogout || reason == .userSwitch { - return .doNothing } else { - return .retryThenSignout + return .doNothing } - case .networkSettingsApplyFailure, .invalidAdapterState: - return .retryThenSignout case .connlibConnectFailure, .connlibDisconnected, .badTunnelConfiguration, .tokenNotFound: return .signoutImmediately @@ -58,7 +50,6 @@ public struct TunnelShutdownEvent: Codable, CustomStringConvertible { case doNothing case signoutImmediately case signoutImmediatelySilently - case retryThenSignout } public let reason: TunnelShutdownEvent.Reason diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift index 1d975d9af..9a4a5a09e 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift @@ -83,9 +83,6 @@ public final class AuthStore: ObservableObject { if status == .disconnected { self.handleTunnelDisconnectionEvent() } - if status == .connected { - self.resetReconnectionAttemptsRemaining() - } self.status = status } } @@ -169,8 +166,6 @@ public final class AuthStore: ObservableObject { } catch { logger.error("\(#function): Error signing out: \(error)") } - - resetReconnectionAttemptsRemaining() } public func cancelSignIn() { @@ -220,37 +215,11 @@ public final class AuthStore: ObservableObject { Task { await self.signOut() } - case .retryThenSignout: - self.retryStartTunnel() case .doNothing: break } } else { self.logger.log("\(#function): Tunnel shutdown event not found") - self.retryStartTunnel() - } - } - - func retryStartTunnel() { - // Try to reconnect, but don't try more than 3 times at a time. - // If this gets called the third time, sign out. - let shouldReconnect = (self.reconnectionAttemptsRemaining > 0) - self.reconnectionAttemptsRemaining = self.reconnectionAttemptsRemaining - 1 - if shouldReconnect { - self.logger.log( - "\(#function): Will try every second to reconnect (\(self.reconnectionAttemptsRemaining) attempts after this)" - ) - DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(self.reconnectDelaySecs)) { - self.logger.log("\(#function): Trying to reconnect") - self.startTunnel() - } - } else { - Task { - await self.signOut() - } - #if os(macOS) - SessionNotificationHelper.showSignedOutAlertmacOS(logger: self.logger, authStore: self) - #endif } } @@ -278,10 +247,6 @@ public final class AuthStore: ObservableObject { } } - func resetReconnectionAttemptsRemaining() { - self.reconnectionAttemptsRemaining = Self.maxReconnectionAttemptCount - } - func tunnelAuthStatus(for authBaseURL: URL) async -> TunnelAuthStatus { if let tokenRef = await keychain.searchByAuthBaseURL(authBaseURL) { return .signedIn(authBaseURL: authBaseURL, tokenReference: tokenRef) diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index 74095378c..aafdafbd6 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -129,9 +129,7 @@ class Adapter { self.logger.log("Adapter.start") guard case .stoppedTunnel = self.state else { - packetTunnelProvider?.handleTunnelShutdown( - dueTo: .invalidAdapterState, - errorMessage: "Adapter is in invalid state") + logger.error("\(#function): Invalid Adapter state") completionHandler(.invalidState) return } @@ -433,9 +431,7 @@ extension Adapter: CallbackHandlerDelegate { networkSettings.setMatchDomains([""]) networkSettings.apply(on: packetTunnelProvider, logger: self.logger) { error in if let error = error { - self.packetTunnelProvider?.handleTunnelShutdown( - dueTo: .networkSettingsApplyFailure, - errorMessage: error.localizedDescription) + self.logger.error("\(#function): \(error)") onStarted?(AdapterError.setNetworkSettings(error)) self.state = .stoppedTunnel } else {