Skip to content

Commit c8c66e1

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 eba918b commit c8c66e1

File tree

4 files changed

+35
-21
lines changed

4 files changed

+35
-21
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: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -779,20 +779,20 @@ 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
)
787787
var status: Int32 = 0
788-
waitpid(pid.value, &status, 0)
788+
waitpid(execution.processIdentifier.value, &status, 0)
789789
#expect(_was_process_exited(status) > 0)
790790
try writeFd.close()
791791
let data = try await readFd.readUntilEOF(upToLength: 10)
792792
let resultPID = try #require(
793793
String(data: Data(data), encoding: .utf8)
794794
).trimmingCharacters(in: .whitespacesAndNewlines)
795-
#expect("\(pid.value)" == resultPID)
795+
#expect("\(execution.processIdentifier.value)" == resultPID)
796796
try readFd.close()
797797
}
798798

Tests/SubprocessTests/SubprocessTests+Windows.swift

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -714,34 +714,23 @@ extension SubprocessWindowsTests {
714714
DWORD(HANDLE_FLAG_INHERIT),
715715
0
716716
)
717-
let pid = try Subprocess.runDetached(
717+
let execution = try Subprocess.runDetached(
718718
.path("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"),
719719
arguments: [
720720
"-Command", "Write-Host $PID",
721721
],
722722
output: writeFd
723723
)
724-
// Wait for process to finish
725-
guard
726-
let processHandle = OpenProcess(
727-
DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
728-
false,
729-
pid.value
730-
)
731-
else {
732-
Issue.record("Failed to get process handle")
733-
return
734-
}
735724

736725
// Wait for the process to finish
737-
WaitForSingleObject(processHandle, INFINITE)
726+
WaitForSingleObject(execution.platformHandles.processInformation.hProcess, INFINITE)
738727

739728
// Up to 10 characters because Windows process IDs are DWORDs (UInt32), whose max value is 10 digits.
740729
let data = try await readFd.readUntilEOF(upToLength: 10)
741730
let resultPID = try #require(
742731
String(data: data, encoding: .utf8)
743732
).trimmingCharacters(in: .whitespacesAndNewlines)
744-
#expect("\(pid.value)" == resultPID)
733+
#expect("\(execution.processIdentifier.value)" == resultPID)
745734
try readFd.close()
746735
try writeFd.close()
747736
}

0 commit comments

Comments
 (0)