Skip to content

Commit 14c7b48

Browse files
committed
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
1 parent e0559da commit 14c7b48

File tree

4 files changed

+34
-19
lines changed

4 files changed

+34
-19
lines changed

Sources/Subprocess/API.swift

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -614,7 +614,7 @@ public func runDetached(
614614
input: FileDescriptor? = nil,
615615
output: FileDescriptor? = nil,
616616
error: FileDescriptor? = nil
617-
) throws -> ProcessIdentifier {
617+
) throws -> ProcessHandle {
618618
let config: Configuration = Configuration(
619619
executable: executable,
620620
arguments: arguments,
@@ -643,7 +643,7 @@ public func runDetached(
643643
input: FileDescriptor? = nil,
644644
output: FileDescriptor? = nil,
645645
error: FileDescriptor? = nil
646-
) throws -> ProcessIdentifier {
646+
) throws -> ProcessHandle {
647647
let execution: Execution
648648
switch (input, output, error) {
649649
case (.none, .none, .none):
@@ -753,7 +753,6 @@ public func runDetached(
753753
errorPipe: try processError.createPipe()
754754
).execution
755755
}
756-
execution.release()
757-
return execution.processIdentifier
756+
return ProcessHandle(execution: execution)
758757
}
759758

Sources/Subprocess/Execution.swift

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,32 @@ public struct Execution: Sendable {
5050
}
5151
}
5252

53+
/// An object that represents a subprocess that has been
54+
/// executed without monitoring the state of the subprocess
55+
/// nor waiting until it exits. You can use this object to control
56+
/// the process using platform-native APIs.
57+
public struct ProcessHandle: Sendable, ~Copyable {
58+
/// The process identifier of the current execution
59+
public var processIdentifier: ProcessIdentifier {
60+
execution.processIdentifier
61+
}
62+
63+
/// The collection of platform-specific handles of the current execution.
64+
public var platformHandles: PlatformHandles {
65+
execution.platformHandles
66+
}
67+
68+
private let execution: Execution
69+
70+
init(execution: Execution) {
71+
self.execution = execution
72+
}
73+
74+
deinit {
75+
execution.release()
76+
}
77+
}
78+
5379
// MARK: - Output Capture
5480
internal enum OutputCapturingState<Output: Sendable, Error: Sendable>: Sendable {
5581
case standardOutputCaptured(Output)

Tests/SubprocessTests/SubprocessTests+Unix.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -779,11 +779,12 @@ extension SubprocessUnixTests {
779779
extension SubprocessUnixTests {
780780
@Test func testRunDetached() async throws {
781781
let (readFd, writeFd) = try FileDescriptor.pipe()
782-
let pid = try runDetached(
782+
let execution = try runDetached(
783783
.path("/bin/sh"),
784784
arguments: ["-c", "echo $$"],
785785
output: writeFd
786786
)
787+
let pid = execution.processIdentifier
787788
var status: Int32 = 0
788789
waitpid(pid.value, &status, 0)
789790
#expect(_was_process_exited(status) > 0)

Tests/SubprocessTests/SubprocessTests+Windows.swift

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -729,35 +729,24 @@ extension SubprocessWindowsTests {
729729
DWORD(HANDLE_FLAG_INHERIT),
730730
0
731731
)
732-
let pid = try Subprocess.runDetached(
732+
let execution = try Subprocess.runDetached(
733733
.path("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"),
734734
arguments: [
735735
"-Command", "Write-Host $PID",
736736
],
737737
output: writeFd
738738
)
739-
// Wait for process to finish
740-
guard
741-
let processHandle = OpenProcess(
742-
DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
743-
false,
744-
pid.value
745-
)
746-
else {
747-
Issue.record("Failed to get process handle")
748-
return
749-
}
750739

751740
// Wait for the process to finish
752-
WaitForSingleObject(processHandle, INFINITE)
741+
WaitForSingleObject(execution.platformHandles.processInformation.hProcess, INFINITE)
753742

754743
// Up to 10 characters because Windows process IDs are DWORDs (UInt32), whose max value is 10 digits.
755744
try writeFd.close()
756745
let data = try await readFd.readUntilEOF(upToLength: 10)
757746
let resultPID = try #require(
758747
String(data: data, encoding: .utf8)
759748
).trimmingCharacters(in: .whitespacesAndNewlines)
760-
#expect("\(pid.value)" == resultPID)
749+
#expect("\(execution.processIdentifier.value)" == resultPID)
761750
try readFd.close()
762751
}
763752
}

0 commit comments

Comments
 (0)