Introduce Swift Subprocess Proposal #397
Conversation
iCharlesHu
force-pushed
the
charles/subprocess-proposal
branch
from
0d6d94f to
233af5e
Compare
|
Updated based on community feedbacks. Thanks all! |
There was a problem hiding this comment.
This looks like a great basis for an API. Requesting changes because two crucial lifecycle things are not specified (from what I can tell):
- What happens under Swift Concurrency cancellation?
- Who manages the lifetime (i.e. who closes) file descriptors inherited into sub processes
| @available(tvOS, unavailable) | ||
| @available(watchOS, unavailable) | ||
| extension Subprocess { | ||
| public static func run( |
There was a problem hiding this comment.
@iCharlesHu This needs to specify the cancellation behaviour. For Swift Concurrency to work it's crucial that under task cancellation (taskGroup.cancelAll()) the async methods return quickly. For sub processes that will mean that it needs to get rid of the process which requires sending a signal -- likely SIGKILL. But this needs to be clearly specified.
There was a problem hiding this comment.
I mentioned this in the pitch as well and Charles said that he is going to change this to send a SIGKILL
| @available(watchOS, unavailable) | ||
| public struct InputMethod: Sendable, Hashable { | ||
| public static var noInput: Self | ||
| public static func readFrom(_ fd: FileDescriptor, closeWhenDone: Bool) -> Self |
There was a problem hiding this comment.
It's crucial here to specify who closes this file descriptor in the parent and precisely when and how. Please note that these file descriptors must usually be closed in the parent right after fork/posix_spawn. It is not acceptable to wait with the closing until after the process exits because that may lead to a deadlock. For example, the parent commonly inherits a pipe into the child. But the child can only see EOF on the pipe if there are no more writers left. Also: It's crucial to keep the number of open file descriptors low (macOS default limit is 256).
I would assume that Subprocess either always closes or has the ability to close file descriptors that it inherits into a child at the earliest possible point. But there doesn't seem to be API or docs around this (unless I missed it).
Proposals/NNNN-swift-subprocess.md
Outdated
|
|
||
| In addition to supporting the direct passing of `Sequence<UInt8>` and `AsyncSequence<UInt8>` as the standard input to the child process, `Subprocess` also provides a `Subprocess.InputMethod` type that includes two additional input options: | ||
| - `.noInput`: Specifies that the subprocess does not require any standard input. This is the default value. | ||
| - `.readFrom`: Specifies that the subprocess should read its standard input from a file descriptor provided by the developer. Subprocess will automatically close the file descriptor after the process exits if `closeWhenDone` is set to `true`. |
There was a problem hiding this comment.
After the process is done is too late, this should happen as soon as possible.
Proposals/NNNN-swift-subprocess.md
Outdated
|
|
||
| ## Automatic `Subprocess` Cancellation | ||
|
|
||
| Currently, `Subprocess` does not terminate the spawned process, even if the `Subprocess` instance goes out of scope. It would be beneficial to investigate whether it is more sensible to attempt terminating the spawned process when the `Subprocess` itself goes out of scope or when the parent task is canceled. |
There was a problem hiding this comment.
It really ought to terminate the process. Or else you'll deadlock super easily. Imagine a task group which reads chunks from the sub process's output. Under cancellation that AsyncSequence will stop being iterated. That however means that the sub process won't be able to write a lot because the pipe will run full (as the parent is no longer reading). And that means the sub process will never exit --> deadlock.
|
Updated for v3 |
There was a problem hiding this comment.
It looks to me that pending questions were answered in V3. I'd like to move this to formal review. @iCharlesHu will keep taking questions and improve the proposal as needed during review period.
|
|
||
| * Proposal: [SF-NNNN](NNNN-swift-subprocess.md) | ||
| * Authors: [Charles Hu](https://github.com/iCharlesHu) | ||
| * Review Manager: TBD |
There was a problem hiding this comment.
| * Review Manager: TBD | |
| * Review Manager: [Tina Liu](https://github.com/itingliu) |
| @@ -0,0 +1,878 @@ | |||
| # Introducing Swift Subprocess | |||
|
|
|||
| * Proposal: [SF-NNNN](NNNN-swift-subprocess.md) | |||
There was a problem hiding this comment.
| * Proposal: [SF-NNNN](NNNN-swift-subprocess.md) | |
| * Proposal: [SF-0007](0007-swift-subprocess.md) |
| * Proposal: [SF-NNNN](NNNN-swift-subprocess.md) | ||
| * Authors: [Charles Hu](https://github.com/iCharlesHu) | ||
| * Review Manager: TBD | ||
| * Status: **Draft** |
There was a problem hiding this comment.
| * Status: **Draft** | |
| * Status: **Active Review: Feb 28, 2024...Mar 06, 2024** |
This PR introduces a proposal for
Subprocess.