Skip to content

Commit 9c63d1f

Browse files
atavismatavism
committed
code review updates
1 parent c1c79eb commit 9c63d1f

4 files changed

Lines changed: 111 additions & 41 deletions

File tree

‎lantern-core/mobile/mobile_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import (
1010

1111
func TestGetClientDoesNotWaitForIPCLifecycleLock(t *testing.T) {
1212
want := &ipc.Client{}
13-
ipcClient.Store(want)
13+
previousClient := ipcClient.Swap(want)
1414
t.Cleanup(func() {
15-
ipcClient.Store(nil)
15+
ipcClient.Store(previousClient)
1616
})
1717

1818
ipcMu.Lock()

‎macos/Runner/VPN/VPNBase.swift‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,21 @@ enum VPNManagerError: LocalizedError, Equatable {
3131

3232
/// Coordinates connection changes while allowing stop to cancel a pending start.
3333
actor VPNLifecycleCoordinator {
34+
private struct StopWaiter {
35+
let connectionID: UInt
36+
let continuation: CheckedContinuation<Void, Never>
37+
}
38+
3439
private var nextConnectionID: UInt = 0
3540
private var activeConnectionID: UInt?
3641
private var stopPending = false
37-
private var stopWaiters: [CheckedContinuation<Void, Never>] = []
42+
private var stopWaiter: StopWaiter?
43+
private var stopHandoffTimeoutTask: Task<Void, Never>?
44+
private let stopHandoffTimeoutNanoseconds: UInt64
45+
46+
init(stopHandoffTimeoutNanoseconds: UInt64 = 5_000_000_000) {
47+
self.stopHandoffTimeoutNanoseconds = stopHandoffTimeoutNanoseconds
48+
}
3849

3950
/// Starts a connection operation unless another lifecycle change owns the manager.
4051
func beginConnectionOperation() throws -> UInt {
@@ -55,22 +66,29 @@ actor VPNLifecycleCoordinator {
5566
func endConnectionOperation(_ id: UInt) {
5667
guard activeConnectionID == id else { return }
5768
activeConnectionID = nil
58-
let waiters = stopWaiters
59-
stopWaiters.removeAll()
60-
waiters.forEach { $0.resume() }
69+
finishStopHandoff(for: id)
6170
}
6271

63-
/// Cancels any active connection operation and waits for its profile writes to finish.
72+
/// Cancels any active connection operation and briefly waits for it to hand off the manager.
6473
/// The return value tells the caller to tear down even if the system status has not caught up.
6574
func beginStopOperation() async throws -> Bool {
6675
guard !stopPending else {
6776
throw VPNManagerError.operationInProgress
6877
}
6978
stopPending = true
70-
let canceledConnectionOperation = activeConnectionID != nil
71-
guard canceledConnectionOperation else { return false }
79+
guard let connectionID = activeConnectionID else { return false }
80+
let timeout = stopHandoffTimeoutNanoseconds
81+
7282
await withCheckedContinuation { continuation in
73-
stopWaiters.append(continuation)
83+
stopWaiter = StopWaiter(connectionID: connectionID, continuation: continuation)
84+
stopHandoffTimeoutTask = Task { [weak self] in
85+
do {
86+
try await Task.sleep(nanoseconds: timeout)
87+
} catch {
88+
return
89+
}
90+
await self?.expireStopHandoff(for: connectionID)
91+
}
7492
}
7593
return true
7694
}
@@ -79,6 +97,24 @@ actor VPNLifecycleCoordinator {
7997
func endStopOperation() {
8098
stopPending = false
8199
}
100+
101+
private func finishStopHandoff(for connectionID: UInt) {
102+
guard let waiter = stopWaiter, waiter.connectionID == connectionID else { return }
103+
stopWaiter = nil
104+
stopHandoffTimeoutTask?.cancel()
105+
stopHandoffTimeoutTask = nil
106+
waiter.continuation.resume()
107+
}
108+
109+
private func expireStopHandoff(for connectionID: UInt) {
110+
guard let waiter = stopWaiter, waiter.connectionID == connectionID else { return }
111+
stopWaiter = nil
112+
stopHandoffTimeoutTask = nil
113+
if activeConnectionID == connectionID {
114+
activeConnectionID = nil
115+
}
116+
waiter.continuation.resume()
117+
}
82118
}
83119

84120
/// Returns whether a new tunnel should start for the current system status.
@@ -102,10 +138,8 @@ func shouldStopTunnel(for status: NEVPNStatus) throws -> Bool {
102138
switch status {
103139
case .connected, .connecting, .reasserting:
104140
return true
105-
case .disconnected, .disconnecting:
141+
case .disconnected, .disconnecting, .invalid:
106142
return false
107-
case .invalid:
108-
throw VPNManagerError.loadingProviderFailed
109143
@unknown default:
110144
throw VPNManagerError.unknown
111145
}

‎macos/Runner/VPN/VPNManager.swift‎

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -136,18 +136,12 @@ class VPNManager: VPNBase {
136136
/// Starts the VPN tunnel.
137137
/// Loads VPN preferences and initiates the VPN connection.
138138
func startTunnel() async throws {
139-
let operationID = try await lifecycleCoordinator.beginConnectionOperation()
140-
do {
139+
try await withConnectionOperation { operationID in
141140
try await startTunnel(operationID: operationID)
142-
} catch {
143-
await lifecycleCoordinator.endConnectionOperation(operationID)
144-
throw error
145141
}
146-
await lifecycleCoordinator.endConnectionOperation(operationID)
147142
}
148143

149144
private func startTunnel(operationID: UInt) async throws {
150-
151145
appLogger.log("Starting tunnel..")
152146
await self.setupVPN()
153147
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
@@ -173,18 +167,12 @@ class VPNManager: VPNBase {
173167
func connectToServer(
174168
serverName: String
175169
) async throws {
176-
let operationID = try await lifecycleCoordinator.beginConnectionOperation()
177-
do {
170+
try await withConnectionOperation { operationID in
178171
try await connectToServer(serverName: serverName, operationID: operationID)
179-
} catch {
180-
await lifecycleCoordinator.endConnectionOperation(operationID)
181-
throw error
182172
}
183-
await lifecycleCoordinator.endConnectionOperation(operationID)
184173
}
185174

186175
private func connectToServer(serverName: String, operationID: UInt) async throws {
187-
188176
await self.setupVPN()
189177
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
190178
throw CancellationError()
@@ -204,12 +192,10 @@ class VPNManager: VPNBase {
204192
return
205193
}
206194

195+
guard await lifecycleCoordinator.canContinueConnectionOperation(operationID) else {
196+
throw CancellationError()
197+
}
207198
try self.manager.connection.startVPNTunnel(options: options)
208-
/// Enable on-demand to allow automatic reconnections
209-
/// if error it will stuck in infinite loop
210-
// self.manager.isOnDemandEnabled = true
211-
// try await self.saveThenLoadProvider()
212-
213199
}
214200

215201
/// Stops the VPN tunnel.
@@ -227,8 +213,21 @@ class VPNManager: VPNBase {
227213
}
228214

229215
private func stopTunnelAfterHandoff(canceledConnectionOperation: Bool) async throws {
230-
231216
appLogger.log("Stopping tunnel..")
217+
218+
// A canceled start already owns the current manager. Stop it before any
219+
// preference call can delay teardown again.
220+
if canceledConnectionOperation {
221+
let shouldSaveOnDemandChange = manager.isOnDemandEnabled
222+
manager.isOnDemandEnabled = false
223+
manager.connection.stopVPNTunnel()
224+
if shouldSaveOnDemandChange {
225+
try await manager.saveToPreferences()
226+
}
227+
appLogger.log("Tunnel stopped.")
228+
return
229+
}
230+
232231
await syncStatus()
233232
let status = manager.connection.status
234233
let shouldStop: Bool
@@ -251,6 +250,20 @@ class VPNManager: VPNBase {
251250
appLogger.log("Tunnel stopped.")
252251
}
253252

253+
private func withConnectionOperation<T>(
254+
_ operation: (UInt) async throws -> T
255+
) async throws -> T {
256+
let operationID = try await lifecycleCoordinator.beginConnectionOperation()
257+
do {
258+
let result = try await operation(operationID)
259+
await lifecycleCoordinator.endConnectionOperation(operationID)
260+
return result
261+
} catch {
262+
await lifecycleCoordinator.endConnectionOperation(operationID)
263+
throw error
264+
}
265+
}
266+
254267
/// Saves the current VPN configuration to preferences and reloads it.
255268
private func saveThenLoadProvider() async throws {
256269
try await self.manager.saveToPreferences()

‎macos/RunnerTests/RunnerTests.swift‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,9 @@ final class RunnerTests: XCTestCase {
3939
for status in [NEVPNStatus.connected, .connecting, .reasserting] {
4040
XCTAssertTrue(try shouldStopTunnel(for: status))
4141
}
42-
for status in [NEVPNStatus.disconnected, .disconnecting] {
42+
for status in [NEVPNStatus.disconnected, .disconnecting, .invalid] {
4343
XCTAssertFalse(try shouldStopTunnel(for: status))
4444
}
45-
46-
assertVPNManagerError(.loadingProviderFailed) {
47-
_ = try shouldStopTunnel(for: .invalid)
48-
}
4945
}
5046

5147
func testVPNLifecycleCoordinatorRejectsOverlappingConnections() async throws {
@@ -67,15 +63,20 @@ final class RunnerTests: XCTestCase {
6763
func testVPNStopCancelsStartupAndWaitsForHandoff() async throws {
6864
let coordinator = VPNLifecycleCoordinator()
6965
let operationID = try await coordinator.beginConnectionOperation()
70-
let stopStarted = expectation(description: "Stop requested")
7166

7267
let stopTask = Task {
73-
stopStarted.fulfill()
7468
return try await coordinator.beginStopOperation()
7569
}
76-
await fulfillment(of: [stopStarted])
7770

71+
let deadline = Date().addingTimeInterval(1)
7872
while await coordinator.canContinueConnectionOperation(operationID) {
73+
if Date() >= deadline {
74+
await coordinator.endConnectionOperation(operationID)
75+
_ = try await stopTask.value
76+
await coordinator.endStopOperation()
77+
XCTFail("Stop request did not take ownership of the manager")
78+
return
79+
}
7980
await Task.yield()
8081
}
8182
await coordinator.endConnectionOperation(operationID)
@@ -94,6 +95,28 @@ final class RunnerTests: XCTestCase {
9495
await coordinator.endConnectionOperation(nextOperationID)
9596
}
9697

98+
func testVPNStopForcesHandoffAfterTimeout() async throws {
99+
let coordinator = VPNLifecycleCoordinator(stopHandoffTimeoutNanoseconds: 10_000_000)
100+
let operationID = try await coordinator.beginConnectionOperation()
101+
let stopFinished = expectation(description: "Stop handoff finished")
102+
103+
let stopTask = Task {
104+
let result = try await coordinator.beginStopOperation()
105+
stopFinished.fulfill()
106+
return result
107+
}
108+
await fulfillment(of: [stopFinished], timeout: 1)
109+
let canceledConnectionOperation = try await stopTask.value
110+
XCTAssertTrue(canceledConnectionOperation)
111+
let canContinue = await coordinator.canContinueConnectionOperation(operationID)
112+
XCTAssertFalse(canContinue)
113+
114+
await coordinator.endConnectionOperation(operationID)
115+
await coordinator.endStopOperation()
116+
let nextOperationID = try await coordinator.beginConnectionOperation()
117+
await coordinator.endConnectionOperation(nextOperationID)
118+
}
119+
97120
func testHashBundleIsStableForIdenticalContents() throws {
98121
let firstURL = try createExtensionBundle(
99122
name: "First.systemextension",

0 commit comments

Comments
 (0)