From 81a9a28dcdc0fed8bf4912988e31ea7944f325ce Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Sat, 14 Jun 2025 15:12:38 -0700 Subject: [PATCH 1/2] withAsyncTaskCleanupHandler calls its cleanup handler twice if body throws If the body closure given to withAsyncTaskCleanupHandler throws, its cleanup handler is called twice: once in the catch block where body is called, and then again in the task group task once the Task.sleep throws due to cancellation, which swallows the error and then continues to call the handler as well. That results in the teardown sequence being invoked twice, as well as the teardown sequence being invoked for non-error cases. This patch ensures the cleanup handler is invoked in failure cases only, and only once. Closes #80 --- Sources/Subprocess/Configuration.swift | 31 +++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/Sources/Subprocess/Configuration.swift b/Sources/Subprocess/Configuration.swift index d67eede..6dcc080 100644 --- a/Sources/Subprocess/Configuration.swift +++ b/Sources/Subprocess/Configuration.swift @@ -752,11 +752,13 @@ extension Optional where Wrapped == String { } } +/// Runs `body`, and then runs `onCleanup` if body throws an error, or if the parent task is cancelled. In the latter case, `onCleanup` may be run concurrently with `body`. `body` is guaranteed to run exactly once. `onCleanup` is guaranteed to run only once, or not at all. internal func withAsyncTaskCleanupHandler( _ body: () async throws -> Result, onCleanup handler: @Sendable @escaping () async -> Void, isolation: isolated (any Actor)? = #isolation ) async rethrows -> Result { + let (runCancellationHandlerStream, runCancellationHandlerContinuation) = AsyncThrowingStream.makeStream(of: Void.self) return try await withThrowingTaskGroup( of: Void.self, returning: Result.self @@ -767,15 +769,38 @@ internal func withAsyncTaskCleanupHandler( // 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() + runCancellationHandlerContinuation.finish(throwing: CancellationError()) + } + + group.addTask { + // Enumerate the async stream until it completes or throws an error. + // Since we signal completion of the stream from cancellation or the + // parent task or the body throwing, this ensures that we run the + // cleanup handler exactly once in any failure scenario, and also do + // so _immediately_ if the failure scenario is due to parent task + // cancellation. We do so in a detached Task to prevent cancellation + // of the parent task from interrupting enumeration of the stream itself. + await Task.detached { + do { + var iterator = runCancellationHandlerStream.makeAsyncIterator() + while let _ = try await iterator.next() { + } + } catch { + await handler() + } + }.value + } + + defer { + group.cancelAll() } do { let result = try await body() - group.cancelAll() + runCancellationHandlerContinuation.finish() return result } catch { - await handler() + runCancellationHandlerContinuation.finish(throwing: error) throw error } } From b38fd2ba7096f8bbedfbbcebc8621d6ff969fc7d Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Mon, 16 Jun 2025 23:13:07 -0700 Subject: [PATCH 2/2] Use of Execution and teardown sequences race with process termination Process termination monitoring is started asynchronously with the closure which receives the Execution, meaning any use of Execution in the body to send signals, etc., may send them to a pid which no longer refers to the original process. The cleanup handler may also run after the process has already terminated. --- Sources/Subprocess/Configuration.swift | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Sources/Subprocess/Configuration.swift b/Sources/Subprocess/Configuration.swift index 6dcc080..0f4d533 100644 --- a/Sources/Subprocess/Configuration.swift +++ b/Sources/Subprocess/Configuration.swift @@ -74,23 +74,17 @@ public struct Configuration: Sendable { let pid = spawnResults.execution.processIdentifier var spawnResultBox: SpawnResult?? = consume spawnResults + var _spawnResult = spawnResultBox!.take()! - return try await withAsyncTaskCleanupHandler { - var _spawnResult = spawnResultBox!.take()! + let processIdentifier = _spawnResult.execution.processIdentifier + + let result = try await withAsyncTaskCleanupHandler { let inputIO = _spawnResult.inputWriteEnd() let outputIO = _spawnResult.outputReadEnd() let errorIO = _spawnResult.errorReadEnd() - let processIdentifier = _spawnResult.execution.processIdentifier - async let terminationStatus = try monitorProcessTermination( - forProcessWithIdentifier: processIdentifier - ) // Body runs in the same isolation - let result = try await body(_spawnResult.execution, inputIO, outputIO, errorIO) - return ExecutionResult( - terminationStatus: try await terminationStatus, - value: result - ) + return try await body(_spawnResult.execution, inputIO, outputIO, errorIO) } onCleanup: { // Attempt to terminate the child process await Execution.runTeardownSequence( @@ -98,6 +92,11 @@ public struct Configuration: Sendable { on: pid ) } + + return ExecutionResult( + terminationStatus: try await monitorProcessTermination(forProcessWithIdentifier: processIdentifier), + value: result + ) } }