From 1a04cd2463720778682e2d7ddf4235297c48d1e3 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Wed, 20 Dec 2023 17:35:57 -0500 Subject: [PATCH 01/12] Rewrite tunnel failure monitor using Task instead of Timer --- ...etworkProtectionTunnelFailureMonitor.swift | 116 ++++-------------- .../PacketTunnelProvider.swift | 19 +-- 2 files changed, 37 insertions(+), 98 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index c801cf2e2..04f453365 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -22,7 +22,7 @@ import NetworkExtension import Common import Combine -final public class NetworkProtectionTunnelFailureMonitor { +public actor NetworkProtectionTunnelFailureMonitor { public enum Result { case failureDetected case failureRecovered @@ -39,52 +39,35 @@ final public class NetworkProtectionTunnelFailureMonitor { private static let monitoringInterval: TimeInterval = .seconds(10) - public var publisher: AnyPublisher { + public nonisolated var publisher: AnyPublisher { failureSubject.eraseToAnyPublisher() } private let failureSubject = PassthroughSubject() - private actor TimerRunCoordinator { - private(set) var isRunning = false - - func start() { - isRunning = true - } - - func stop() { - isRunning = false + @MainActor + private var task: Task? { + willSet { + task?.cancel() } } - private var timer: DispatchSourceTimer? - private let timerRunCoordinator = TimerRunCoordinator() - private let timerQueue: DispatchQueue + @MainActor + var isStarted: Bool { + task?.isCancelled == false + } private let tunnelProvider: PacketTunnelProvider private let networkMonitor = NWPathMonitor() private let log: OSLog - private let lock = NSLock() - - private var _failureReported = false - private(set) var failureReported: Bool { - get { - lock.lock(); defer { lock.unlock() } - return _failureReported - } - set { - lock.lock() - self._failureReported = newValue - lock.unlock() - } - } + @MainActor + private var failureReported = false // MARK: - Init & deinit - init(tunnelProvider: PacketTunnelProvider, timerQueue: DispatchQueue, log: OSLog) { + init(tunnelProvider: PacketTunnelProvider, log: OSLog) { self.tunnelProvider = tunnelProvider - self.timerQueue = timerQueue self.log = log os_log("[+] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) @@ -92,84 +75,34 @@ final public class NetworkProtectionTunnelFailureMonitor { deinit { os_log("[-] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) - - cancelTimerImmediately() } // MARK: - Start/Stop monitoring - func start() async throws { - guard await !timerRunCoordinator.isRunning else { - os_log("Will not start the tunnel failure monitor as it's already running", log: log) - return - } - + @MainActor + func start() { os_log("⚫️ Starting tunnel failure monitor", log: log) - do { - networkMonitor.start(queue: .global()) + failureReported = false + networkMonitor.start(queue: .global()) - failureReported = false - try await scheduleTimer() - } catch { - os_log("⚫️ Stopping tunnel failure monitor prematurely", log: log) - throw error + task = Task.periodic(interval: Self.monitoringInterval) { [weak self] in + await self?.monitorHandshakes() } } - func stop() async { + @MainActor + func stop() { os_log("⚫️ Stopping tunnel failure monitor", log: log) - await stopScheduledTimer() + task = nil networkMonitor.cancel() } - // MARK: - Timer scheduling - - private func scheduleTimer() async throws { - await stopScheduledTimer() - - await timerRunCoordinator.start() - - let timer = DispatchSource.makeTimerSource(queue: timerQueue) - self.timer = timer - - timer.schedule(deadline: .now() + Self.monitoringInterval, repeating: Self.monitoringInterval) - timer.setEventHandler { [weak self] in - guard let self else { return } - - Task { - try? await self.monitorHandshakes() - } - } - - timer.setCancelHandler { [weak self] in - self?.timer = nil - } - - timer.resume() - } - - private func stopScheduledTimer() async { - await timerRunCoordinator.stop() - - cancelTimerImmediately() - } - - private func cancelTimerImmediately() { - guard let timer else { return } - - if !timer.isCancelled { - timer.cancel() - } - - self.timer = nil - } - // MARK: - Handshake monitor @MainActor - func monitorHandshakes() async throws { + private func monitorHandshakes() async { let mostRecentHandshake = await tunnelProvider.mostRecentHandshake() ?? 0 let difference = Date().timeIntervalSince1970 - mostRecentHandshake @@ -188,7 +121,8 @@ final public class NetworkProtectionTunnelFailureMonitor { } } - var isConnected: Bool { + @MainActor + private var isConnected: Bool { let path = networkMonitor.currentPath let connectionType = NetworkConnectionType(nwPath: path) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 03ac8b0d5..0d28a98ff 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -256,7 +256,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { }() public lazy var tunnelFailureMonitor = NetworkProtectionTunnelFailureMonitor(tunnelProvider: self, - timerQueue: timerQueue, log: .networkProtectionPixel) public lazy var latencyMonitor = NetworkProtectionLatencyMonitor(serverIP: { [weak self] in self?.lastSelectedServerInfo?.ipv4 }, @@ -1033,12 +1032,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { os_log("🔵 Tunnel interface is %{public}@", log: .networkProtection, type: .info, adapter.interfaceName ?? "unknown") - do { - try await tunnelFailureMonitor.start() - } catch { - os_log("⚫️ Tunnel failure monitor error: %{public}@", log: .networkProtectionPixel, type: .error, String(reflecting: error)) - throw error - } + await startTunnelFailureMonitor() do { try await latencyMonitor.start() @@ -1066,6 +1060,17 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { await self.latencyMonitor.stop() } + // MARK: - Monitors + + @MainActor + private func startTunnelFailureMonitor() { + if tunnelFailureMonitor.isStarted { + tunnelFailureMonitor.stop() + } + + tunnelFailureMonitor.start() + } + // MARK: - Connection Tester private enum ConnectionTesterError: Error { From d4e2b2cda1221a90cbff161b281d804aec763cb6 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Wed, 20 Dec 2023 22:28:08 -0500 Subject: [PATCH 02/12] Rewrite latency monitor using Task instead of Timer --- .../NetworkProtectionLatencyMonitor.swift | 124 ++++-------------- .../PacketTunnelProvider.swift | 18 +-- ...NetworkProtectionLatencyMonitorTests.swift | 8 +- 3 files changed, 40 insertions(+), 110 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 3271c447f..17a186065 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -21,7 +21,7 @@ import Network import Common import Combine -final public class NetworkProtectionLatencyMonitor { +public actor NetworkProtectionLatencyMonitor { public enum ConnectionQuality: String { case terrible case poor @@ -59,67 +59,42 @@ final public class NetworkProtectionLatencyMonitor { private static let unknownLatency: TimeInterval = -1 - public var publisher: AnyPublisher { + public nonisolated var publisher: AnyPublisher { subject.eraseToAnyPublisher() } private let subject = PassthroughSubject() private let latencySubject = PassthroughSubject() - private var latencyCancellable: AnyCancellable? - - private actor TimerRunCoordinator { - private(set) var isRunning = false - func start() { - isRunning = true - } + @MainActor + private var latencyCancellable: AnyCancellable? - func stop() { - isRunning = false + @MainActor + private var task: Task? { + willSet { + task?.cancel() } } - private var timer: DispatchSourceTimer? - private let timerRunCoordinator = TimerRunCoordinator() - private let timerQueue: DispatchQueue + @MainActor + var isStarted: Bool { + task?.isCancelled == false + } - private let lock = NSLock() + @MainActor + private var lastLatencyReported: Date = .distantPast - private var _lastLatencyReported: Date = .distantPast - private(set) var lastLatencyReported: Date { - get { - lock.lock(); defer { lock.unlock() } - return _lastLatencyReported - } - set { - lock.lock() - self._lastLatencyReported = newValue - lock.unlock() - } - } + @MainActor + private var ignoreThreshold = false private let serverIP: () -> IPv4Address? private let log: OSLog - private var _ignoreThreshold = false - private(set) var ignoreThreshold: Bool { - get { - lock.lock(); defer { lock.unlock() } - return _ignoreThreshold - } - set { - lock.lock() - self._ignoreThreshold = newValue - lock.unlock() - } - } - // MARK: - Init & deinit - init(serverIP: @escaping () -> IPv4Address?, timerQueue: DispatchQueue, log: OSLog) { + init(serverIP: @escaping () -> IPv4Address?, log: OSLog) { self.serverIP = serverIP - self.timerQueue = timerQueue self.log = log os_log("[+] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) @@ -127,18 +102,12 @@ final public class NetworkProtectionLatencyMonitor { deinit { os_log("[-] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) - - cancelTimerImmediately() } // MARK: - Start/Stop monitoring - public func start() async throws { - guard await !timerRunCoordinator.isRunning else { - os_log("Will not start the latency monitor as it's already running", log: log) - return - } - + @MainActor + public func start() { os_log("⚫️ Starting latency monitor", log: log) latencyCancellable = latencySubject.eraseToAnyPublisher() @@ -164,59 +133,17 @@ final public class NetworkProtectionLatencyMonitor { } } - do { - try await scheduleTimer() - } catch { - os_log("⚫️ Stopping latency monitor prematurely", log: log) - throw error + task = Task.periodic(interval: Self.measurementInterval) { [weak self] in + await self?.measureLatency() } } - public func stop() async { + @MainActor + public func stop() { os_log("⚫️ Stopping latency monitor", log: log) - await stopScheduledTimer() - } - - // MARK: - Timer scheduling - - private func scheduleTimer() async throws { - await stopScheduledTimer() - - await timerRunCoordinator.start() - - let timer = DispatchSource.makeTimerSource(queue: timerQueue) - self.timer = timer - timer.schedule(deadline: .now() + Self.measurementInterval, repeating: Self.measurementInterval) - timer.setEventHandler { [weak self] in - guard let self else { return } - - Task { - await self.measureLatency() - } - } - - timer.setCancelHandler { [weak self] in - self?.timer = nil - } - - timer.resume() - } - - private func stopScheduledTimer() async { - await timerRunCoordinator.stop() - - cancelTimerImmediately() - } - - private func cancelTimerImmediately() { - guard let timer else { return } - - if !timer.isCancelled { - timer.cancel() - } - - self.timer = nil + latencyCancellable = nil + task = nil } // MARK: - Latency monitor @@ -241,6 +168,7 @@ final public class NetworkProtectionLatencyMonitor { } } + @MainActor public func simulateLatency(_ timeInterval: TimeInterval) { ignoreThreshold = true latencySubject.send(timeInterval) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 0d28a98ff..e743c8e98 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -259,7 +259,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { log: .networkProtectionPixel) public lazy var latencyMonitor = NetworkProtectionLatencyMonitor(serverIP: { [weak self] in self?.lastSelectedServerInfo?.ipv4 }, - timerQueue: timerQueue, log: .networkProtectionPixel) private var lastTestFailed = false @@ -1033,13 +1032,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { os_log("🔵 Tunnel interface is %{public}@", log: .networkProtection, type: .info, adapter.interfaceName ?? "unknown") await startTunnelFailureMonitor() - - do { - try await latencyMonitor.start() - } catch { - os_log("⚫️ Latency monitor error: %{public}@", log: .networkProtectionPixel, type: .error, String(reflecting: error)) - throw error - } + await startLatencyMonitor() do { // These cases only make sense in the context of a connection that had trouble @@ -1071,6 +1064,15 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { tunnelFailureMonitor.start() } + @MainActor + private func startLatencyMonitor() { + if latencyMonitor.isStarted { + latencyMonitor.stop() + } + + latencyMonitor.start() + } + // MARK: - Connection Tester private enum ConnectionTesterError: Error { diff --git a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift index 89f9b305e..ca2c2f38c 100644 --- a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift +++ b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift @@ -27,7 +27,7 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { override func setUp() async throws { try await super.setUp() - monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, timerQueue: DispatchQueue.main, log: .networkProtectionPixel) + monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, log: .networkProtectionPixel) try await monitor?.start() } @@ -62,7 +62,7 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { break } } - monitor?.simulateLatency(-1) + await monitor?.simulateLatency(-1) await fulfillment(of: [expectation], timeout: 1) } @@ -80,7 +80,7 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { } private func testConnectionLatency(_ timeInterval: TimeInterval, expecting expectedQuality: NetworkProtectionLatencyMonitor.ConnectionQuality) async { - let monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, timerQueue: DispatchQueue.main, log: .networkProtectionPixel) + let monitor = NetworkProtectionLatencyMonitor(serverIP: { .init("127.0.0.1") }, log: .networkProtectionPixel) var reportedQuality = NetworkProtectionLatencyMonitor.ConnectionQuality.unknown cancellable = monitor.publisher @@ -94,7 +94,7 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { } try? await monitor.start() - monitor.simulateLatency(timeInterval) + await monitor.simulateLatency(timeInterval) await monitor.stop() XCTAssertEqual(expectedQuality, reportedQuality) From 270b4a4e37da810078992380412a3342f427f43e Mon Sep 17 00:00:00 2001 From: Anh Do Date: Wed, 20 Dec 2023 23:44:23 -0500 Subject: [PATCH 03/12] Use callbacks --- .../NetworkProtectionLatencyMonitor.swift | 14 +++---- ...etworkProtectionTunnelFailureMonitor.swift | 15 +++---- .../PacketTunnelProvider.swift | 40 +++++-------------- 3 files changed, 21 insertions(+), 48 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 17a186065..2d696be15 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -59,11 +59,7 @@ public actor NetworkProtectionLatencyMonitor { private static let unknownLatency: TimeInterval = -1 - public nonisolated var publisher: AnyPublisher { - subject.eraseToAnyPublisher() - } - private let subject = PassthroughSubject() - + @MainActor private let latencySubject = PassthroughSubject() @MainActor @@ -107,16 +103,16 @@ public actor NetworkProtectionLatencyMonitor { // MARK: - Start/Stop monitoring @MainActor - public func start() { + public func start(callback: @escaping (Result) -> Void) { os_log("⚫️ Starting latency monitor", log: log) latencyCancellable = latencySubject.eraseToAnyPublisher() - .scan(ExponentialGeometricAverage()) { [weak self] measurements, latency in + .scan(ExponentialGeometricAverage()) { measurements, latency in if latency >= 0 { measurements.addMeasurement(latency) os_log("⚫️ Latency: %{public}f milliseconds", log: .networkProtectionPixel, type: .debug, latency) } else { - self?.subject.send(.error) + callback(.error) } os_log("⚫️ Average: %{public}f milliseconds", log: .networkProtectionPixel, type: .debug, measurements.average) @@ -128,7 +124,7 @@ public actor NetworkProtectionLatencyMonitor { let now = Date() if let self, (now.timeIntervalSince1970 - self.lastLatencyReported.timeIntervalSince1970 >= Self.reportThreshold) || ignoreThreshold { - self.subject.send(.quality(quality)) + callback(.quality(quality)) self.lastLatencyReported = now } } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index 04f453365..ae1f4860d 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -39,11 +39,6 @@ public actor NetworkProtectionTunnelFailureMonitor { private static let monitoringInterval: TimeInterval = .seconds(10) - public nonisolated var publisher: AnyPublisher { - failureSubject.eraseToAnyPublisher() - } - private let failureSubject = PassthroughSubject() - @MainActor private var task: Task? { willSet { @@ -80,14 +75,14 @@ public actor NetworkProtectionTunnelFailureMonitor { // MARK: - Start/Stop monitoring @MainActor - func start() { + func start(callback: @escaping (Result) -> Void) { os_log("⚫️ Starting tunnel failure monitor", log: log) failureReported = false networkMonitor.start(queue: .global()) task = Task.periodic(interval: Self.monitoringInterval) { [weak self] in - await self?.monitorHandshakes() + await self?.monitorHandshakes(callback: callback) } } @@ -102,7 +97,7 @@ public actor NetworkProtectionTunnelFailureMonitor { // MARK: - Handshake monitor @MainActor - private func monitorHandshakes() async { + private func monitorHandshakes(callback: @escaping (Result) -> Void) async { let mostRecentHandshake = await tunnelProvider.mostRecentHandshake() ?? 0 let difference = Date().timeIntervalSince1970 - mostRecentHandshake @@ -112,11 +107,11 @@ public actor NetworkProtectionTunnelFailureMonitor { if failureReported { os_log("⚫️ Tunnel failure already reported", log: .networkProtectionPixel, type: .debug) } else { - failureSubject.send(.failureDetected) + callback(.failureDetected) failureReported = true } } else if difference <= Result.failureRecovered.threshold, failureReported { - failureSubject.send(.failureRecovered) + callback(.failureRecovered) failureReported = false } } diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index e743c8e98..6ec8c6faf 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -299,8 +299,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { observeSettingChanges() observeConnectionStatusChanges() - observeTunnelFailures() - observeConnectionQuality() } deinit { @@ -447,31 +445,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { .store(in: &cancellables) } - private func observeTunnelFailures() { - tunnelFailureMonitor.publisher - .sink { [weak self] result in - self?.providerEvents.fire(.reportTunnelFailure(result: result)) - } - .store(in: &cancellables) - } - - private func observeConnectionQuality() { - latencyMonitor.publisher - .flatMap { [weak self] result in - switch result { - case .error: - self?.providerEvents.fire(.reportLatency(result: .error)) - return Empty().eraseToAnyPublisher() - case .quality(let quality): - return Just(quality).eraseToAnyPublisher() - } - } - .sink { [weak self] quality in - self?.providerEvents.fire(.reportLatency(result: .quality(quality))) - } - .store(in: &cancellables) - } - // MARK: - Tunnel Start open override func startTunnel(options: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void) { @@ -1061,7 +1034,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { tunnelFailureMonitor.stop() } - tunnelFailureMonitor.start() + tunnelFailureMonitor.start { [weak self] result in + self?.providerEvents.fire(.reportTunnelFailure(result: result)) + } } @MainActor @@ -1070,7 +1045,14 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { latencyMonitor.stop() } - latencyMonitor.start() + latencyMonitor.start { [weak self] result in + switch result { + case .error: + self?.providerEvents.fire(.reportLatency(result: .error)) + case .quality(let quality): + self?.providerEvents.fire(.reportLatency(result: .quality(quality))) + } + } } // MARK: - Connection Tester From 23433b8d5894a760a62e7f0510cec630691f6f96 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 00:12:37 -0500 Subject: [PATCH 04/12] Update test cases --- ...NetworkProtectionLatencyMonitorTests.swift | 53 ++++++++----------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift index ca2c2f38c..a7dec0006 100644 --- a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift +++ b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift @@ -27,9 +27,7 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { override func setUp() async throws { try await super.setUp() - monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, log: .networkProtectionPixel) - - try await monitor?.start() + monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, log: .disabled) } override func tearDown() async throws { @@ -38,30 +36,28 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { func testInvalidIP() async { let expectation = XCTestExpectation(description: "Invalid IP reported") - cancellable = monitor?.publisher - .sink { result in - switch result { - case .error: - expectation.fulfill() - case .quality: - break - } + await monitor?.start { result in + switch result { + case .error: + expectation.fulfill() + case .quality: + break } + } await monitor?.measureLatency() await fulfillment(of: [expectation], timeout: 1) } func testPingFailure() async { let expectation = XCTestExpectation(description: "Ping failure reported") - cancellable = monitor?.publisher - .sink { result in - switch result { - case .error: - expectation.fulfill() - case .quality: - break - } + await monitor?.start { result in + switch result { + case .error: + expectation.fulfill() + case .quality: + break } + } await monitor?.simulateLatency(-1) await fulfillment(of: [expectation], timeout: 1) } @@ -80,20 +76,17 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { } private func testConnectionLatency(_ timeInterval: TimeInterval, expecting expectedQuality: NetworkProtectionLatencyMonitor.ConnectionQuality) async { - let monitor = NetworkProtectionLatencyMonitor(serverIP: { .init("127.0.0.1") }, log: .networkProtectionPixel) + let monitor = NetworkProtectionLatencyMonitor(serverIP: { .init("127.0.0.1") }, log: .disabled) var reportedQuality = NetworkProtectionLatencyMonitor.ConnectionQuality.unknown - cancellable = monitor.publisher - .sink { result in - switch result { - case .quality(let quality): - reportedQuality = quality - case .error: - XCTFail("Unexpected result") - } + await monitor.start { result in + switch result { + case .quality(let quality): + reportedQuality = quality + case .error: + XCTFail("Unexpected result") } - - try? await monitor.start() + } await monitor.simulateLatency(timeInterval) await monitor.stop() From 14fee7fde9fb6a6d371be22014c518a15cabd5ed Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 02:05:00 -0500 Subject: [PATCH 05/12] Refactor --- .../NetworkProtectionLatencyMonitor.swift | 22 +++++++++---------- ...etworkProtectionTunnelFailureMonitor.swift | 15 ++++++------- .../PacketTunnelProvider.swift | 16 +++++++++----- ...NetworkProtectionLatencyMonitorTests.swift | 22 ++++--------------- 4 files changed, 32 insertions(+), 43 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 2d696be15..170db7ecb 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -83,28 +83,28 @@ public actor NetworkProtectionLatencyMonitor { @MainActor private var ignoreThreshold = false - private let serverIP: () -> IPv4Address? - - private let log: OSLog + @MainActor + private(set) var serverIP: IPv4Address? // MARK: - Init & deinit - init(serverIP: @escaping () -> IPv4Address?, log: OSLog) { - self.serverIP = serverIP - self.log = log - + init() { os_log("[+] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) } deinit { + task?.cancel() + os_log("[-] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) } // MARK: - Start/Stop monitoring @MainActor - public func start(callback: @escaping (Result) -> Void) { - os_log("⚫️ Starting latency monitor", log: log) + public func start(serverIP: IPv4Address, callback: @escaping (Result) -> Void) { + os_log("⚫️ Starting latency monitor", log: .networkProtectionPixel) + + self.serverIP = serverIP latencyCancellable = latencySubject.eraseToAnyPublisher() .scan(ExponentialGeometricAverage()) { measurements, latency in @@ -136,7 +136,7 @@ public actor NetworkProtectionLatencyMonitor { @MainActor public func stop() { - os_log("⚫️ Stopping latency monitor", log: log) + os_log("⚫️ Stopping latency monitor", log: .networkProtectionPixel) latencyCancellable = nil task = nil @@ -146,7 +146,7 @@ public actor NetworkProtectionLatencyMonitor { @MainActor public func measureLatency() async { - guard let serverIP = serverIP() else { + guard let serverIP else { latencySubject.send(Self.unknownLatency) return } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index ae1f4860d..644488440 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -54,21 +54,22 @@ public actor NetworkProtectionTunnelFailureMonitor { private let tunnelProvider: PacketTunnelProvider private let networkMonitor = NWPathMonitor() - private let log: OSLog - @MainActor private var failureReported = false // MARK: - Init & deinit - init(tunnelProvider: PacketTunnelProvider, log: OSLog) { + init(tunnelProvider: PacketTunnelProvider) { self.tunnelProvider = tunnelProvider - self.log = log + self.networkMonitor.start(queue: .global()) os_log("[+] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) } deinit { + task?.cancel() + networkMonitor.cancel() + os_log("[-] %{public}@", log: .networkProtectionMemoryLog, type: .debug, String(describing: self)) } @@ -76,10 +77,9 @@ public actor NetworkProtectionTunnelFailureMonitor { @MainActor func start(callback: @escaping (Result) -> Void) { - os_log("⚫️ Starting tunnel failure monitor", log: log) + os_log("⚫️ Starting tunnel failure monitor", log: .networkProtectionPixel) failureReported = false - networkMonitor.start(queue: .global()) task = Task.periodic(interval: Self.monitoringInterval) { [weak self] in await self?.monitorHandshakes(callback: callback) @@ -88,10 +88,9 @@ public actor NetworkProtectionTunnelFailureMonitor { @MainActor func stop() { - os_log("⚫️ Stopping tunnel failure monitor", log: log) + os_log("⚫️ Stopping tunnel failure monitor", log: .networkProtectionPixel) task = nil - networkMonitor.cancel() } // MARK: - Handshake monitor diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 6ec8c6faf..80b9fe5f8 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -255,11 +255,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } }() - public lazy var tunnelFailureMonitor = NetworkProtectionTunnelFailureMonitor(tunnelProvider: self, - log: .networkProtectionPixel) - - public lazy var latencyMonitor = NetworkProtectionLatencyMonitor(serverIP: { [weak self] in self?.lastSelectedServerInfo?.ipv4 }, - log: .networkProtectionPixel) + public lazy var tunnelFailureMonitor = NetworkProtectionTunnelFailureMonitor(tunnelProvider: self) + public lazy var latencyMonitor = NetworkProtectionLatencyMonitor() private var lastTestFailed = false private let bandwidthAnalyzer = NetworkProtectionConnectionBandwidthAnalyzer() @@ -1041,11 +1038,18 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { @MainActor private func startLatencyMonitor() { + guard let ip = lastSelectedServerInfo?.ipv4 else { + latencyMonitor.stop() + return + } if latencyMonitor.isStarted { + if latencyMonitor.serverIP == ip { + return + } latencyMonitor.stop() } - latencyMonitor.start { [weak self] result in + latencyMonitor.start(serverIP: ip) { [weak self] result in switch result { case .error: self?.providerEvents.fire(.reportLatency(result: .error)) diff --git a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift index a7dec0006..75521ea10 100644 --- a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift +++ b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift @@ -27,30 +27,16 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { override func setUp() async throws { try await super.setUp() - monitor = NetworkProtectionLatencyMonitor(serverIP: { nil }, log: .disabled) + monitor = NetworkProtectionLatencyMonitor() } override func tearDown() async throws { await monitor?.stop() } - func testInvalidIP() async { - let expectation = XCTestExpectation(description: "Invalid IP reported") - await monitor?.start { result in - switch result { - case .error: - expectation.fulfill() - case .quality: - break - } - } - await monitor?.measureLatency() - await fulfillment(of: [expectation], timeout: 1) - } - func testPingFailure() async { let expectation = XCTestExpectation(description: "Ping failure reported") - await monitor?.start { result in + await monitor?.start(serverIP: .init("127.0.0.1")!) { result in switch result { case .error: expectation.fulfill() @@ -76,10 +62,10 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { } private func testConnectionLatency(_ timeInterval: TimeInterval, expecting expectedQuality: NetworkProtectionLatencyMonitor.ConnectionQuality) async { - let monitor = NetworkProtectionLatencyMonitor(serverIP: { .init("127.0.0.1") }, log: .disabled) + let monitor = NetworkProtectionLatencyMonitor() var reportedQuality = NetworkProtectionLatencyMonitor.ConnectionQuality.unknown - await monitor.start { result in + await monitor.start(serverIP: .init("127.0.0.1")!) { result in switch result { case .quality(let quality): reportedQuality = quality From 02bca61d54e924fe0903d4f0760118cfd33b4f08 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 13:53:02 -0500 Subject: [PATCH 06/12] Remove ignoreThreshold --- .../Diagnostics/NetworkProtectionLatencyMonitor.swift | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 170db7ecb..b54a3027c 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -80,9 +80,6 @@ public actor NetworkProtectionLatencyMonitor { @MainActor private var lastLatencyReported: Date = .distantPast - @MainActor - private var ignoreThreshold = false - @MainActor private(set) var serverIP: IPv4Address? @@ -123,7 +120,7 @@ public actor NetworkProtectionLatencyMonitor { .sink { [weak self] quality in let now = Date() if let self, - (now.timeIntervalSince1970 - self.lastLatencyReported.timeIntervalSince1970 >= Self.reportThreshold) || ignoreThreshold { + now.timeIntervalSince1970 - self.lastLatencyReported.timeIntervalSince1970 >= Self.reportThreshold { callback(.quality(quality)) self.lastLatencyReported = now } @@ -166,9 +163,7 @@ public actor NetworkProtectionLatencyMonitor { @MainActor public func simulateLatency(_ timeInterval: TimeInterval) { - ignoreThreshold = true latencySubject.send(timeInterval) - ignoreThreshold = false } } From 1a1a7979c1051769ab0e4f7705af850f29af1a4d Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 13:55:12 -0500 Subject: [PATCH 07/12] Update logging categories --- .../NetworkProtectionLatencyMonitor.swift | 14 +++++++------- .../NetworkProtectionTunnelFailureMonitor.swift | 8 ++++---- Sources/NetworkProtection/Logging/Logging.swift | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index b54a3027c..f3f380a6c 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -99,7 +99,7 @@ public actor NetworkProtectionLatencyMonitor { @MainActor public func start(serverIP: IPv4Address, callback: @escaping (Result) -> Void) { - os_log("⚫️ Starting latency monitor", log: .networkProtectionPixel) + os_log("⚫️ Starting latency monitor", log: .networkProtectionLatencyMonitorLog) self.serverIP = serverIP @@ -107,12 +107,12 @@ public actor NetworkProtectionLatencyMonitor { .scan(ExponentialGeometricAverage()) { measurements, latency in if latency >= 0 { measurements.addMeasurement(latency) - os_log("⚫️ Latency: %{public}f milliseconds", log: .networkProtectionPixel, type: .debug, latency) + os_log("⚫️ Latency: %{public}f milliseconds", log: .networkProtectionLatencyMonitorLog, type: .debug, latency) } else { callback(.error) } - os_log("⚫️ Average: %{public}f milliseconds", log: .networkProtectionPixel, type: .debug, measurements.average) + os_log("⚫️ Average: %{public}f milliseconds", log: .networkProtectionLatencyMonitorLog, type: .debug, measurements.average) return measurements } @@ -133,7 +133,7 @@ public actor NetworkProtectionLatencyMonitor { @MainActor public func stop() { - os_log("⚫️ Stopping latency monitor", log: .networkProtectionPixel) + os_log("⚫️ Stopping latency monitor", log: .networkProtectionLatencyMonitorLog) latencyCancellable = nil task = nil @@ -148,15 +148,15 @@ public actor NetworkProtectionLatencyMonitor { return } - os_log("⚫️ Pinging %{public}s", log: .networkProtectionPixel, type: .debug, serverIP.debugDescription) + os_log("⚫️ Pinging %{public}s", log: .networkProtectionLatencyMonitorLog, type: .debug, serverIP.debugDescription) - let result = await Pinger(ip: serverIP, timeout: Self.pingTimeout, log: .networkProtectionPixel).ping() + let result = await Pinger(ip: serverIP, timeout: Self.pingTimeout, log: .networkProtectionLatencyMonitorLog).ping() switch result { case .success(let pingResult): latencySubject.send(pingResult.time * 1000) case .failure(let error): - os_log("⚫️ Ping error: %{public}s", log: .networkProtectionPixel, type: .debug, error.localizedDescription) + os_log("⚫️ Ping error: %{public}s", log: .networkProtectionLatencyMonitorLog, type: .debug, error.localizedDescription) latencySubject.send(Self.unknownLatency) } } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index 644488440..1ae42aaf2 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -77,7 +77,7 @@ public actor NetworkProtectionTunnelFailureMonitor { @MainActor func start(callback: @escaping (Result) -> Void) { - os_log("⚫️ Starting tunnel failure monitor", log: .networkProtectionPixel) + os_log("⚫️ Starting tunnel failure monitor", log: .networkProtectionTunnelFailureMonitorLog) failureReported = false @@ -88,7 +88,7 @@ public actor NetworkProtectionTunnelFailureMonitor { @MainActor func stop() { - os_log("⚫️ Stopping tunnel failure monitor", log: .networkProtectionPixel) + os_log("⚫️ Stopping tunnel failure monitor", log: .networkProtectionTunnelFailureMonitorLog) task = nil } @@ -100,11 +100,11 @@ public actor NetworkProtectionTunnelFailureMonitor { let mostRecentHandshake = await tunnelProvider.mostRecentHandshake() ?? 0 let difference = Date().timeIntervalSince1970 - mostRecentHandshake - os_log("⚫️ Last handshake: %{public}f seconds ago", log: .networkProtectionPixel, type: .debug, difference) + os_log("⚫️ Last handshake: %{public}f seconds ago", log: .networkProtectionTunnelFailureMonitorLog, type: .debug, difference) if difference > Result.failureDetected.threshold, isConnected { if failureReported { - os_log("⚫️ Tunnel failure already reported", log: .networkProtectionPixel, type: .debug) + os_log("⚫️ Tunnel failure already reported", log: .networkProtectionTunnelFailureMonitorLog, type: .debug) } else { callback(.failureDetected) failureReported = true diff --git a/Sources/NetworkProtection/Logging/Logging.swift b/Sources/NetworkProtection/Logging/Logging.swift index 6f604e606..d5c4a9a45 100644 --- a/Sources/NetworkProtection/Logging/Logging.swift +++ b/Sources/NetworkProtection/Logging/Logging.swift @@ -29,6 +29,14 @@ extension OSLog { Logging.networkProtectionBandwidthAnalysisLoggingEnabled ? Logging.networkProtectionBandwidthAnalysis : .disabled } + public static var networkProtectionLatencyMonitorLog: OSLog { + Logging.networkProtectionLatencyMonitorLoggingEnabled ? Logging.networkProtectionLatencyMonitor : .disabled + } + + public static var networkProtectionTunnelFailureMonitorLog: OSLog { + Logging.networkProtectionTunnelFailureMonitorLoggingEnabled ? Logging.networkProtectionTunnelFailureMonitor : .disabled + } + public static var networkProtectionConnectionTesterLog: OSLog { Logging.networkProtectionConnectionTesterLoggingEnabled ? Logging.networkProtectionConnectionTesterLog : .disabled } @@ -72,6 +80,12 @@ struct Logging { fileprivate static let networkProtectionBandwidthAnalysisLoggingEnabled = true fileprivate static let networkProtectionBandwidthAnalysis: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Bandwidth Analysis") + + fileprivate static let networkProtectionLatencyMonitorLoggingEnabled = true + fileprivate static let networkProtectionLatencyMonitor: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Latency Monitor") + + fileprivate static let networkProtectionTunnelFailureMonitorLoggingEnabled = true + fileprivate static let networkProtectionTunnelFailureMonitor: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Tunnel Failure Monitor") fileprivate static let networkProtectionConnectionTesterLoggingEnabled = true fileprivate static let networkProtectionConnectionTesterLog: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Connection Tester") From 43da635af85857ab95622d4985305831323d9ea8 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 14:15:35 -0500 Subject: [PATCH 08/12] Remove @MainActor --- .../NetworkProtectionLatencyMonitor.swift | 29 +++++++++---------- ...etworkProtectionTunnelFailureMonitor.swift | 7 ----- .../PacketTunnelProvider.swift | 22 +++++++------- 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index f3f380a6c..11f2cc912 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -59,28 +59,22 @@ public actor NetworkProtectionLatencyMonitor { private static let unknownLatency: TimeInterval = -1 - @MainActor private let latencySubject = PassthroughSubject() - @MainActor private var latencyCancellable: AnyCancellable? - @MainActor private var task: Task? { willSet { task?.cancel() } } - @MainActor var isStarted: Bool { task?.isCancelled == false } - @MainActor private var lastLatencyReported: Date = .distantPast - @MainActor private(set) var serverIP: IPv4Address? // MARK: - Init & deinit @@ -97,13 +91,13 @@ public actor NetworkProtectionLatencyMonitor { // MARK: - Start/Stop monitoring - @MainActor public func start(serverIP: IPv4Address, callback: @escaping (Result) -> Void) { os_log("⚫️ Starting latency monitor", log: .networkProtectionLatencyMonitorLog) self.serverIP = serverIP latencyCancellable = latencySubject.eraseToAnyPublisher() + .receive(on: DispatchQueue.main) .scan(ExponentialGeometricAverage()) { measurements, latency in if latency >= 0 { measurements.addMeasurement(latency) @@ -117,12 +111,14 @@ public actor NetworkProtectionLatencyMonitor { return measurements } .map { ConnectionQuality(average: $0.average) } - .sink { [weak self] quality in - let now = Date() - if let self, - now.timeIntervalSince1970 - self.lastLatencyReported.timeIntervalSince1970 >= Self.reportThreshold { - callback(.quality(quality)) - self.lastLatencyReported = now + .sink { quality in + Task { [weak self] in + let now = Date() + if let self, + await now.timeIntervalSince1970 - self.lastLatencyReported.timeIntervalSince1970 >= Self.reportThreshold { + callback(.quality(quality)) + await self.updateLastLatencyReported(date: now) + } } } @@ -131,7 +127,6 @@ public actor NetworkProtectionLatencyMonitor { } } - @MainActor public func stop() { os_log("⚫️ Stopping latency monitor", log: .networkProtectionLatencyMonitorLog) @@ -139,9 +134,12 @@ public actor NetworkProtectionLatencyMonitor { task = nil } + private func updateLastLatencyReported(date: Date) { + lastLatencyReported = date + } + // MARK: - Latency monitor - @MainActor public func measureLatency() async { guard let serverIP else { latencySubject.send(Self.unknownLatency) @@ -161,7 +159,6 @@ public actor NetworkProtectionLatencyMonitor { } } - @MainActor public func simulateLatency(_ timeInterval: TimeInterval) { latencySubject.send(timeInterval) } diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index 1ae42aaf2..8dbfcf1fc 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -39,14 +39,12 @@ public actor NetworkProtectionTunnelFailureMonitor { private static let monitoringInterval: TimeInterval = .seconds(10) - @MainActor private var task: Task? { willSet { task?.cancel() } } - @MainActor var isStarted: Bool { task?.isCancelled == false } @@ -54,7 +52,6 @@ public actor NetworkProtectionTunnelFailureMonitor { private let tunnelProvider: PacketTunnelProvider private let networkMonitor = NWPathMonitor() - @MainActor private var failureReported = false // MARK: - Init & deinit @@ -75,7 +72,6 @@ public actor NetworkProtectionTunnelFailureMonitor { // MARK: - Start/Stop monitoring - @MainActor func start(callback: @escaping (Result) -> Void) { os_log("⚫️ Starting tunnel failure monitor", log: .networkProtectionTunnelFailureMonitorLog) @@ -86,7 +82,6 @@ public actor NetworkProtectionTunnelFailureMonitor { } } - @MainActor func stop() { os_log("⚫️ Stopping tunnel failure monitor", log: .networkProtectionTunnelFailureMonitorLog) @@ -95,7 +90,6 @@ public actor NetworkProtectionTunnelFailureMonitor { // MARK: - Handshake monitor - @MainActor private func monitorHandshakes(callback: @escaping (Result) -> Void) async { let mostRecentHandshake = await tunnelProvider.mostRecentHandshake() ?? 0 @@ -115,7 +109,6 @@ public actor NetworkProtectionTunnelFailureMonitor { } } - @MainActor private var isConnected: Bool { let path = networkMonitor.currentPath let connectionType = NetworkConnectionType(nwPath: path) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 80b9fe5f8..0f4cf57ed 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -1025,31 +1025,29 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // MARK: - Monitors - @MainActor - private func startTunnelFailureMonitor() { - if tunnelFailureMonitor.isStarted { - tunnelFailureMonitor.stop() + private func startTunnelFailureMonitor() async { + if await tunnelFailureMonitor.isStarted { + await tunnelFailureMonitor.stop() } - tunnelFailureMonitor.start { [weak self] result in + await tunnelFailureMonitor.start { [weak self] result in self?.providerEvents.fire(.reportTunnelFailure(result: result)) } } - @MainActor - private func startLatencyMonitor() { + private func startLatencyMonitor() async { guard let ip = lastSelectedServerInfo?.ipv4 else { - latencyMonitor.stop() + await latencyMonitor.stop() return } - if latencyMonitor.isStarted { - if latencyMonitor.serverIP == ip { + if await latencyMonitor.isStarted { + if await latencyMonitor.serverIP == ip { return } - latencyMonitor.stop() + await latencyMonitor.stop() } - latencyMonitor.start(serverIP: ip) { [weak self] result in + await latencyMonitor.start(serverIP: ip) { [weak self] result in switch result { case .error: self?.providerEvents.fire(.reportLatency(result: .error)) From 84ed8315c52d2ffc6522ceb45026995974cf124d Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 14:45:04 -0500 Subject: [PATCH 09/12] Update tests --- .../NetworkProtectionLatencyMonitorTests.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift index 75521ea10..12e9e0008 100644 --- a/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift +++ b/Tests/NetworkProtectionTests/NetworkProtectionLatencyMonitorTests.swift @@ -69,13 +69,12 @@ final class NetworkProtectionLatencyMonitorTests: XCTestCase { switch result { case .quality(let quality): reportedQuality = quality + XCTAssertEqual(expectedQuality, reportedQuality) case .error: XCTFail("Unexpected result") } } await monitor.simulateLatency(timeInterval) await monitor.stop() - - XCTAssertEqual(expectedQuality, reportedQuality) } } From 0e2a9d98e3b7d2f0da287fd5ab50525b5c7b1859 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 16:34:14 -0500 Subject: [PATCH 10/12] Address PR comments --- .../NetworkProtectionLatencyMonitor.swift | 13 ++++--------- .../NetworkProtectionTunnelFailureMonitor.swift | 4 ++-- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 11f2cc912..7e4a8aa4a 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -55,7 +55,7 @@ public actor NetworkProtectionLatencyMonitor { private static let reportThreshold: TimeInterval = .minutes(10) private static let measurementInterval: TimeInterval = .seconds(5) - private static let pingTimeout: TimeInterval = 0.3 + private static let pingTimeout: TimeInterval = .seconds(1) private static let unknownLatency: TimeInterval = -1 @@ -123,7 +123,7 @@ public actor NetworkProtectionLatencyMonitor { } task = Task.periodic(interval: Self.measurementInterval) { [weak self] in - await self?.measureLatency() + await self?.measureLatency(to: serverIP) } } @@ -140,15 +140,10 @@ public actor NetworkProtectionLatencyMonitor { // MARK: - Latency monitor - public func measureLatency() async { - guard let serverIP else { - latencySubject.send(Self.unknownLatency) - return - } - + private func measureLatency(to ip: IPv4Address) async { os_log("⚫️ Pinging %{public}s", log: .networkProtectionLatencyMonitorLog, type: .debug, serverIP.debugDescription) - let result = await Pinger(ip: serverIP, timeout: Self.pingTimeout, log: .networkProtectionLatencyMonitorLog).ping() + let result = await Pinger(ip: ip, timeout: Self.pingTimeout, log: .networkProtectionLatencyMonitorLog).ping() switch result { case .success(let pingResult): diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift index 8dbfcf1fc..06a219c39 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift @@ -49,7 +49,7 @@ public actor NetworkProtectionTunnelFailureMonitor { task?.isCancelled == false } - private let tunnelProvider: PacketTunnelProvider + private weak var tunnelProvider: PacketTunnelProvider? private let networkMonitor = NWPathMonitor() private var failureReported = false @@ -91,7 +91,7 @@ public actor NetworkProtectionTunnelFailureMonitor { // MARK: - Handshake monitor private func monitorHandshakes(callback: @escaping (Result) -> Void) async { - let mostRecentHandshake = await tunnelProvider.mostRecentHandshake() ?? 0 + let mostRecentHandshake = await tunnelProvider?.mostRecentHandshake() ?? 0 let difference = Date().timeIntervalSince1970 - mostRecentHandshake os_log("⚫️ Last handshake: %{public}f seconds ago", log: .networkProtectionTunnelFailureMonitorLog, type: .debug, difference) From 4c0c3aed60974ba6a8b187eb2e175cf9fc868467 Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 16:42:14 -0500 Subject: [PATCH 11/12] Fix swiftlint issues --- Sources/NetworkProtection/Logging/Logging.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/NetworkProtection/Logging/Logging.swift b/Sources/NetworkProtection/Logging/Logging.swift index d5c4a9a45..33aad17b9 100644 --- a/Sources/NetworkProtection/Logging/Logging.swift +++ b/Sources/NetworkProtection/Logging/Logging.swift @@ -32,7 +32,7 @@ extension OSLog { public static var networkProtectionLatencyMonitorLog: OSLog { Logging.networkProtectionLatencyMonitorLoggingEnabled ? Logging.networkProtectionLatencyMonitor : .disabled } - + public static var networkProtectionTunnelFailureMonitorLog: OSLog { Logging.networkProtectionTunnelFailureMonitorLoggingEnabled ? Logging.networkProtectionTunnelFailureMonitor : .disabled } @@ -80,7 +80,7 @@ struct Logging { fileprivate static let networkProtectionBandwidthAnalysisLoggingEnabled = true fileprivate static let networkProtectionBandwidthAnalysis: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Bandwidth Analysis") - + fileprivate static let networkProtectionLatencyMonitorLoggingEnabled = true fileprivate static let networkProtectionLatencyMonitor: OSLog = OSLog(subsystem: subsystem, category: "Network Protection: Latency Monitor") From 256a31cb498449b1aa5ce7e095b80c2bb40e08ab Mon Sep 17 00:00:00 2001 From: Anh Do Date: Fri, 22 Dec 2023 17:40:31 -0500 Subject: [PATCH 12/12] Clean up --- .../Diagnostics/NetworkProtectionLatencyMonitor.swift | 6 +----- Sources/NetworkProtection/PacketTunnelProvider.swift | 3 --- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift index 7e4a8aa4a..287fabf43 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift @@ -75,8 +75,6 @@ public actor NetworkProtectionLatencyMonitor { private var lastLatencyReported: Date = .distantPast - private(set) var serverIP: IPv4Address? - // MARK: - Init & deinit init() { @@ -94,8 +92,6 @@ public actor NetworkProtectionLatencyMonitor { public func start(serverIP: IPv4Address, callback: @escaping (Result) -> Void) { os_log("⚫️ Starting latency monitor", log: .networkProtectionLatencyMonitorLog) - self.serverIP = serverIP - latencyCancellable = latencySubject.eraseToAnyPublisher() .receive(on: DispatchQueue.main) .scan(ExponentialGeometricAverage()) { measurements, latency in @@ -141,7 +137,7 @@ public actor NetworkProtectionLatencyMonitor { // MARK: - Latency monitor private func measureLatency(to ip: IPv4Address) async { - os_log("⚫️ Pinging %{public}s", log: .networkProtectionLatencyMonitorLog, type: .debug, serverIP.debugDescription) + os_log("⚫️ Pinging %{public}s", log: .networkProtectionLatencyMonitorLog, type: .debug, ip.debugDescription) let result = await Pinger(ip: ip, timeout: Self.pingTimeout, log: .networkProtectionLatencyMonitorLog).ping() diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 0f4cf57ed..9572b51bc 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -1041,9 +1041,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { return } if await latencyMonitor.isStarted { - if await latencyMonitor.serverIP == ip { - return - } await latencyMonitor.stop() }