[Review, 2nd] SF-0007: Subprocess

Related Projects Foundation
1

Hello community,

The second review of SF-0007: Introducing Swift Subprocess begins now and runs through Thursday, Dec 19th, 2024.

Summary of the first review

There are multiple positive comments about the thoughts and efforts put into this proposal, and that they see this as a good alternative to the existing Process API.

There was also a lot of discussion. Notable areas where we'd like the author to clarify and address includes the following:

  • Clarifying the problem scope: There were questions about how to manage a child process that outlives the parent process, and how to pipe output from one process to another.
  • More discussion or reconsideration about input and output: There were still some open questions about whether RedirectedOutputMethod and StandardInputWriter would block parent process.
  • Platform specific API: Some API need to be refined to match platform specific behaviors such as how file descriptors opened in the parent process are handled on Linux.
  • Naming inconsistency and protocol requirement inconsistency

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

Check out the package.

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

8 Likes
2

One minor thing: what do you think about making the executable type ExpressibleByStringLiteral, with it producing a .named value?

That would turn:

let gitResult = try await Subprocess.run(   // <- 0
    .named("git"),                          // <- 1
    arguments: ["diff", "--name-only"]
)

In to:

let gitResult = try await Subprocess.run(   // <- 0
    "git",                                  // <- 1
    arguments: ["diff", "--name-only"]
)

This would only apply to string literals, not string values, and if you did write a path as a literal (e.g. Subprocess.run("/bin/sh", arguments: [...])), I think that would still work, wouldn't it?

3 Likes
3

One minor thing: what do you think about making the executable type ExpressibleByStringLiteral , with it producing a .named value?

In my opinion, omitting the label would make the API more confusing. We initially introduced the concrete Executable type for security reasons. Sometimes, developers do not want Subprocess to check whether a given String actually points to a valid executable before using it (time of check vs time of use error). This is where .at() comes in. Executable forces developers to choose between the two modes: looking up the full path or using the path provided. These labels clearly indicate the mode being used. It did not feel right to remove the label for one mode because one mode is not “more supported” than others. You can equally make the case to drop the label for the at() mode as well.

6 Likes
4

Subprocess.run executes immediately though, doesn't it? Also, is this likely for string literals?

Perhaps, if not adding a protocol conformance to the type, an overload of Subprocess.run could be added which takes a StaticString.

5

I like where this is heading … except I find the .run(...) and runDetached(...) methods exceptionally distasteful. If they ever need to be expanded, then they have to be deprecated and replaced with new versions.

Instead, it would be nice to have a Subprocess.Configuration struct that encapsulates all those parameters, so that the .run and .runDetached methods just take those. That would also make it easier to come up with templates for spawning subprocesses (as would be common for apps that wrap command line tools, especially git), because the code could grab the default configuration for the command, tweak the .arguments parameter, and then run it.

7 Likes
6

Subprocess.run executes immediately though, doesn't it? Also, is this likely for string literals?

The security concern was the fact that "checking whether the given string is a valid path to executable" and "call posix_spawn/fork/exec" are two separate steps, which would always introduce the possibility of some bad actor manipulating the path on disk between the two steps. When you specify .at(), Subprocess uses the value directly without checking, thus eliminating the first step.

Executable was designed to make developers explicitly choose between "checking" and "not checking".

7

Subprocess already supports configuration based run:

public static func run<R>(
    _ configuration: Configuration,
    output: RedirectedOutputMethod = .redirectToSequence,
    error: RedirectedOutputMethod = .redirectToSequence,
     _ body: (sending @escaping (Subprocess, StandardInputWriter) async throws -> R)
) async throws -> ExecutionResult<R>

I'll add an overload for runDetached as well.

3 Likes
8

I’ve written a similar library for myself, but one problem I ran into is that Ctrl-C doesn’t interrupt the child process. Does this library handle that? If not, is there a way to at least make it easy to implement in my command-line scripts?

9

The ServiceLifecycle library can handle this for you, see the "Graceful Shutdown" section for more details, gracefulShutdownSignals parameter of ServiceGroup configuration in particular.

1 Like
10

Is that easy to use for command line scripts using swift-argument-parser? I’m not writing services, just scripts that perform a series of tasks that involve a bunch of subprocesses.

11

IMO it's the easiest approach for handling signals and long-running tasks like spawning processes. The service word in its name probably mostly refers to how the package initially incubated in the swift-server org, but it can also refer to any long-running tasks spawned from a script as well, not just servers or daemons.

12

Yes, you should be able to interrupt a child process via Ctrl-C. For example, if you run

let result = try await Subprocess.run(
    .at("/bin/bash"),
    arguments: ["-c", "sleep 100s"]
)

You will be able to interrupt the child process using Ctrl-C

2 Likes
13

Subprocess also introduced this concept of "teardown sequence" with version 5. Now you can specify a sequence of signal to send to the child process for teardown:

var platformOptions = Subprocess.PlatformOptions()
platformOptions.teardownSequence = [
    .sendSignal(.quit, allowedNanoSecondsToExit: 100_000_000),
    .sendSignal(.terminate, allowedNanoSecondsToExit: 100_000_000),
]
let result = try await Subprocess.run(
    .at("..."),
    platformOptions: platformOptions
)

