[Review] SF-0007: Introducing Swift Subprocess

Related Projects Foundation
1

Hello Swift community,

The review of SF-0007: Introducing Swift Subprocess begins now and runs through Thursday, Mar 06, 2024.

All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager by email or DM. When contacting the review manager directly, please put "SF-0007" in the subject line.

Trying it out

If you'd like to try this proposal out, you can check out the pull request .

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Foundation. When writing your review, here are some questions you might want to
answer in your review:

  • What is your evaluation of the proposal?
  • If you have used other languages or libraries with a similar
    feature, how do you feel that this proposal compares to those?

More information about Foundation review process is available at

https://github.com/apple/swift-foundation/blob/main/Evolution.md

Thank you,

Tina Liu
Review Manager

12 Likes
2

Mostly looks good, but I'd like to bring up again the issue of what happens to file descriptors open in the parent process when the child process is launched. Current on macOS, these are closed via POSIX_SPAWN_CLOEXEC_DEFAULT, and on Linux a best-effort attempt is made that isn't threadsafe.

It seems that this implementation does not bring over the flawed emulation from Process, is this correct?

If so, it would be great to be clear in the proposal what happens to such open file descriptors: are they closed on macOS but left open on Linux? In this case the default configuration is really more like platformDefault as the behavior can be meaningfully different depending on platform.

6 Likes
3

I only see Linux and Windows mentioned once, but it sounds like they would be supported out of the box. Is that true?

4

I read the proposal and I’m impressed with the quality of the API this provides. Great improvement!

My main experience with comparable functionality is Process. Which gets the job done for simple cases. Luckily I’ve been able to avoid having to use it for more complex cases.

One question though: does this need to be part of Foundation? I definitely support this becoming part of Foundation, but could it also be its own package? That might make it easier to (gradually) adopt over time/evolve it based on usage in the field.

5

I'm actually in the midst of writing a non-trivial case of subprocess execution, whereby I need to keep the subprocess alive indefinitely and interact with it at various times from various parts of my program.

I reviewed every relevant package I could find on the Swift Package Index, ultimately concluding that Foundation's Process is my best bet. The only other interesting-looking option I found was SwiftCommand, which improves Process's ergonomics slightly. Everything else out there is "old fashioned" (no async APIs) which is a non-starter for me given the nature of this problem domain.

Looking at the proposal for this Subprocess, there's a few things which seem like they'd be significant problems for this use case. For example, it appears I can only interact with a subprocess through the run variant that takes a closure, and only while that closure is live. Which means I have to keep a Task somewhere running that closure indefinitely, and somehow communicate from inside that closure to the rest of my program. I also have to somehow make that work even when the subprocess dies and I need to restart it from scratch. This all seems technically doable, but a lot more work than what I'm currently doing with Process, which is simply using a lock (for mutual exclusion) and interacting with the long-lived instance that's stored in a variable (inside an actor, or a class, or as a global). It's easy to handle errors such as subprocess termination by just re-setting the variable to a new Process instance - this can be done automatically without callers having a clue.

The one potential saving grace is that I haven't actually been able to try writing real code against this Subprocess API yet, so maybe I'm overlooking something.

