From ad13fb66e5afc2cf27496a56f5b412f83ea507ad Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 26 Apr 2024 15:10:53 +0200 Subject: [PATCH] macOS: Tentative fix for VPN stop issues (#794) Task/Issue URL: https://app.asana.com/0/0/1207063924984126/f iOS PR: Not needed macOS PR: https://github.com/duckduckgo/macos-browser/pull/2689 What kind of version bump will this require?: Patch ## Description: Tentative fix for an issue where the tunnel provider is taking a long time to stop. --- .../Common/Concurrency/TaskExtension.swift | 7 +-- .../NetworkProtectionConnectionTester.swift | 61 +++++-------------- .../NetworkProtectionEntitlementMonitor.swift | 1 + .../NetworkProtectionLatencyMonitor.swift | 1 + ...etworkProtectionTunnelFailureMonitor.swift | 3 + .../PacketTunnelProvider.swift | 2 +- 6 files changed, 23 insertions(+), 52 deletions(-) diff --git a/Sources/Common/Concurrency/TaskExtension.swift b/Sources/Common/Concurrency/TaskExtension.swift index 6a72e6203..d4fcca951 100644 --- a/Sources/Common/Concurrency/TaskExtension.swift +++ b/Sources/Common/Concurrency/TaskExtension.swift @@ -20,13 +20,12 @@ import Foundation public extension Task where Success == Never, Failure == Error { - static func periodic(priority: TaskPriority? = nil, - delay: TimeInterval? = nil, + static func periodic(delay: TimeInterval? = nil, interval: TimeInterval, operation: @escaping @Sendable () async -> Void, cancellationHandler: (@Sendable () async -> Void)? = nil) -> Task { - Task.detached(priority: priority) { + Task { do { if let delay { try await Task.sleep(interval: delay) @@ -42,9 +41,7 @@ public extension Task where Success == Never, Failure == Error { throw error } } - } - } public extension Task where Success == Never, Failure == Never { diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift index 880ecca2b..be98ebf95 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift @@ -51,7 +51,7 @@ final class NetworkProtectionConnectionTester { static let monitorQueue = DispatchQueue(label: "com.duckduckgo.NetworkProtectionConnectionTester.monitorQueue") static let endpoint = NWEndpoint.hostPort(host: .name("www.duckduckgo.com", nil), port: .https) - private var timer: DispatchSourceTimer? + private var task: Task? // MARK: - Dispatch Queue @@ -100,7 +100,7 @@ final class NetworkProtectionConnectionTester { deinit { os_log("[-] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) - timer?.cancel() + task?.cancel() } // MARK: - Testing @@ -131,9 +131,9 @@ final class NetworkProtectionConnectionTester { } } - func stop() async { + func stop() { os_log("🔴 Stopping connection tester", log: log) - await stopScheduledTimer() + stopScheduledTimer() isRunning = false } @@ -168,7 +168,7 @@ final class NetworkProtectionConnectionTester { // MARK: - Timer scheduling private func scheduleTimer(testImmediately: Bool) async throws { - await stopScheduledTimer() + stopScheduledTimer() if testImmediately { do { @@ -179,50 +179,19 @@ final class NetworkProtectionConnectionTester { } } - let timer = DispatchSource.makeTimerSource(queue: timerQueue) - self.timer = timer - - timer.schedule(deadline: .now() + self.intervalBetweenTests, repeating: self.intervalBetweenTests) - timer.setEventHandler { [weak self] in - Task { [self] in - // During regular connection tests we don't care about the error thrown - // by this method, as it'll be handled through the result handler callback. - // The error we're ignoring here is only used when this class is initialized - // with an immediate test, to know whether the connection is up while the user - // still sees "Connecting..." - try? await self?.testConnection() - } + task = Task.periodic(interval: intervalBetweenTests) { [weak self] in + // During regular connection tests we don't care about the error thrown + // by this method, as it'll be handled through the result handler callback. + // The error we're ignoring here is only used when this class is initialized + // with an immediate test, to know whether the connection is up while the user + // still sees "Connecting..." + try? await self?.testConnection() } - - // Enable back if needed. The only reason this commented code is left in is because it has - // documentation purposes, and while the timer should not be released here, it's ok to enable - // back the cancellation handler if it's needed for other purposes. - // - // timer.setCancelHandler { [weak self] in - // Do not re-enable this. - // Releasing the timer here is causing a crash. I'm leaving this here for documentation - // purposes, so that we're not tempted to add this back. - // - // self?.timer = nil - // } - - timer.resume() } - private func stopScheduledTimer() async { - cancelTimerImmediately() - } - - private func cancelTimerImmediately() { - guard let timer = timer else { - return - } - - if !timer.isCancelled { - timer.cancel() - } - - self.timer = nil + private func stopScheduledTimer() { + task?.cancel() + task = nil } // MARK: - Testing the connection diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionEntitlementMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionEntitlementMonitor.swift index 7d9bd6967..9b7326004 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionEntitlementMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionEntitlementMonitor.swift @@ -76,6 +76,7 @@ public actor NetworkProtectionEntitlementMonitor { public func stop() { os_log("⚫️ Stopping entitlement monitor", log: .networkProtectionEntitlementMonitorLog) + task?.cancel() // Just making extra sure in case it's detached task = nil } } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 309cb19c0..c080cf9ff 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -129,6 +129,7 @@ public actor NetworkProtectionLatencyMonitor { os_log("⚫️ Stopping latency monitor", log: .networkProtectionLatencyMonitorLog) latencyCancellable = nil + task?.cancel() // Just making extra sure in case it's detached task = nil } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index c7c1bee51..037aa7752 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -95,7 +95,10 @@ public actor NetworkProtectionTunnelFailureMonitor { func stop() { os_log("⚫️ Stopping tunnel failure monitor", log: .networkProtectionTunnelFailureMonitorLog) + networkMonitor.cancel() networkMonitor.pathUpdateHandler = nil + + task?.cancel() // Just making extra sure in case it's detached task = nil } diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 6de46fc05..7b5b77d03 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -1302,7 +1302,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { @MainActor public func stopMonitors() async { - await self.connectionTester.stop() + self.connectionTester.stop() await self.tunnelFailureMonitor.stop() await self.latencyMonitor.stop() await self.entitlementMonitor.stop()