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

Fix a race condition bug involving timers and async tasks #6045

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ios/PacketTunnelCore/Actor/Task+Duration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,21 @@ extension Task where Success == Never, Failure == Never {

try await withTaskCancellationHandler {
try await withCheckedThrowingContinuation { continuation in
// The `continuation` handler should never be `resume`'d more than once.
// Setting the eventHandler on the timer after it has been cancelled will be ignored.
// https://github.com/apple/swift-corelibs-libdispatch/blob/77766345740dfe3075f2f60bead854b29b0cfa24/src/source.c#L338
// Therefore, set a flag indicating `resume` has already been called to avoid `resume`ing more than once.
// Cancelling the timer does not cancel an event handler that is already running however,
// the cancel handler will be scheduled after the event handler has finished.
// https://developer.apple.com/documentation/dispatch/1385604-dispatch_source_cancel
// Therefore, there it is safe to do this here.
var hasResumed = false
timer.setEventHandler {
hasResumed = true
continuation.resume()
}
timer.setCancelHandler {
guard hasResumed == false else { return }
continuation.resume(throwing: TaskCancellationError())
}
timer.schedule(wallDeadline: .now() + DispatchTimeInterval.milliseconds(duration.milliseconds))
Expand Down
27 changes: 27 additions & 0 deletions ios/PacketTunnelCoreTests/TaskSleepTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,31 @@ final class TaskSleepTests: XCTestCase {
XCTAssert(error is CancellationError)
}
}

/// This test triggers a race condition in `sleepUsingContinuousClock` where an `AutoCancellingTask` will
/// cancel a `DispatchSourceTimer` in a `Task` trying to call `resume` on its continuation handler more than once
func testSuccessfulEventHandlerRemovesCancellation() async throws {
for _ in 0 ... 20 {
let task = recoveryTask()
try await Task.sleep(duration: .milliseconds(10))
task.callDummyFunctionToForceConcurrencyWait()
}
}

private func recoveryTask() -> AutoCancellingTask {
AutoCancellingTask(Task.detached {
while Task.isCancelled == false {
try await Task.sleepUsingContinuousClock(for: .milliseconds(10))
}
})
}
}

private extension AutoCancellingTask {
/// This function is here to silence a warning about unused variables in `testSuccessfulEventHandlerRemovesCancellation`
/// The following construct `_ = recoveryTask()` cannot be used as the resulting `AutoCancellingTask`
/// would immediately get `deinit`ed, changing the test scenario.
/// A dummy function is needed to make sure the task is not cancelled before concurrency is forced
/// by having a call to `Task.sleep`
func callDummyFunctionToForceConcurrencyWait() {}
}
Loading