From 14c7b48d8e5b55d3d43df88bb53b7e9f7d0e57ca Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Sun, 29 Jun 2025 13:02:23 -0700 Subject: [PATCH] Change runDetached to return a ProcessHandle instead of a ProcessIdentifier Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns. This patch now returns a non-copyable ProcessHandle, which provides access to both the ProcessIdentifier and the underlying platform handles (like Execution), while also guaranteeing that the underlying platform handles are released once the ProcessHandle goes out of scope. In effect, the platform handles are now released at the end of the runDetached caller's scope, rather than just before runDetached itself returns to the caller, solving the Windows-specific race condition. This requires new API because ProcessIdentifier is Hashable and Codable and can't reasonably be made non-Copyable. Similarly, it's likely undesirable to use Execution as the return type because it too would need to be made non-Copyable to prevent the resource leak and there is at least one example in existing tests which requires Execution to be Copyable. Closes #94 --- Sources/Subprocess/API.swift | 7 +++-- Sources/Subprocess/Execution.swift | 26 +++++++++++++++++++ .../SubprocessTests+Unix.swift | 3 ++- .../SubprocessTests+Windows.swift | 17 +++--------- 4 files changed, 34 insertions(+), 19 deletions(-) diff --git a/Sources/Subprocess/API.swift b/Sources/Subprocess/API.swift index b7788d6..1c1c97e 100644 --- a/Sources/Subprocess/API.swift +++ b/Sources/Subprocess/API.swift @@ -614,7 +614,7 @@ public func runDetached( input: FileDescriptor? = nil, output: FileDescriptor? = nil, error: FileDescriptor? = nil -) throws -> ProcessIdentifier { +) throws -> ProcessHandle { let config: Configuration = Configuration( executable: executable, arguments: arguments, @@ -643,7 +643,7 @@ public func runDetached( input: FileDescriptor? = nil, output: FileDescriptor? = nil, error: FileDescriptor? = nil -) throws -> ProcessIdentifier { +) throws -> ProcessHandle { let execution: Execution switch (input, output, error) { case (.none, .none, .none): @@ -753,7 +753,6 @@ public func runDetached( errorPipe: try processError.createPipe() ).execution } - execution.release() - return execution.processIdentifier + return ProcessHandle(execution: execution) } diff --git a/Sources/Subprocess/Execution.swift b/Sources/Subprocess/Execution.swift index ded276f..fcd816d 100644 --- a/Sources/Subprocess/Execution.swift +++ b/Sources/Subprocess/Execution.swift @@ -50,6 +50,32 @@ public struct Execution: Sendable { } } +/// An object that represents a subprocess that has been +/// executed without monitoring the state of the subprocess +/// nor waiting until it exits. You can use this object to control +/// the process using platform-native APIs. +public struct ProcessHandle: Sendable, ~Copyable { + /// The process identifier of the current execution + public var processIdentifier: ProcessIdentifier { + execution.processIdentifier + } + + /// The collection of platform-specific handles of the current execution. + public var platformHandles: PlatformHandles { + execution.platformHandles + } + + private let execution: Execution + + init(execution: Execution) { + self.execution = execution + } + + deinit { + execution.release() + } +} + // MARK: - Output Capture internal enum OutputCapturingState: Sendable { case standardOutputCaptured(Output) diff --git a/Tests/SubprocessTests/SubprocessTests+Unix.swift b/Tests/SubprocessTests/SubprocessTests+Unix.swift index a50c4e8..d1d469d 100644 --- a/Tests/SubprocessTests/SubprocessTests+Unix.swift +++ b/Tests/SubprocessTests/SubprocessTests+Unix.swift @@ -779,11 +779,12 @@ extension SubprocessUnixTests { extension SubprocessUnixTests { @Test func testRunDetached() async throws { let (readFd, writeFd) = try FileDescriptor.pipe() - let pid = try runDetached( + let execution = try runDetached( .path("/bin/sh"), arguments: ["-c", "echo $$"], output: writeFd ) + let pid = execution.processIdentifier var status: Int32 = 0 waitpid(pid.value, &status, 0) #expect(_was_process_exited(status) > 0) diff --git a/Tests/SubprocessTests/SubprocessTests+Windows.swift b/Tests/SubprocessTests/SubprocessTests+Windows.swift index 00d9416..9c222e3 100644 --- a/Tests/SubprocessTests/SubprocessTests+Windows.swift +++ b/Tests/SubprocessTests/SubprocessTests+Windows.swift @@ -729,27 +729,16 @@ extension SubprocessWindowsTests { DWORD(HANDLE_FLAG_INHERIT), 0 ) - let pid = try Subprocess.runDetached( + let execution = try Subprocess.runDetached( .path("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"), arguments: [ "-Command", "Write-Host $PID", ], output: writeFd ) - // Wait for process to finish - guard - let processHandle = OpenProcess( - DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE), - false, - pid.value - ) - else { - Issue.record("Failed to get process handle") - return - } // Wait for the process to finish - WaitForSingleObject(processHandle, INFINITE) + WaitForSingleObject(execution.platformHandles.processInformation.hProcess, INFINITE) // Up to 10 characters because Windows process IDs are DWORDs (UInt32), whose max value is 10 digits. try writeFd.close() @@ -757,7 +746,7 @@ extension SubprocessWindowsTests { let resultPID = try #require( String(data: data, encoding: .utf8) ).trimmingCharacters(in: .whitespacesAndNewlines) - #expect("\(pid.value)" == resultPID) + #expect("\(execution.processIdentifier.value)" == resultPID) try readFd.close() } }