Skip to content

withAsyncTaskCleanupHandler calls its cleanup handler twice if body throws #81

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
139 changes: 132 additions & 7 deletions Sources/Subprocess/Configuration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -752,31 +752,156 @@ extension Optional where Wrapped == String {
}
}

internal func withAsyncTaskCleanupHandler<Result>(
_ body: () async throws -> Result,
internal func withAsyncTaskCleanupHandler<Success>(
_ body: () async throws -> Success,
onCleanup handler: @Sendable @escaping () async -> Void,
isolation: isolated (any Actor)? = #isolation
) async rethrows -> Result {
) async rethrows -> Success {
let runCancellationHandlerPromise = Promise<Bool, Never>()
return try await withThrowingTaskGroup(
of: Void.self,
returning: Result.self
returning: Success.self
) { group in
group.addTask {
// Keep this task sleep indefinitely until the parent task is cancelled.
// `Task.sleep` throws `CancellationError` when the task is canceled
// before the time ends. We then run the cancel handler.
do { while true { try await Task.sleep(nanoseconds: 1_000_000_000) } } catch {}
// Run task cancel handler
await handler()
runCancellationHandlerPromise.fulfill(with: true)
}

group.addTask {
if await runCancellationHandlerPromise.value {
await handler()
}
}

defer {
group.cancelAll()
}

do {
let result = try await body()
group.cancelAll()
runCancellationHandlerPromise.fulfill(with: false)
return result
} catch {
await handler()
runCancellationHandlerPromise.fulfill(with: true)
throw error
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the issue is simply we don't have to call handler() here and let the one above run. Why do we need Promise?

On a related note, we are really trying very very hard to move away from LockedState like types like this at least on Darwin (this is why we decided to move .standardOutput and .standardError out from the Execution type, so we don't have to hold a LockedState).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot see any other way to ensure the cancellation handler runs only once and only on cancellation or failure. The group.addTask is ALWAYS going to run to completion no matter what happens, but we specifically don't want to run the teardown sequence if no cancellation was invoked or error was thrown. Otherwise you may incorrectly attempt to terminate a still-running process after the body closure runs.

}
}
}

#if canImport(Darwin)
import os
#else
import Synchronization
#endif

/// Represents a placeholder for a value which will be provided by synchronous code running in another execution context.
internal final class Promise<Success: Sendable, Failure: Swift.Error>: Sendable {
private struct State {
var waiters: [CheckedContinuation<Success, Failure>] = []
var value: Result<Success, Failure>?
}

#if canImport(Darwin)
private let state: OSAllocatedUnfairLock<State>
#else
private let state: Mutex<State>
#endif

/// Creates a new promise in the unfulfilled state.
public init() {
#if canImport(Darwin)
state = OSAllocatedUnfairLock(initialState: State(value: nil))
#else
state = Mutex(State(value: nil))
#endif
}

deinit {
state.withLock { state in
precondition(state.waiters.isEmpty, "Deallocated with remaining waiters")
}
}

/// Fulfills the promise with the specified result.
///
/// - returns: Whether the promise was already fulfilled.
@discardableResult
public func fulfill(with result: Result<Success, Failure>) -> Bool {
let (waiters, alreadyFulfilled): ([CheckedContinuation<Success, Failure>], Bool) = state.withLock { state in
if state.value != nil {
return ([], true)
}
state.value = result
let waiters = state.waiters
state.waiters.removeAll()
return (waiters, false)
}

// Resume the continuations outside the lock to avoid potential deadlock if invoked in a cancellation handler.
for waiter in waiters {
waiter.resume(with: result)
}

return alreadyFulfilled
}

/// Fulfills the promise with the specified value.
///
/// - returns: Whether the promise was already fulfilled.
@discardableResult
public func fulfill(with value: Success) -> Bool {
fulfill(with: .success(value))
}

/// Fulfills the promise with the specified error.
///
/// - returns: Whether the promise was already fulfilled.
@discardableResult
public func fail(throwing error: Failure) -> Bool {
fulfill(with: .failure(error))
}
}

extension Promise where Success == Void {
/// Fulfills the promise.
///
/// - returns: Whether the promise was already fulfilled.
@discardableResult
internal func fulfill() -> Bool {
fulfill(with: ())
}
}

extension Promise where Failure == Never {
/// Suspends if the promise is not yet fulfilled, and returns the value once it is.
internal var value: Success {
get async {
await withCheckedContinuation { continuation in
let value: Result<Success, Never>? = state.withLock { state in
if let value = state.value {
return value
} else {
state.waiters.append(continuation)
return nil
}
}

// Resume the continuations outside the lock to avoid potential deadlock if invoked in a cancellation handler.
if let value {
continuation.resume(with: value)
}
}
}
}

/// Suspends if the promise is not yet fulfilled, and returns the result once it is.
internal var result: Result<Success, Failure> {
get async {
await .success(value)
}
}
}
Loading