-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: main
Are you sure you want to change the base?
Conversation
Resolves #111 |
|
// MARK: - ProcesIdentifier | ||
|
||
/// A platform independent identifier for a Subprocess. | ||
public struct ProcessIdentifier: Sendable, Hashable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worthwhile to make this a protocol given the repetition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on how would we use this protocol? The reason it's repeated is because on different platforms we have different sets of fields. I don't think having a protocol would solve this problem because we'd still need to offer different concrete types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File this one under premature optimization but a protocol might help if the shared, non-platform-specific code started relying on the existence of methods or attributes of ProcessIdentifier
. I took a look and don't see any at the moment beyond description
.
If there are more expectations requiring the various ProcessIdentifier
definitions to stay in sync, it might be helpful to introduce a protocol, not because any given platform needs more than one concrete type, but because the protocol will serve as a contract to keep the implementations in sync with the expectations of the shared code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see your point. IMO that's not a great use case here because there's no place we want to write some ProcessIdentifierProtocol
as opposed to the concrete type. A protocol would help ensure all variants of ProcessIdentifier
to have the value
field but that's about it. As part of execution we still want to have the concrete version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about combining that with a typealias
, e.g.
protocol SomeSubprocessConstruct {
func doSomething()
...
}
// SomeFile+Windows.swift
#if canImport(WinSDK)
struct WindowsSubprocessConstruct: SomeSubprocessConstruct {
func doSomething()
...
}
typealias SubprocessConstruct = WindowsSubprocessConstruct
#endif
// SomeFile+Darwin.swift
#if canImport(Darwin)
struct DarwinSubprocessConstruct: SomeSubprocessConstruct {
func doSomething()
...
}
typealias SubprocessConstruct = DarwinSubprocessConstruct
#endif
// Some shared code
struct Execution {
private var construct: SubprocessConstruct
...
if construct.doSomething() { ... }
}
You know this domain way better than I do but as a naive reader, it feels to me like there should be a clearer separation between core logic and platform-specific details. Right now there's a bit of a mix: there are platform-specific files with similarly named methods as well as shared code with #if canImport()
.
Given there's so much platform-specific code, it'd be great to better separate it out and keep platform-specific details more sequestered away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given there's so much platform-specific code, it'd be great to better separate it out and keep platform-specific details more sequestered away.
I 100% agree with this statement. However, could you elaborate on how typealias
would help in this case? Because currently we have
// SomeFile+Windows.swift
#if canImport(WinSDK)
struct ProcessIdentifier {
func doSomething()
// Additional stuff
}
#endif
// SomeFile+Darwin.swift
#if canImport(Darwin)
struct ProcessIdentifier {
func doSomething()
// additional stuff
}
#endif
// Some shared code
struct Execution {
private var construct: ProcessIdentifier
...
if construct.doSomething() { ... }
}
To me having a dedicated type name (which unfortunately is also an API change) doesn't necessarily make the code clearer because 1) the platform-specific ProcessIdentifier
is already separated to different files; and 2) because of the typealias, you still ended up using the generic name SubprocessConstruct
at the call site which makes it the same as just using the type directly. To me with the suggestion you basically get the same code structure as today, but now there are additional type-specific names only used once in typealias typealias SubprocessConstruct = DarwinSubprocessConstruct
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protocol
+ typealias
approach would provide a contract that all platform-specific implementations follow when interfacing with shared code. This would make it easier to ensure all platform-specific code was being updated when that contract changes as you work on shared code. As it stands today, one has to remember to update any platform-specific types that are shared by virtue of being spelled the same way. A protocol
solidifies this idea.
A pattern like this could help keep all #if canImport
statements in platform-specific files (e.g. out of Input.swift, Output.swift, API.swift, AsyncBufferSequence.swift, etc.), which would be a good way to measure whether the platform-specific code has been abstracted enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative idea, you could define a protocol and retroactively make the ProcessIdentifier type conform to that protocol in the SubprocessTests target. That way, it serves as a type system level "test" in the way @cthielen is advocating for, but without adding complexity to the production code / library interface.
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally left it there because I wasn't sure if it is useful... I agree with you that it seems not that useful. I'll remove it in a dedicate PR since it's an API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, it's not an API change since it has internal
visibility.
-1 | ||
) | ||
if eventCount < 0 { | ||
if errno == EINTR || errno == EAGAIN { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: it might be worth introducing a helper function to handle EINTR/EAGAIN since it's such a common pattern throughout this codebase; see https://github.com/apple/swift-system/blob/6ee9a58c36ad98f4bd917a64d153dd211512e65d/Sources/System/Util.swift#L27 for example.
This is not a bug in the existing implementation. It is a bug in the POSIX specification (and a bug in the program.) |
@@ -664,6 +504,10 @@ int _subprocess_fork_exec( | |||
// If we reached this point, something went wrong | |||
write_error_and_exit; | |||
} else { | |||
int _pidfd = _pidfd_open(childPid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use clone
+ CLONE_PIDFD
(Linux 5.2) instead of fork
+ pidfd_open
? Like FreeBSD's pdfork
, this avoids races since combining the latter two functions is not atomic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Unfortunately it still needs to be conditionalized due to kernel version requirements.
That may be true, but the implementation Charles is proposing here is more defensive against other parts of the program misbehaving, which seems like a good thing. Including scenarios where zombies are being reaped correctly throughout the entire program, but maybe the body of one particular |
// - musl 1.1.24 (October 2019) | ||
// - FreeBSD 13.1 (May 2022) | ||
// - Android 14 (API level 34) (October 2023) | ||
return posix_spawn_file_actions_addchdir_np(file_actions, path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will emit a deprecation warning as of *OS 26 since the standardized version has been added.
// MARK: - ProcessIdentifier | ||
|
||
/// A platform independent identifier for a Subprocess. | ||
public struct ProcessIdentifier: Sendable, Hashable { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 inclose()
other thanEINTR
/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.
There was a problem hiding this comment.
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.
Added fallback implementation that uses |
1380d11
to
5c17f7b
Compare
5c17f7b
to
5e80453
Compare
… to create a temporary user
5e80453
to
b628c68
Compare
The current process monitoring code for Linux has a flaw that makes it susceptible to infinite hangs under specific conditions:
Subprocess
.This is because currently, we rely on running
waitid()
withP_ALL
andWNOWAIT
in an infinite loop to detect possible child process state transitions. However, we don’t reap the child process (by specifyingWNOWAIT
) unless we (Subprocess) actually spawned the process.Here’s a simplified pseudo-code to illustrate the issue:
With this setup, if there are zombie children in the process table without reaping,
waitid(P_ALL)
will repeatedly return the same (non-Subprocess-spawned) PID with every call, causing an infinite loop.You can observe this behavior with the following sample code:
After running this example, you’ll notice that the parent process seems to be stuck, and the “cat finished” message is never printed. This is because the parent process never calls
waitid
on theecho
call, leaving it in the process table. Consequently, the monitor thread runs in an infinite loop.While some may argue that this is not a bug in
Subprocess
, but rather an issue with the parent code, since the POSIX standard mandates that the process spawning child process must reap the child process viawaitid
. However, Subprocess should still not hang due to someone else’s bug.To resolve this issue, switch to a Linux-specific process monitoring method by creating and observing the process file descriptor (pidfd) using epoll. This approach is similar to the epoll implementation introduced in #117, with the only difference being that we’re polling pidfd instead of a regular file descriptor.
As part of this change, I also unified the “process handle” design to make it easier to expose process handles to clients later (after the 1.0 release, as requested by #101). We chose to use
ProcessIdentifier
to host platform-specific process file descriptors and process handles because it perfectly aligns with the original use case. To ensure flexibility, we opted for a concreteProcessIdentifier
type instead of just a number, allowing us to add more information if necessary.