Skip to content

Introduce process file descriptor (pidfd) based process monitoring for Linux #125

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.

Sign up for GitHub

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 7 commits into
base: main
Choose a base branch
from
Open

Introduce process file descriptor (pidfd) based process monitoring for Linux #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
165eca8
Introduce process file descriptor (pidfd) based process monitoring fo…
iCharlesHu Jul 15, 2025
199eb72
Use signal handler for process state monitoring on Linux kernel lower…
iCharlesHu Jul 22, 2025
086c8d8
Disable testPlatformOptionsRunAsUser on CI if we don't have privilege…
iCharlesHu Jul 24, 2025
bbb5432
Remove unused consoleBehavior from Execution
iCharlesHu Jul 25, 2025
fbc0f2c
Add PlatformConformance in tests to ensure platform specifc types fol…
iCharlesHu Jul 28, 2025
89e5d2f
Disable testPlatformOptionsRunAsUser until we can resolve CI filure i…
iCharlesHu Jul 28, 2025
a2de33f
Prob whether waitid supports P_PIDFD instead of checking for Linux ke…
iCharlesHu Jul 29, 2025
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
2 changes: 1 addition & 1 deletion Sources/Subprocess/API.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ public func runDetached(
errorPipe: try processError.createPipe()
).execution
}
execution.release()
execution.processIdentifier.close()
return execution.processIdentifier
}

7 changes: 6 additions & 1 deletion Sources/Subprocess/Configuration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,12 @@ public struct Configuration: Sendable {
// even if `body` throws, and we are not leaving zombie processes in the
// process table which will cause the process termination monitoring thread
// to effectively hang due to the pid never being awaited
let terminationStatus = try await Subprocess.monitorProcessTermination(forExecution: _spawnResult.execution)
let terminationStatus = try await Subprocess.monitorProcessTermination(
for: execution.processIdentifier
)

// Close process file descriptor now we finished monitoring
execution.processIdentifier.close()

return try ExecutionResult(terminationStatus: terminationStatus, value: result.get())
}
Expand Down
26 changes: 0 additions & 26 deletions Sources/Subprocess/Execution.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,37 +34,11 @@ public struct Execution: Sendable {
/// The process identifier of the current execution
public let processIdentifier: ProcessIdentifier

#if os(Windows)
internal nonisolated(unsafe) let processInformation: PROCESS_INFORMATION
internal let consoleBehavior: PlatformOptions.ConsoleBehavior

init(
processIdentifier: ProcessIdentifier,
processInformation: PROCESS_INFORMATION,
consoleBehavior: PlatformOptions.ConsoleBehavior
) {
self.processIdentifier = processIdentifier
self.processInformation = processInformation
self.consoleBehavior = consoleBehavior
}
#else
init(
processIdentifier: ProcessIdentifier
) {
self.processIdentifier = processIdentifier
}
#endif // os(Windows)

internal func release() {
#if os(Windows)
guard CloseHandle(processInformation.hThread) else {
fatalError("Failed to close thread HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))")
}
guard CloseHandle(processInformation.hProcess) else {
fatalError("Failed to close process HANDLE: \(SubprocessError.UnderlyingError(rawValue: GetLastError()))")
}
#endif
}
}

// MARK: - Output Capture
Expand Down
27 changes: 21 additions & 6 deletions Sources/Subprocess/Platforms/Subprocess+Darwin.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,26 +477,41 @@ extension Configuration {
}
}

// Special keys used in Error's user dictionary
extension String {
static let debugDescriptionErrorKey = "NSDebugDescription"
// MARK: - ProcessIdentifier

/// A platform independent identifier for a Subprocess.
public struct ProcessIdentifier: Sendable, Hashable {
Copy link

Choose a reason for hiding this comment

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

Could you make this type move-only and incorporate the close() operation into deinit?

Copy link
Member

Choose a reason for hiding this comment

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

Closing might involve closing FDs right? Which might be an asynchronous and throwing operation.

Copy link

Choose a reason for hiding this comment

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

That close() can fail at all is an unfortunate weird corner of POSIX that I personally tend to ignore, because a failure in close() other than EINTR/EAGAIN is basically non-recoverable. What are you even supposed to do? What can a user do to fix the problem? Generally nothing.

So I just about always just drop a close() failure on the floor. </hottake>

(As for asynchronous, it's a blocking operation in userland but it can't fail to make forward progress in this case because there's no network I/O involved unless we're doing something really wonky.)

Copy link
Contributor

Choose a reason for hiding this comment

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

We've had this discussion in another place (here?). We ended up not calling close, but asserting that close has already been called in deinit.

But it does seem like a design people are going to reach for repeatedly. I wonder if we can put our thought process down somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

That close() can fail at all is an unfortunate weird corner of POSIX that I personally tend to ignore, because a failure in close() other than EINTR/EAGAIN is basically non-recoverable. What are you even supposed to do? What can a user do to fix the problem? Generally nothing.

I agree that it is a weirdness; nevertheless, we need to handle it and most likely surface it to the user. We shouldn't just swallow those errors.

(As for asynchronous, it's a blocking operation in userland but it can't fail to make forward progress in this case because there's no network I/O involved unless we're doing something really wonky.)

This is not entirely true. If you are using io_uring you can asynchronously listen for the subprocess termination with pidfd and signalfd via io_uring. We need to account for changes in the underlying I/O system where closing can become asynchronous otherwise we will lock ourselves in a corner API-wise.

The only pattern that keeps us flexible is a with-style based approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

ProcessIdentifier is only ever provided as a parameter to a closure called within a with-style function, except for runDetached. If we remove the latter from the API, there's no place where ProcessIdentifier needs to be used as a handle that needs to be closed and therefore no reason it needs to be responsible for lifetime management.

/// The platform specific process identifier value
public let value: pid_t

public init(value: pid_t) {
self.value = value
}

internal func close() { /* No-op on Darwin */ }
}

extension ProcessIdentifier: CustomStringConvertible, CustomDebugStringConvertible {
public var description: String { "\(self.value)" }

public var debugDescription: String { "\(self.value)" }
}

// MARK: - Process Monitoring
@Sendable
internal func monitorProcessTermination(
forExecution execution: Execution
for processIdentifier: ProcessIdentifier
) async throws -> TerminationStatus {
return try await withCheckedThrowingContinuation { continuation in
let source = DispatchSource.makeProcessSource(
identifier: execution.processIdentifier.value,
identifier: processIdentifier.value,
eventMask: [.exit],
queue: .global()
)
source.setEventHandler {
source.cancel()
var siginfo = siginfo_t()
let rc = waitid(P_PID, id_t(execution.processIdentifier.value), &siginfo, WEXITED)
let rc = waitid(P_PID, id_t(processIdentifier.value), &siginfo, WEXITED)
guard rc == 0 else {
continuation.resume(
throwing: SubprocessError(
Expand Down
Loading