There are definitely some awkward aspects to the existing Process, like:

  • A lack of convenience initialisers to succinctly set up the desired state.
  • No way to search PATH for a command.
  • The inability to send signals other than SIGTERM.
  • An inability to read from the subprocess without blocking (contrary to its name and obviously intended purpose, availableData and similar block if there's no data available :face_with_raised_eyebrow:).
  • The ability to do pipeline-common operations like read up until a sentinel string.
  • The ability to read from both stdout and stderr simultaneously.
    • Incidentally, the SwiftCommand package tries to add this but fails to indicate which pipe a given piece of data came from, making it useless for most purposes (although at least it avoids deadlock).
  • A lack of convenience methods for writing strings (it's all based on FileHandles which only take DataProtocols).
  • No [built-in] way to run the subprocess in a sandbox, or otherwise influence its environment for security purposes.

This proposal addresses some but not all of these. It'd be great to see it cover all of them.

I'm also wondering what happens if you don't immediately read from the subprocess when using the redirect method, given there's no option to specify a buffer size. When does the subprocess end up blocked on its writes (to stdout or stderr)?

Also, why is stderr discarded by default with many of the run methods? That seems like it's inviting bad behaviour and unwitting design flaws.

Why is there no inherit option for stdin, stdout, & stderr?

Overall I think the proposed API looks alright for simplistic cases - mainly run-once-and-read-output situations, particularly for background processes (that aren't visible to the parent process's user). For interactive use of subprocesses, or shell-like usage where everything shares the same tty by default, I'm not sure it's a net improvement over Process. Better in some ways, but worse in others.

6 Likes
6

As @LiarPrincess pointed out in the pitch thread, waiting for exit and reading outputs (stdout or stderr) are inherently coupled, due to finite buffers that may be (and often are) involved. The API currently exposes many deadlock scenarios by having them be separate.

It's important to be able to read from stdout & stderr as a logically single stream. Otherwise, you might be awaiting on stdout while the subprocess has communicated via stderr, or vice versa, and deadlock.

It's also important to not await on subprocesses termination until stdout & stderr are already closed. Perhaps wait shouldn't be available on Subprocess, but rather something you get access to by closing stdout & stderr? e.g. closing them (both) returns a token object which provides the wait method, along with whatever other handful of functions still make sense at that time.

You can naturally read to EOF on them (stdout & stderr), or force them closed, but either way until they're closed it's not safe to wait for subprocess termination.

3 Likes
7

Unfortunately It sounds like Subprocess isn't designed to support your specific use case. In the pitch thread I mentioned I wasn't sure if we should terminate the child task for this very reason. The majority of the commenter vouched for Subprocess to kill the child process when the parent task is cancelled, which unfortunately means it will no longer support this particular use case.

8

A minor nit: Because Subprocess.run takes an enum as its first argument, I don’t think it needs an argument label. IMO, spawning a process should be as succinct as Subprocess.run("/usr/bin/git")., but I understand that this would require doubling the number of overloads into versions that take a FilePath vs. a String. Can we at least drop the first argument label and let the enum case do the talking?

4 Likes
9

IMO this is a user error, with the user being either the programmer or the subprocess itself (since it misuses stdout for stderr or vise versa). I agree it's nice to have but today you can read the standard input and error in parallel tasks if you determine the subprocess you are calling is misusing stdout and stderr.

10

I think I've seen FileDescriptor-like types used in examples where the descriptor's close() method "consumes" ("moves"?) the descriptor. (Not sure about the correct terminology for this.)

I don't know if the FileDescriptor in the proposal works like that -- or if we would maybe want it to work like that at some point.

It seems like the proposed readFrom(_:closeWhenDone:) and writeTo(_:closeWhenDone:) would not allow that. When closeWhenDone is true the descriptor would need to be consumed. And when it's false the descriptor would need to be borrowed.

Any thoughts on that?

2 Likes
11

One possibility for supporting both semantics was outlined here: [Pitch] Swift Subprocess - #44 by wadetregaskis

1 Like
12

How is it user error? Are you saying it's an implementation error to be capable of emitting to both stdout and stderr within the same program?!

Whether an operation succeeds may depend on practically anything. The inputs it's given, the environment it's running in, the versions of all the programs involved, the state of the internet, the phase of the moon, etc. In general it's impossible to predict which pipe the subprocess is going to use, because you can't know with certainty if every given operation is going to succeed or fail.

Having to spawn multiple parallel Tasks to read from all the pipes, and then merge all that back together into a logically serial stream, is certainly doable but that's a lot of work and exactly the kind of thing that's optimal for a library to handle, since many use-cases require it.

6 Likes
13

No. I'm saying the fact that the subprocess communicated something to standard error when it's supposed to do that through standard output is an user error -- not an user error of the current developer but an user error of the subprocess' author.

I understand that having a single combined stream is useful in some use cases. I'm not convinced this API is required for the initial version of Subprocess (there's nothing in the current design that prohibits us from adding it later).

14

I still don't understand. How is the caller supposed to know, in advance, if the subprocess is going to experience an error or not? And if it did know that the subprocess will error out, why would it invoke the subprocess at all?

To be clear, the problem I see - and it's a problem with Process and all 3rd party equivalents today, in Swift - is that the following code looks at a glance like it's fine but is fundamentally wrong:

let subprocess = Process(…) // Or equivalent.

for line in subprocess.standardOutput.lines {
    …
}

Until the subprocess writes something to stdout (or closes stdout) that will hang indefinitely. So if the subprocess instead writes to stderr (and doesn't exit, or the stderr pipe buffer fills up), not only will the program hang permanently but you will never see that error output (making it hard to debug the problem).

AsyncBytes provides no way to check if there's input already available - the only way to use it is to try reading, and hang indefinitely until there's something available. And even if it did, this is the socket polling problem. The solution is ultimately the same - you need a way to read from either of the pipes, whichever one happens to have data available first (and you need to know which pipe it was, to help determine what to do with it).

If the intent really is to support only one-shot subprocess execution, not interactive use, then I suppose this is less important. Not unimportant, though - it's still possible to deadlock if one of the two pipes fills up and is never read by the parent process (because the parent is expecting to read the other pipe to conclusion first).

7 Likes
15

Tangentially, the proposal doesn't actually provide any API for reading more than individual bytes (as Foundation's Process does). It would be nice if it did. Though probably this should be a general facility on AsyncSequence. Does the standard lib or Foundation already provide that? I don't see any in the documentation, but it's hard to be sure without Xcode in front of me.

16

Ahh okay I see what you are saying now. Does merge not work for this particular case? Do you need something specific from the Subprocess itself?

Yes this is the intent for this first iteration. As you can see this API has very wide use cases from scripting to server side development and many use cases have conflicting requirements (for example: for scripting you probably want Subprocess to lookup executable path for you, but this might not acceptable in server side development due to security concerns). Therefore I made the conscious design decision to employ progressive disclosure as much as possible (btw this is also why this API has "escape hatches"... to support the really advanced usages). The intended "easy to use" case for this API is one-shot subprocess execution, and I'd say this is true for NSTask as well. I was hoping the new name, **Sub**process, communicates the intention here that we are really launching a new process as a child process of the existing process, and we are generally expecting the child process to finish before the parent process and get some data out of it. Once you drop down to the "closure approach", I consider it an advance usage where you want to have precise control of the subprocess, it is then therefore up to the user to correctly implement the body (by reading standard output and standard error in parallel if necessary, for example).

With all that being said, I'm definitely planning on keep improving Subprocess in the future, and we will start bringing more and more use cases to the "easy to use" side. Until then for the sake of scope control I think we have the right balance for the first iteration.

17

It can be made to work, yes, whether with merge or otherwise. My concern is that:

  1. This is fundamental functionality so it should be built-in, rather than requiring additional packages.
  2. This should probably be the only way to read from subprocesses's stdout & stderr, as it's generally unsafe any other way.

Note that if for some reason the user elects to redirect stdout xor stderr, it still doesn't hurt to have a unified API (as long as it's named and documented appropriately).

