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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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
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 {

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.

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