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.
Subprocess will be introduced as a new, separate package, overseen by Foundation workgroup. In addition, there is a major redesign around input and output handling.
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
We were using a couple of libraries (SwiftSlash and SwiftCommand) that existed before swift-subprocess for our CI pipelines, and had issues collecting the full output for certain commands (such as running atos passing hundreds of addresses).
We replaced them with swift-subprocess and it has worked like a charm
The migration so far has been easy. I'll share specific feedback after we complete the migration (if there's any).
I disagree. Since the intention is to create a new package that is part of the Swift project and exposes new public APIs it should go through a formal review process. If we would only do this after its initial release we would lose the opportunity to review the initial set of APIs.
Within this package, we propose two modules: Subprocess and SubprocessFoundation . Subprocess serves as the ācoreā module, relying solely on swift-system and the standard library. SubprocessFoundation extends Subprocess by depending on and incorporating types from Foundation .
I would strongly encourage the use of package traits here. This would remove the need for a second module and you could even make it a default enabled trait since a dependency on FoundationEssentials is often not problematic.
For the run methods that take a body closure, why is the body closure @escaping?
extension Execution where Output == SequenceOutput {
/// The standard output of the subprocess.
/// Accessing this property will **fatalError** if
/// - `.output` wasn't set to `.redirectToSequence` when the subprocess was spawned;
/// - This property was accessed multiple times. Subprocess communicates with
/// parent process via pipe under the hood and each pipe can only be consumed once.
public var standardOutput: some AsyncSequence<Buffer, any Swift.Error>
}
extension Execution where Error == SequenceOutput {
/// The standard error of the subprocess.
/// Accessing this property will **fatalError** if
/// - `.error` wasn't set to `.redirectToSequence` when the subprocess was spawned;
/// - This property was accessed multiple times. Subprocess communicates with
/// parent process via pipe under the hood and each pipe can only be consumed once.
public var standardError: some AsyncSequence<Buffer, any Swift.Error>
}
Can we clarify the semantics of those async sequences here? In particular, can those be iterated multiple times or even concurrently? My expectation is that they can only be iterated once and non-concurrently.
extension Execution {
/// Performs a sequence of teardown steps on the Subprocess.
/// Teardown sequence always ends with a `.kill` signal
/// - Parameter sequence: The steps to perform.
public func teardown(using sequence: [TeardownStep]) async
}
Should this be rather taking some Sequence<TeardownStep> here?
First the argument label isn't aligned with the other sendSignal method i.e. it should drop the _ to be aligned
allowedDurationToExit this seems a bit weird to me. Is it really the duration to exit? I was kinda expecting this to be the duration between this step and the next step if the sub process didn't exit by then
public final actor StandardInputWriter
I was surprised to see that this is an actor. Why?
public func write(
_ array: [UInt8]
) async throws -> Int
Can't this API take some Sequence<UInt8> and then you don't need the Data overload or rather take RawSpan if we are leveraging the new Span APIs already?
public func write<Encoding: Unicode.Encoding>(
_ string: some StringProtocol,
using encoding: Encoding.Type = UTF8.self
) async throws -> Int
Do we need this overload if we already provide one that takes a RawSpan?
public func write<AsyncSendableSequence: AsyncSequence & Sendable>(
_ asyncSequence: AsyncSendableSequence
) async throws -> Int where AsyncSendableSequence.Element == Data
Why is this only available for Data but not [UInt8] can we just make this generic over an some AsyncSequence<some Sequence<UInt8>, any Error>?
Input/Output protocols
I have a few comments/questions on these two new protocols:
How can these types be Sendable when from the looks of it, it looks like it needs to store FDs which are not-Sendable? Looking at the implementation it looks like they require locks around the FDs. That seems really non-optimal when we should just be taking ownership of the FDs.
Instead of having distinct create/close methods can we use with-style methods instead here? We can pass the closeAfterSpawningProcess to the with style method so that it knows it if should close the FD or not.
In general, I feel like those protocols look like they are leaking implementation details of the subprocess to the outside. In my opinion, nobody should be seeing those types nor implement them. Are we potentially missing a lower level primitive here?
Pipe
The proposal text already says that but this is leaking an implementation detail as well. Additionally, why is Pipe a class? Could it be a ~Copyable struct instead? It looks like it's using a lock under the hood as well which shouldn't be needed since we have ownership of the FDs right?
Overall, I'm not yet convinced this new API design with exposing the protocols and underlying implementation details in the right choice. I understand why the choice was made to reduce the number of overloads of the run method but in doing so we exposes a huge swath of APIs that I personally think shouldn't be public. I like that we are adopting the new span types here though and that we are incubating this as a package so we can iterate faster and are uncoupled from the toolchains.
I donāt like the idea of run() being a freestanding function since itās so very likely to conflict with another method or function in local scope. Why not rename Execution to Subprocess and move the run methods back under it as static functions?
The first two declarations of CollectedResult-returning overloads refer to a ācustom closureā, but they take no closure parameter.
ProcessIdentifier is still prone to pid-reuse attacks, especially on Linux. On Darwin, ProcessIdentifier can be a pid + pidversion. Not sure what can be done on Linux or Windows.
Relatedly, on all platforms it would be good to opt-in to receiving a process handle rather than an identifier. On POSIX, this can be implemented with a file descriptor. On Windows, this can be the HANDLE returned by CreateProcess. This should be opt-in because the client must close the descriptor/handle.
A thread ID is not part of a process identifier on Windows. It feels strange to be part of the ProcessIdentifier type.
I concur that the freestanding run() functions are a bad choice. I don't want to have to track down which module a naming conflict is coming from. This should be a static function on a type.
Even though Subprocess is a separate package, it's still part of the Foundation project, and its API surface is still considered part of Foundation. Therefore, we treat it like any other API proposals from Foundation itself.
Admittedly, this is the first time Foundation has tried something like this, so it'll be a learning process for us as well.
This is a great idea. Thanks so much for bringing it up!
Good catch! I removed the @escaping requirement when I removed the sending requirement and forgot to update. It won't be @escaping nor sending.
You are correct. You can only access it once due to how pipe works, but you should be able to access it concurrently.
Sure. Originally I didn't bother because I think most cases people will just use array to spell out the steps.
Hmm maybe allowedDurationToNextStep?
To serialize the writes to child process. Since you are expected to call write concurrently with read but at the end of the day we are writing to a pipe so it needs to be serialized.
This was a deliberate design choice. If this API takes some Sequence<UInt8> then there is no way to guarantee that its contiguous, which means we might have to either implicitly copy the buffer to contiguous bytes before we can write it or iterate through byte by byte. Both of these causes unexpected performance issues so we deliberately limit the input to Array to Data so we won't have to copy.
You brought up a good point that StandardInputWriter should support writing RawSpan, which I'll add. However, it can't be the only way to write to StandardInputWriter because this is a compromise for the developers that can't use RawSpan yet.
On of the obvious downside of requiring RawSpan is of course clients will have to be updated in order to use RawSpan. Since RawSpan was just proposed, it will be a while before wide adoption is possible. Consequently, we've decided to only require RawSpan if you want to use the CollectedResult family of runs. You are still able to use Subprocess if your system does not support Span -- you just have to use the run() methods with execution body, which StandardInputWriter is part of.
I hear and 100% understand your concern. I didn't like the fact that I had to expose Pipe either so I spend a lot of time weighting different options and comparing this approach with the original approach. I eventually decided on this protocol based approach for a few reasons despite its downsides:
Most importantly, protocol based approch makes Subprocess more extensible. With the old approach, you are stuck with the pre-defined list of InputMethods and OutputMethods provided by Subprocess whereas with protocols you can define your own input output (although I'm expeting it to be extremely uncommon). One of the major driving factor to this point is the adoption of String output. In the previous review I received lots of requsts to support String output. I realized with the old design since the actual reading logic is spread out through Subprocess, I'll have to many changes in order to support this one additional output type. In contrast, with protocols I only have to implement one concrete class to conform to one protocol in order to add additional "output"s. This is something developers could do as well to fit their specific needs.
The protocol based appraoch, although "leaking" some implementation details, follows Swift's progressive disclosure philosophy. In most (if not all) situations, I don't envision developers would need to create their own IO types, which means they can use the built in IO types directly, just as they were with InputMethod and OutputMethod. If a developer needs special IO, they have the option to create IOs that conform to ManagedInputProtocol and ManagedOutputProtocol, which allows developers to only having to implement one method instead of deadling with FleDescriptors and Pipes directly. With these two levels of disclosure, developers will hardly ever use/see the implementaion details.
The "implementation details" in question (dealing with FileDescriptors and Pipes) is unlikely to change due to the fact that all platform APIs (fork/exec, posix_spawn, CreateProcessW) use file descriptor as the currency type. Therefore, it's less concerning that we expose these as "API"s since I'm not expecting them to change, just like the public APIs.
This is more so an implementation detail but the short answer is I didn't find a great way to make it ~Copyable because we actually do need to send these IOs to different execution context in case we need clean up. The next best option would be an actor to avoid having to use locks, but we need to support the runDetached() method where no Swift concurrency is involved, hence this approach. I'm still reevaluating other potential solutions.
This was also a deliberate design decision. I made this change because Subprocess is now a separate module rather than being part of Foundation, which means by default it already gets its own namespace. This also means you can still optionally write Subprocess.run if you want, which will solve any ambiguous problem:
import Subprocess
let result = Subprocess.run(.name("ls"))
However, the upside for this approach is that the Subprocess. bit is now optional to write. Which means if you are writing scripts, the code looks way cleaner:
import Subprocess
let lsResult = run(.name("ls"))
let gitResult = run(.name("git"), arguments: ["diff"])
Sorry I don't follow... Could you elaborate on how would pid+pidversion solve pid-reuse? We are getting this value from posix_spawn and CreateProcessW directly.
How would a process handle be utilized on UNIX? Most relevant APIs typically use a process ID rather than a handle, correct? On Windows, although I thought about returning the process handle, itās not feasible to do so safely since as you pointed out, the client would need to manually close the handle. To avoid creating an unsafe API, I think itās simpler for developers to call OpenProcess directly themselves. This way, they can also specify the desired access.
I don't have strong opinion over this since I'm not a Windows expert. I'm fine removing it as long as no one else objects.
Agree. This will cause breakage, feels contrary to most other APIs and also makes shim'ing out a concrete implementation via a protocol harder. I can't confirm the Subprocess module to a protocol SubprocessAPI which would allow me to shim it out for tests.
Furthermore, namespacing helps with code completions etc.
Agreed, new software shouldn't exclusively use pids.
One important piece of documentation would be to document the exact errors when something goes wrong. Let's not repeat the mistake that posix_spawn did where 'executable not found' and 'working directory not found' both degreade into the same error.
That's highly problematic because one can't easily implement a race-free program that tries out a bunch of different paths for an executable. Something simlilar to $PATH resolution of an executable. What you want to do is to try a bunch of paths one after the other. And if you get 'executable not found' you'll try the next one. But if you get 'working directory not found' you stop trying because that'll affect everything.
try await run(.name("ls")), I dislike the name .name here. It's not that there is a specially "named" program ls, what it actually means is that the environment variable $PATH is traversed front-to-back. So maybe run(.resolvedFromPath("ls")) or something that's clear about what's going on.
Doing $PATH traversals is a security-sensitive choice which must be prohibited in certain scenarios so it should be explicit.
Why are teardown sequences macOS & Linux only? What happens on Windows?
public var preSpawnProcessConfigurator: (
@Sendable (
inout posix_spawnattr_t?,
inout posix_spawn_file_actions_t?
) throws -> Void
)? = nil
This seems highly problematic as it forces Subprocess to use posix_spawn. Similarly,
// Set Group ID for process
platformOptions.preSpawnProcessConfigurator = {
setgid(4321)
}
forces us to use fork()+execve() on Linux.
This is problematic
public protocol InputProtocol: Sendable {
/// Lazily create and return the FileDescriptor for reading
func readFileDescriptor() throws -> FileDescriptor?
/// Lazily create and return the FileDescriptor for writing
func writeFileDescriptor() throws -> FileDescriptor?
/// Close the FileDescriptor for reading
func closeReadFileDescriptor() throws
/// Close the FileDescriptor for writing
func closeWriteFileDescriptor() throws
In general, file descriptor creations & destructions have to be asynchronous. Once we can leverage io_uring they will be async. And if you were to use a system that uses epoll/kqueue you may first have to de-register a file descriptor which means you may need to hop threads -- that also makes closes/creations async in general.
/// The subprocess was signalled with given exception value
case unhandledException(Code)
This seems like odd verbiage. A process doesn't exit with an exception (at least on UNIX), it may exit with an uncaught signal.
/// Error thrown from Subprocess
public struct SubprocessError: Swift.Error, Hashable, Sendable {
/// The error code of this error
public let code: SubprocessError.Code
/// The underlying error that caused this error, if any
public let underlyingError: UnderlyingError?
}
This is missing a subject. As mentioned above: It's crucial to be able to distinguish ENOENT on the executable vs. the chosen working directory. So if we get a not found, we need to know what wasn't found.
Finally, a few general comments:
It feels like Subprocess is overly platform-dependent. Some functionality (e.g. teardown sequences) are crucial to the correctness, they should be present everywhere
I agree with @FranzBusch that Subprocess in its current form leaks way too many platform details (using posix_spawn on Darwin, using fork+exec on Linux) that may change
I think we need more documentation on how exactly Subprocess interacts with Structured Concurrency and cancellation. I saw the Task Cancellation paragraph which is great but I think it needs to be fleshed out a little more.
I see Subprocess is using FileDescriptor and FilePath. Which ones are those, are they SystemPackage.{FileDescriptor,FilePath} on Linux and System.{FileDescriptor,FilePath} on Darwin? If so, that'd be bad because everybody will have to add a #if canImport(System) import System #else import SystemPackage #endif and conditionally depend on https://github.com/apple/swift-system.git.
How can I make the child process inherit my file descriptor N as the child's file descriptor M? It's often important to take a parent file descriptor and install this as the child's fd 3 or something.
I saw no documentation regarding the closure of the other file descriptors. UNIX programs typically need to close all file descriptors that they don't want to inherit into the child. What mechanism is used here?
I like the free function - I think they're great for basic operating system services, and I wish Foundation would use them more.
For example, I remember that back when async sequences were being proposed, the Foundation team included an example that looked like:
let url = URL(string: "http://example.com")!
for try await line in url.lines {
// ...
}
I suggested I had some other API ideas and was hoping they'd make a community pitch so we could bikeshed it, but unforuntately they didn't. I think networking is a great candidate for a free function, and that's what I would have suggested instead:
for try await line in fetch("http://example.com").lines {
// ...
}
// Currently.
let data = try await URLSession.shared.data(from: URL(string: "http://example.com"))!
// With a 'fetch' global function...
let data = try await fetch("http://example.com").data
That's how it works in Javascript, and it's really nice.
With the Fetch API, you make a request by calling fetch(), which is available as a global function in both window and worker contexts. You pass it a Request object or a string containing the URL to fetch, along with an optional argument to configure the request.
I remember this, because one thing that's been on my backlog is to make a package with this kind of API. I'll probably never get around to it, so Foundation team... I know my feedback is a bit late, but... please steal this
In general, I very much like the idea of these kinds of basic services as global functions. It feels nice to just have fetch available everywhere, without ceremony. I think launching subprocesses could be a good candidate as well, although it's possible the name run is a bit too vague.
While it may feel nice to some, polluting the global namespace with free functions is irreversible for the rest of us. Users who prefer this behavior can opt in and define their own free functions (in a helper module/package if reusability is needed) that call into namespaced functions. The reverse is not true, I'm unable to opt out of global namespace pollution if I prefer to avoid it.
What's the point of having global functions if we are effectively unable to use them?
Part of API design is making choices; having an opinion. Think of Karoy's "name vs description" argument. I think there are cases where global functions can be a great design - for these kinds of globally available services, and we should not be afraid to use them.
fetch is a name. URLSession.shared.data(from:) is a description.
If we need better disambiguation options or other support in the language, we should do that rather than effectively ban global functions as a design option.
IMO discoverability, disambiguation, and testability are prerequisites for great design. "Feels nice" is a very subjective point that doesn't address these prerequisites. After years of writing JavaScript, I don't have any fond memories of trying to make fetch (or any of those global functions) testable.
Again, it's irreversible consequences of global namespacing that I'm concerned with. One can always make it global in their own codebase, or Foundation can make it universally global later when/if appropriate facilities become available in the language itself. But making it universally global before those issues are resolved feels rushed.
There can be some technical arguments against global functions, but we should consider which of them may be wholly/partially solvable, because they open up a wider creative space.
As for testability, URLSession is also not particularly testable, and whether you write URLSession.shared.data(for:) or fetch (with some default arguments) does not impact testability. This is what I mean by not outright dismissing global functions as a design tool, trying to keep a more open mind, and appreciating their strengths as well as their weaknesses.
I'm interested in discussing objective design qualities, not sure how a discussion about subjective things can be meaningful here.
Right, but consider global functions after technical issues are wholly/partially solved, not before. Without resolving those issues, that creative space is not open.
These concerns are specific to URLSession and are not universally applicable. One could design a namespaced alternative to URLSession that's testable, while a global free function would eliminate that option.
Darwin has pid versions is seems, Linux has pidfd and Windows has handles. I'm not saying v1 of this package has to use pidfd/pid versions/handles but I think it should be sufficiently opaque that a later version can add support for that.
So I believe the low-level race free process-addressing APIs do exist on all relevant platforms.
I donāt see how pidfd closes the reuse gap. You create one from a pid, so even if the implementation does let pid = posix_spawn(ā¦); let pidfd = syscall(SYS_pidfd_open, pid, ā¦) thereās still a window in which the launched process can crash and its pid reassigned to an attacking processās child.
Even on Darwin, as @Jean-Daniel points out, the OS primitive only returns a pid. Darwin does have the equivalent to a Windows handle (the audit token) but you canāt get one from posix_spawn. We might be stuck with the pid abstraction until other platforms catch up to Windows here.
I know my feedback is a bit late, but... please steal this 