Iterative development is fine and wise, but I'm concerned that the current design doesn't have a good path towards more advanced uses, beyond one-shots.

It's worth nothing that there's a philosophical presumption here about the purpose of Foundation, as to whether it should be broadly useful tools or tools that are used broadly. I'm not sure how the Foundation team actually see it, but I know which approach I'd prefer.

3 Likes
18

I agree. The run API reads strangely to me. For example,

let result = try await Subprocess.run(executing: .at("/path/to/binary"), …)

almost sounds like the process is running with PWD=/path/to/binary. Perhaps changing the argument label to executable would work:

try await Subprocess.run(executable: .at("/usr/bin/git"), …)
try await Subprocess.run(executable: .named("git"), …)

Otherwise, I’m in favor of the proposal. It’s a good start to a more ergonomic API.

8 Likes
19

Merge doesn’t seem like the right answer either. A process that produces formatted output on stdout will not usually use the same format for its error messages. I get that it’s better than deadlocking, and you can’t use “hasn’t written anything to stdout yet” as a signal that you’re in a deadlock, but it won’t result in more correct programs either. It is less bad than a deadlock though.

Maybe the answer is “filling up stderr while blocking on stdout”, very specifically, is an error that gets thrown through stdout. The reverse is much less likely, though not impossible, and I suspect many uses of Subprocess will want stderr to be passthrough, merged, or ignored anyway. It’s only when you have two capture streams that can be driven independently that you can get this particular deadlock.

2 Likes
20

Right - merge alone is probably insufficient. But with a little more work you can make it usable, e.g. by maping the two input streams into a common tuple type, like (isError: Bool, UInt8).

To be clear, I'm not happy about this whole problem domain in the sense that I don't like having to play fragile games of reading strings from multiple pipes just to tell if an operation worked or not. But unfortunately that's the nature of many things that you are forced to run as subprocesses (if their functionality were available through a library or native Swift alternative, I'd surely use that instead).

2 Likes