Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a 2 second delay before shutting down the tunnel when encountering an error #1078

Merged
merged 9 commits into from
Nov 22, 2024
12 changes: 10 additions & 2 deletions Sources/NetworkProtection/PacketTunnelProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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) {
// Wait for the provider to complete its pixel request.
providerEvents.fire(.malformedErrorDetected(error))
try? await Task.sleep(interval: .seconds(2))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to wait for max 2s, or do we want to focus on reliability of sending? Asking cause we could use .fire call and wait for completion, but that could take some time.

Copy link
Contributor Author

@samsymons samsymons Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focusing on reliability is a better path here, I'll adapt the EventMapper instance such that we can await the fire call and remove the sleep.

EDIT: Started working on this but the change isn't trivial, I'll ping for re-review once it's ready.

throw wrappedError
} else {
// Wait for the provider to complete its pixel request.
try? await Task.sleep(interval: .seconds(2))
throw error
}
}
Expand All @@ -725,7 +729,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)
Expand All @@ -738,9 +742,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
}
}
Expand Down
Loading