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 1 commit 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
1 commit
Select commit
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 @@ -101,7 +101,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
14 changes: 0 additions & 14 deletions Sources/Subprocess/Execution.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,13 @@ public struct Execution: Sendable {
public let processIdentifier: ProcessIdentifier

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

Choose a reason for hiding this comment

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

nit: unrelated, but you could delete consoleBehavior as well as nothing actually uses it.


init(
processIdentifier: ProcessIdentifier,
processInformation: PROCESS_INFORMATION,
consoleBehavior: PlatformOptions.ConsoleBehavior
) {
self.processIdentifier = processIdentifier
self.processInformation = processInformation
self.consoleBehavior = consoleBehavior
}
#else
Expand All @@ -54,17 +51,6 @@ public struct Execution: Sendable {
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.

/// 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
Loading