(Note that I'm updating allowedNanoSecondsToExit to use Duration in the next version.)

2 Likes
14

Nit: if it’s not updated, then the spelling should be Nanoseconds rather than NanoSeconds.

3 Likes
15

Something I'd really like to see is for us to fix signal handling on Windows. Right now, we conflate "uncaught exception" with "signal" which makes it hard to portably detect signals.

Swift Testing's experimental exit tests feature has logic to smuggle signals out of child processes on Windows, but it requires that the child process replace the default signal handlers at start, so I don't know if that's helpful.

CC @compnerd

3 Likes
16

nit: I believe there's a small mistake in one of the proposal's example code.

// Simple ls with no standard input
let ls = try await Subprocess.run(
    .named("ls"),
    output: .collect
)
print("Items in current directory: \(String(data: ls.standardOutput, encoding: .utf8)!)")

// Launch VSCode with arguments
let code = try await Subprocess.run(
    .named("code"),
    arguments: ["/some/directory"]
)
print("Code launched successfully: \(result.terminationStatus.isSuccess)")
                                     ^^^^^^
                                     Should be `code` instead of `result`.


// Launch `cat` with sequence written to standardInput
let inputData = "Hello SwiftFoundation".utf8CString.map { UInt8($0) }
let cat = try await Subprocess.run(
    .named("cat"),
    input: inputData,
    output: .collect
)
print("Cat result: \(String(data: cat.standardOutput, encoding: .utf8)!)")
1 Like
17

This is looking really great. Can't wait to finally use it! There were a few things that stood out while reviewing the proposal in depth again.

Sending closures

    public static func run<R>(
        ...
        _ body: (sending @escaping (Subprocess) async throws -> R)
    ) async throws -> ExecutionResult<R>

All of the run methods that take a closure require the closure to be sending. Why is that the case? Most with-style methods in the ecosystem take non-sending/non-Sendable closures that makes them easily composable with most other code. Is there an implementation detail that requires this? If so can we restructure the implementation to not require it?

Isolation of closures

I believe the closure taking methods are missing a isolation: isolated (any Actor)? = #isolation parameter here. Without that I assume most of those methods are almost impossible to use from an actor isolated context without data race errors. I talked about the need for the parameter in my recent talk here.

AsyncSequence of UInt8

I recall previous discussions around the usage of AsyncSequence<UInt8> and the performance problems that it can bring. Byte wise async iteration is often a cause for a huge performance loss and from our server experience we often use ByteBuffer/Array[Slice]<UInt8>/Data as the element of an async sequence here. @icharleshu you have done some performance benchmarking right? What was the outcome of that w.r.t. the type of the element used here? (I see further down that some async sequences use Data so this looks a bit inconsistent throughout the proposal)

Usage of pid_t and DWORD

I understand that the ProcessIdentifier APIs need to be platform specific though is it the right thing to expose APIs that use types like pid_t and DWORD directly or should we just expose them as Int?

ProcessIdentifier.value vs ProcessIdentifier.processID

Why are we differing between the platforms here? Couldn't it also be .value on Windows? Furthermore having a type called ProcessIdentifier have a property called processIDs feels redundant.

Unmanaged Subprocess

This section tries to achieve multiple things at the same time:

  1. Allow to pass unmanaged file descriptors
  2. Unstructured concurrency to be able to outlive the current task

I think 1. is really valuable and something that we should expose for expert users. However, it doesn't follow to me why this is tied to 2. There might be use-cases where you pass in file descriptors directly but you still want to wait for the process to terminate. Furthermore, you can achieve 2. by just sticking it into an unstructured Task like this, right?

Task {
  let ls = try await Subprocess.run(
    .named("ls"),
    output: .collect
  )
}

Usage of FileDescriptor & FilePath

FileDescriptor and FilePath are types provided by swift-system. Currently swift-system is not part of the toolchain. How do you expect to solve using those types in a public Foundation API?

Teardown

I got confused by this section for a second and assumed that there is only a teardown method but later on saw that there is also a configuration option. Is my assumption correct here that the teardown sequence passed via platform options is used when the task calling run is cancelled? If so could we spell this out more explicitly in the documentation?

ExecutionResult requires Sendable type

public struct ExecutionResult<T: Sendable>: Sendable

Why does this require a Sendable type? I guess it is tied to my above question why the closure is sending?

Minor: Generic parameter names

Can we use longer generic parameter names than R or S? So far the standard library tried to use names like Return for such parameters. It makes reading the docs later on a lot easier.

Minor: Common signals

We are currently offering a similar type in swift-service-lifecycle here. Over time we added more signals that were commonly used. We might want to extend the common values with the missing ones from there. Though this is not as important since there is a rawValue escape hatch.

Minor: send signal parameter label

public func send(_ signal: Signal, toProcessGroup shouldSendToProcessGroup: Bool)

Should the first external parameter label here really be underscored? I personally feel like subprocess.send(signal: .kill) reads better. (Same applies to the sendSignal on TeardownStep

9 Likes
18

Just to emphasize the importance of this, it's not always just about documentation either. Because of the ability to write constrained extensions, the names of generic arguments on types are public API on those types and should be named as any nested type member would. Most of the parameters cited above are on methods (where the point about clarity in documentation still stands), but there is one proposed type API that uses a single-letter name:

extension Subprocess {
  public struct ExecutionResult<T: Sendable>: Sendable {
    // ...
    public let value: T
  }
}

A user writing a conditional extension on this type would awkwardly write extension Subprocess.ExecutionResult where T: .... A name like Value would be better to clarify its purpose.

9 Likes
19

My issue is that the debugger killing the app leaves the subprocesses running (VSCode, Linux, Swift 6.0.1)

20

I’m a bit late to the party but have we considered adding overrides that use String as the input/output instead of Data or Sequence? I understand there are disadvantages for more complex use cases, but for many simple cases like in the given examples I think it would be a better fit. We could maybe use the parameter labels to specify that the string is being converted to bytes using utf8CString as well.

In the same direction, we might also want to consider accepting Strings (containing file paths) instead of file descriptors.

Again, I understand there are disadvantages, but for simpler use cases it seems to me that the resulting code would be easier to read and would more closely resemble the equivalent bash code (like the comparison in the start of the proposal suggests).

1 Like