From 53d4104459e169d5868d4c8f9d7d784f3967b4d0 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 14 Nov 2024 19:52:38 -0800 Subject: [PATCH 1/6] Add a 2 second delay before shutting down the tunnel when encountering an error. --- Sources/NetworkProtection/PacketTunnelProvider.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index f1a8ba409..41e7073f0 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -691,7 +691,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // expired. In either case it should be enough to record the manual failures // for these prerequisited to avoid flooding our metrics. providerEvents.fire(.tunnelStartOnDemandWithoutAccessToken) - try await Task.sleep(interval: .seconds(15)) + try? await Task.sleep(interval: .seconds(15)) } else { // If the VPN was started manually without the basic prerequisites we always // want to know as this should not be possible. @@ -704,7 +704,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { + // Wait for the provider to complete its pixel request. providerEvents.fire(.malformedErrorDetected(error)) + try? await Task.sleep(interval: .seconds(2)) throw wrappedError } else { throw error @@ -725,7 +727,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // We add a delay when the VPN is started by // on-demand and there's an error, to avoid frenetic ON/OFF // cycling. - try await Task.sleep(interval: .seconds(15)) + try? await Task.sleep(interval: .seconds(15)) } let errorDescription = (error as? LocalizedError)?.localizedDescription ?? String(describing: error) @@ -738,9 +740,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { + // Wait for the provider to complete its pixel request. providerEvents.fire(.malformedErrorDetected(error)) + try? await Task.sleep(interval: .seconds(2)) throw wrappedError } else { + // Wait for the provider to complete its pixel request. + try? await Task.sleep(interval: .seconds(2)) throw error } } From 69d5dbc93b2e781a42806e91a56c1697db504194 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 14 Nov 2024 19:56:47 -0800 Subject: [PATCH 2/6] Add additional delay. --- Sources/NetworkProtection/PacketTunnelProvider.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 41e7073f0..5ebce3409 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -709,6 +709,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { try? await Task.sleep(interval: .seconds(2)) throw wrappedError } else { + // Wait for the provider to complete its pixel request. + try? await Task.sleep(interval: .seconds(2)) throw error } } From c587b858cb71b996f78ea50a63b1bad72d9b7b6d Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Sun, 17 Nov 2024 17:16:12 -0800 Subject: [PATCH 3/6] Add async wrapper around EventMapper.fire. Using a name like `asyncFire` isn't ideal, but otherwise this function signature perfectly matches the function it's wrapping. --- Sources/Common/EventMapping.swift | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Sources/Common/EventMapping.swift b/Sources/Common/EventMapping.swift index b152f7370..72b593b79 100644 --- a/Sources/Common/EventMapping.swift +++ b/Sources/Common/EventMapping.swift @@ -30,7 +30,20 @@ open class EventMapping { eventMapper = mapping } + @available(*, renamed: "fire(_:error:parameters:)") public func fire(_ event: Event, error: Error? = nil, parameters: [String: String]? = nil, onComplete: @escaping (Error?) -> Void = {_ in }) { eventMapper(event, error, parameters, onComplete) } + + public func asyncFire(_ event: Event, error: Error? = nil, parameters: [String: String]? = nil) async throws { + return try await withCheckedThrowingContinuation { continuation in + fire(event, error: error, parameters: parameters) { error in + if let error = error { + continuation.resume(throwing: error) + return + } + continuation.resume(returning: ()) + } + } + } } From c4f2c76f0549d27fffcc657760f3517178f84868 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Sun, 17 Nov 2024 17:16:36 -0800 Subject: [PATCH 4/6] Remove 2-second sleep calls during VPN shutdown. --- .../PacketTunnelProvider.swift | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 5ebce3409..d86263179 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -695,8 +695,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } else { // If the VPN was started manually without the basic prerequisites we always // want to know as this should not be possible. - providerEvents.fire(.tunnelStartAttempt(.begin)) - providerEvents.fire(.tunnelStartAttempt(.failure(error))) + try? await providerEvents.asyncFire(.tunnelStartAttempt(.begin)) + try? await providerEvents.asyncFire(.tunnelStartAttempt(.failure(error))) } Logger.networkProtection.log("🔴 Stopping VPN due to no auth token") @@ -704,13 +704,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { - // Wait for the provider to complete its pixel request. - providerEvents.fire(.malformedErrorDetected(error)) - try? await Task.sleep(interval: .seconds(2)) + try? await providerEvents.asyncFire(.malformedErrorDetected(error)) throw wrappedError } else { - // Wait for the provider to complete its pixel request. - try? await Task.sleep(interval: .seconds(2)) throw error } } @@ -738,17 +734,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.connectionStatus = .disconnected self.knownFailureStore.lastKnownFailure = KnownFailure(error) - providerEvents.fire(.tunnelStartAttempt(.failure(error))) + try? await providerEvents.asyncFire(.tunnelStartAttempt(.failure(error))) // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { - // Wait for the provider to complete its pixel request. - providerEvents.fire(.malformedErrorDetected(error)) - try? await Task.sleep(interval: .seconds(2)) + try? await providerEvents.asyncFire(.malformedErrorDetected(error)) throw wrappedError } else { - // Wait for the provider to complete its pixel request. - try? await Task.sleep(interval: .seconds(2)) throw error } } From cea228610cc6172e86d4d0d7661c4483527c3fc2 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Mon, 18 Nov 2024 11:02:15 -0800 Subject: [PATCH 5/6] Revert "Remove 2-second sleep calls during VPN shutdown." This reverts commit c4f2c76f0549d27fffcc657760f3517178f84868. --- .../PacketTunnelProvider.swift | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index d86263179..5ebce3409 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -695,8 +695,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } else { // If the VPN was started manually without the basic prerequisites we always // want to know as this should not be possible. - try? await providerEvents.asyncFire(.tunnelStartAttempt(.begin)) - try? await providerEvents.asyncFire(.tunnelStartAttempt(.failure(error))) + providerEvents.fire(.tunnelStartAttempt(.begin)) + providerEvents.fire(.tunnelStartAttempt(.failure(error))) } Logger.networkProtection.log("🔴 Stopping VPN due to no auth token") @@ -704,9 +704,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { - try? await providerEvents.asyncFire(.malformedErrorDetected(error)) + // Wait for the provider to complete its pixel request. + providerEvents.fire(.malformedErrorDetected(error)) + try? await Task.sleep(interval: .seconds(2)) throw wrappedError } else { + // Wait for the provider to complete its pixel request. + try? await Task.sleep(interval: .seconds(2)) throw error } } @@ -734,13 +738,17 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.connectionStatus = .disconnected self.knownFailureStore.lastKnownFailure = KnownFailure(error) - try? await providerEvents.asyncFire(.tunnelStartAttempt(.failure(error))) + providerEvents.fire(.tunnelStartAttempt(.failure(error))) // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down if let wrappedError = wrapped(error: error) { - try? await providerEvents.asyncFire(.malformedErrorDetected(error)) + // Wait for the provider to complete its pixel request. + providerEvents.fire(.malformedErrorDetected(error)) + try? await Task.sleep(interval: .seconds(2)) throw wrappedError } else { + // Wait for the provider to complete its pixel request. + try? await Task.sleep(interval: .seconds(2)) throw error } } From 8ac7c24163ce301ca092cb13804b47bb32937bdd Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Mon, 18 Nov 2024 11:02:29 -0800 Subject: [PATCH 6/6] Revert "Add async wrapper around EventMapper.fire." This reverts commit c587b858cb71b996f78ea50a63b1bad72d9b7b6d. --- Sources/Common/EventMapping.swift | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/Sources/Common/EventMapping.swift b/Sources/Common/EventMapping.swift index 72b593b79..b152f7370 100644 --- a/Sources/Common/EventMapping.swift +++ b/Sources/Common/EventMapping.swift @@ -30,20 +30,7 @@ open class EventMapping { eventMapper = mapping } - @available(*, renamed: "fire(_:error:parameters:)") public func fire(_ event: Event, error: Error? = nil, parameters: [String: String]? = nil, onComplete: @escaping (Error?) -> Void = {_ in }) { eventMapper(event, error, parameters, onComplete) } - - public func asyncFire(_ event: Event, error: Error? = nil, parameters: [String: String]? = nil) async throws { - return try await withCheckedThrowingContinuation { continuation in - fire(event, error: error, parameters: parameters) { error in - if let error = error { - continuation.resume(throwing: error) - return - } - continuation.resume(returning: ()) - } - } - } }