Is this statement true? Given the amount of comments and arguments in the pitch thread and this thread I wouldn't call it "well trod ground", especially not in Swift, especially when the API isn't just a simple wrapper around posix_spawn and tries to use many of the new language features from Swift.
1 Like
Let me bump this. An example could be the parent process launching two subprocesses and redirecting the first subprocess output to the second subprocess input (I believe that would be possible via Process API). Will that be possible with the Subprocess API, and is it possible for the first subprocess to write in an async/await manner and what API could be used for that?
2 Likes
I interpreted your previous concerns to be about the nature of process management APIs and user expectations. This is well trod, as the functionality exists in many languages and, as youāve seen previously in these threads, there are already a huge number of use cases that can be used as example motivators for solutions in the new API. Given this breadth of existing work I see little reason to be conservative in the APIās scope, at least in regard to the problem domain.
If, instead, your concern is that trying solve these well known problems in Swift may lead to language changes, then I think you need to be clearer about where, specifically, you see these issues, what language features may help, and whether those features are even being considered for the language. And frankly, if the concern is that significant functionality, or even things like not deadlocking in certain scenarios, must wait for language features, then this entire proposal should be delayed. Thereās no pressing need for this API so it can certainly give the language and system APIs time to evolve to the point where proper Swift wrappers can be built.
5 Likes
May I please beg that once this review is complete, that the documentation is very explicit about which use-cases this type is designed to work well with, and which is specifically not designed to work with. The average user will assume that this is a like-for-like replacement of Process and most likely never read this thread.
5 Likes
What was the outcome of this? From the proposal it seems that nothing is being added to System.
3 Likes
Overall I like where this is going. I have definitely struggled with adopting the Process APIs (do I need a file handle or pipe for output? how do I deal with reading from them safely and correctly?), so I'm very pleased to see this becoming easier.
As I'm reading the proposal, the thing that keeps jumping out at me is "this is great, but I'm immediately going to be creating convenience methods on a bunch of this to simplify it even further". Allow me to explain.
The vast majority of my personal usage is invoking known tools with specific argument sets for specific scenarios. One might imagine writing a Git client that works by shelling out to the git command line tool. You're always going to be using a specific executable with almost entirely hard-coded sets of arguments. With that example in mind, here are the things I would add to this type before I even tried it once:
Hiding Subprocess.Executable
There's a lot of ceremony around specifying the executable I want to run, with superfluous argument labels and parameter wrappers. What I would really want to write is:
let result = try await Subprocess.run("git", ...)
Therefore, I would write a convenience thing that looks at the first string parameter and asks "does this have a leading /? If so, treat it as a .at(...), otherwise assume it's a .named(...)". (I understand that different platforms deal with path separators differently, but all my use-cases are for unix-derived systems so far)
Supporting Variadic Arguments
I would also look at adding some static func run(...) versions that take a String... so I could easily use variadic arguments to avoid the unnecessary ceremony of brackets around arguments:
let result = try await Subprocess.run("git", "checkout", "-b", newBranchName, cwd: myCurrentWorkingDirectory)
Simplified String Output
I can't recall the last time I used a command line tool that produced anything other than text. Maybe some do if you're dealing with specialized media-handling tools, but in my couple of decades of experience, I've maybe had to consciously deal with that ā¦ once? If at all?
Therefore, before using this, I would immediately add conveniences for dealing with the Data? values on CollectedResult to turn them into String values. And I think everyone will be doing this.
(Oh I thought of an example: something like find . -print0 | xargs -0 echo. However, it's exceptionally rare for me to need to do that programmatically)
Other Thoughts:
- It's odd to me that
Subprocess.PlatformOptionshas POSIX-specific methods as part of its API. Are those relevant on non-POSIX-based operating systems? Subprocess.InputMethodhas a.noInputstatic var, which is the default. This feels like the place that uses it should be using anInputMethod?instead so the default can just be.none.- I often deal with setting up piped tools, such as doing an
lscommand and piping the results throughgrep. As I'm reading this, I'm having a hard time imagining how I'd even begin to go about setting that up.
Maybe a tangent?
It's weird to me that all the major functionality of this is provided as static func methods on a type that is almost entirely empty, making it barely more useful than as a pseudo-namespace. As I'm reading through this, I'm left wondering what it might look like if Subprocess were defining a template for a thing you want launched, and it had a .launch(...) instance method that returns a handle to the launched subprocess (or even just the result).
Then I could potentially do things like:
let gitStatus = Subprocess("git", "status")
while shouldKeepCheckingStatus == true {
let result = try await gitStatus.launch()
print(result.outputString)
try await Task.sleep(for: .seconds(5))
}
Maybe this would make it easier to build up piped processes?
let list = Subprocess("ls", "-al")
let grep = Subprocess("grep", theThingToSearchFor)
let trim = Subprocess("tr", "-d", "' '")
let result = try await (list | grep | trim).launch()
Overall Review
I'm really really glad to see this area of the frameworks getting some love, because it desperately needs it.
However, I'm left with the feeling that it doesnāt meet the #1 rule of designing a good API:
Make common things easy. Make hard things possible.
This API feels like it's built to handle all the hard things, which is what you want in a system-level framework. It's carefully considering non-textual input and output. It's handling corner cases around providing that IO in "correct" ways and stuff.
But it's failing at making the common things easy. "Launch an executable with some hard-coded arguments and get the result as a string" seems excessively complicated, and that feels like it's going to be the 99% use-case, which implies to me that everyone using this will be forced to do extra work just for doing simple tasks.
So, I love the functionality of this API, but its interfaces are still lacking.
-1 needs more work before shipping.
19 Likes
In addition to .named() to launch a process by name and .at() to launch it by URL consider having .current to launch the (copy of the) current process.
You canāt safely get a path to the current process image on many POSIX platforms.
e.g.:
catcurl --output - <URL>find . -print0(technically text, but not compatible with simplistic notions of text like NULL-terminated-strings)
There's a big difference between "text is common and should be convenient to work with" and "only text should be supported".
Also, define "text". e.g. what character set? What encoding?
Certainly there are many folks that take the "everything is a file" Linux philosophy to an extreme, but nonetheless it's not a universal opinion nor practice; there are often good reasons to avoid resorting to the file system, such as:
- Performance (pipes are much lower overhead than going through a whole file system).
- Efficiency (pipes can provide back pressure).
- Reliability (interacting with the file systems adds a lot of failure modes).
- Endurance / uptime / cost (persistent storage does in fact wear out, which may be a rare occurrence in some contexts - e.g. typical personal device use - but is expensively not rare in others - e.g. servers).
- Portability (not every environment even has a writable file system available).
This is a bit humorous, because most of this discussion thread so far has centred around concerns it's only designed to handle the simple things.
I don't envy @icharleshu in this case (nor the review committee), because this proposal concerns very important functionality that clearly a lot of people care about and have opinions about.
4 Likes
tar, gzip, curl, ā¦
1 Like
Yeah, that's fair. I did think of find . -print0 as I was composing my answer, and @Jean-Daniel is correct that other tools do non-text things. Thinking about it some more, I suppose I would clarify that, based on my usage of NSTask/Process, I'm always trying to get UTF8 text out of the result so that I can have rich information about what the tool did. Usually my use-cases for invoking tools are to perform tasks that I cannot in my own app (due to sandboxing, the perpetual bane of my existence) or because the APIs to accomplish my task are massively arcane and it's just simpler to invoke the ready-made tool to do it for me. Perhaps non-textual output is more common in non-app development.
I'm not sure what to say about the "hard vs common" things, beyond restating that if the API were to ship as-is, I believe most app developers would end up creating convenience methods to make interacting with the type simpler. That's what I'd certainly be doing.
1 Like
I'm personally in favor of keeping the Subprocess.Executable wrapper, as call sites such as system("ls") leave a lot unsaid and often leave me wondering: "Does this examine PATH? Would this resolve to a binary named ls in the current working directory?" and so on. Subprocess.run(executing: .named("ls")) makes that clear and saves me from a trip to the documentation (or option-clicking if I'm in Xcode).
Spawning subprocesses and all of its various details can be nightmarish, so I'd appreciate an abstraction that prioritizes explicitness and correctness here at the expense of ergonomics. A minimal wrapper function to fix some of the arguments doesn't sound too bad to me.
I do wonder why .named wasn't spelled as .resolving/.fromEnvironment/similar, as that makes the "path resolved heuristically" aspect incredibly obvious, and, technically speaking, in both .named("git") and .at("/usr/bin/git") you are ultimately targeting a binary named "git". (Or, at least, that's the intention. Symlinks are a thing, after all.)
And speaking of Subprocess.Executable, I think .named("git").resolveExecutablePath(in: .inherit) is neat, but what would e.g. .at("/usr/bin/git").resolveExecutablePath(in: ā¦) do here?
With the review period ended, as an observer I find myself fairly uncertain of the proposal and status of the issues raised -- and uncertain even if all issues have been covered (esp. things like signal propagation and termination).
I would appreciate some authoritative summary, and feedback on that. I realize it's a big ask, and the review effort is likely dwarfing implementation at this point, but bugs or bad API here could cause a lot of difficulty and confusion.
Those 2 are quite similar, so lets try to tackle it. There are a few gotchas in there, so I think that it would be an excellent example in the documentation. Though, I believe that @tera "first subprocess to write in an async/await manner and what API could be used for that" is not an Subprocess.run area of responsibility, so I will not mention it here.
I will use grep -o "Elizabeth" "Pride and Prejudice.txt" | wc -l as an example. This should count the number of times that the word "Elizabeth" appears in "Pride and Prejudice.txt" (or something like this, whateverā¦).
Option 1: Naive - be carefull!
let pipe = try FileDescriptor.pipe()
let grep = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeTo(pipe.writeEnd, closeWhenDone: true)
)
let wc = try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: .readFrom(pipe.readEnd, closeWhenDone: true)
)
let result = wc.standardOutput.map { String(decoding: $0, as: UTF8.self) }
print(result ?? "?")
This works, it will print "645".
Deadlock 1: wc starts only after the grep finished. If grep fills the pipe it will deadlock, as nobody is reading it. This is the cat "Pride and Prejudice.txt" all over again.
Deadlock 2: If we change the grep to:
// Change the 'grep', everything else is the same.
let grep = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeTo(pipe.writeEnd, closeWhenDone: false) // < ----- HERE
)
I just changed closeWhenDone: true to false (user mistake). Because pipe.writeEnd never gets closed then wc cannot finish, as it is possible that somebody will write something in there. This would be equivalent to:
let pipe = try FileDescriptor.pipe()
// Write something to 'pipe.writeEnd', but DO NOT close it.
let wc = try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: .readFrom(pipe.readEnd, closeWhenDone: true)
)
// (Code here never executes.)
Side-note: 1 week ago this would always deadlock because the PR used posix_spawn_file_actions_adddup2 to send the file to a child without closing it in the parent process. Even if the child closed the file, it would still be open in the parent resulting in an infinite loop. The rule is: as long as there is writeEnd open then the read will never return 0 (read() == 0 means end of file).
This was fixed, so it should not matter. But what matters is that: since we close the parent writeEnd and the child also closes its end then it is possible to get the read() == 0. If I'm correct it would be possible to redefine CollectedOutputMethod.collect from:
Collect the output as Data with the default 16kb limit. Possible deadlock.
To:
Collect all of the Data. Never deadlock.
Personally I like the new definition, so much that I would make it a default argument for both stdout/stderr.
Option 2: Nested compose - suboptimal
Now we know that we have to run both of the processes at the same time. We can either nest or withTaskGroup. Let's start with nesting.
let runResult = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .redirect
) { p in
return try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: p.standardOutput! // YOLO. It's not like I can handle 'nil' meaningfully anyway.
)
}
let result = String(decoding: runResult.value.standardOutput!, as: UTF8.self)
print(result)
This will work. As far as I know it will not deadlock. It looks pretty, because we used input: p.standardOutput! for the wc. It composes! In practice this is a bit sub-optimal. To see why let's do a tiny renaming:
let runResult = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeToParentProcess // <---- HERE, it was 'redirect'
) { p in
// Same as aboveā¦
}
I renamed RedirectedOutputMethod.redirect to .writeToParentProcess to make it more clear. What we are doing is:
- CHILD-1 writes to pipe
- PARENT reads CHILD-1 pipe in
AsyncBytes - PARENT writes to CHILD-2 pipe
- CHILD-2 reads the pipe
You can validate this by modifying the AsyncBytes.Iterator.next method to print every time it is called.
I'm not exactly sure if I like the Subprocess.output being composable with Subprocess.input (via AsyncBytes). But it will work. And, as far as I can tell it should not deadlock unless we apply some additional buffering in AsyncBytes and there are some VERY specific requirements. Then: maybe.
Bike shedding: I'm not a fan of the redirect name. For me it fails in the following ways:
- redirect where?
- what do I gain from redirect?
If we renamed it as [write/pipe]To[Parent/Current]Process then I think it would be more clear what is happening:
- where do we redirect? To
[Parent/Current]Process. - what do I gain from redirect? We can interact with it in the current process.
public struct RedirectedOutputMethod: Sendable, Hashable {
public static var discard: Self
public static var writeToParentProcess: Self
public static func writeToFile(_ fd: FileDescriptor, close: Bool) -> Self
}
We can:
- rename
noInputtonone- this would beSubprocess.run(ā¦, input: .none)for the callers - make it
NilLiteralConvertible - leave as it is
No strong opinion here, I don't like bike shedding.
Option 3: Nested with pipe - works
Same as above, but we create a pipe to connect the children.
let pipe = try FileDescriptor.pipe()
let runResult = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeTo(pipe.writeEnd, closeWhenDone: true)
) { p in
return try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: .readFrom(pipe.readEnd, closeWhenDone: true)
)
}
let result = String(decoding: runResult.value.standardOutput!, as: UTF8.self)
print(result)
It works.
Option 4: Task group
let result = try await withThrowingTaskGroup(of: String.self, returning: String.self) { group in
let pipe = try FileDescriptor.pipe()
group.addTask {
try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeTo(pipe.writeEnd, closeWhenDone: true)
)
return ""
}
group.addTask {
let r = try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: .readFrom(pipe.readEnd, closeWhenDone: true)
)
return String(decoding: r.standardOutput!, as: UTF8.self)
}
var result = ""
for try await partial in group {
if !partial.isEmpty {
result = partial
}
}
return result
}
print(result)
It works. It can be simplified, but whateverā¦
3 Likes
This highlights the importance of having this as an easily available option, right? Or do you imply this can't be done in the API implementation because of the reason you are stating?
(Yeah I agree it should exist and should be part of the "designated initializer" path; my complaint is about there not being conveniences are eliding it for simple call-sites.)
Iām saying we canāt assume itās possible to implement. That said, Linux has /proc/self/exe, Darwin has proc_pidinfo(), and Windows has GetModuleFileName (which returns a full path, not just a filename). Whether itās a good idea to use this information is debatableāif you overwrite a programās file on disk, thereās a very big difference between āclone the current processā and āspawn a new process from the same path I was spawned fromā.
2 Likes
A simpler fix is to use async let.
let pipe = try FileDescriptor.pipe()
async let grep = try await Subprocess.run(
executing: .at(FilePath("\(bin)/grep")),
arguments: ["-o", "Elizabeth", "Pride and Prejudice.txt"],
output: .writeTo(pipe.writeEnd, closeWhenDone: true)
)
async let wc = try await Subprocess.run(
executing: .at(FilePath("\(bin)/wc")),
arguments: ["-l"],
input: .readFrom(pipe.readEnd, closeWhenDone: true)
)
let result = await wc.standardOutput.map { String(decoding: $0, as: UTF8.self) }
print(result ?? "?")
The problem is:
- Users have to first know that they must do something like this (which the API provides little help in suggesting), and thenā¦
- ā¦always remember to do it. And correctly.
It's a bit error-prone.
It flies in the face of convention in shell scripting languages, where the concurrency of individual tasks within a pipeline is implicit. I have to imagine that a big use-case for executing subprocesses is in fact these kinds of shell scripting scenarios (whether literally in 'shell scripts' written in Swift, or merely that sort of behaviour in "traditional" programs).
It's very difficult to hit these kind of deadlock problems with the existing Process API. The simple and intuitive version using Process is free of these issues:
let connectingPipe = Pipe()
let grep = Process()
grep.executableURL = URL(fileURLWithPath: "\(bin)/grep")
grep.arguments = ["-o", "Elizabeth", "Pride and Prejudice.txt"]
grep.standardInput = nil
grep.standardOutput = connectingPipe
grep.run()
let wc = Process()
wc.executableURL = URL(fileURLWithPath: "\(bin)/wc")
wc.arguments = ["-l"]
wc.standardInput = connectingPipe
let outputPipe = Pipe()
wc.standardOutput = outputPipe
wc.run()
wc.waitUntilExit()
let output = outputPipe.fileHandleForReading.readToEnd()
print(String(data: output, encoding: .utf8) ?? "?")
It's a bit verbose (and has other don't-forget-you-have-to-do-this pitfalls) because it uses the "init as a useless lump and then mutate the hell out of it" anti-pattern, as was the style of the Objective-C times, but otherwise it's quite intuitive and straightforward.
4 Likes
Ooo! I forgot about async let. The code looks cleaner than any of my examples.
Currently when I run it I get one of:
- crashes before printing anything
grepfinishes,wchangs
There is some race going on. I managed to run it successfully once, but I had a few breakpoints, so I probably un-raced it this way. I'm not going to look into it. Ubuntu 22.04; Swift 5.10; 2 core Intel CPU.
At least we know that there is a good way to solve this. I assume that this will work in the future, and it would be "the correct way" to do this.
EDIT: The reason for crash/race was because I tried to run it on 2 CPU cores. The code from the PR uses Swift Concurrency in a way that does not like low-end machines.
As far as the old Process goes: Python does a very similar thing (I copied this example from their docs; but I used their API in production for many years and never had any problems):
proc = subprocess.Popen(...)
try:
outs, errs = proc.communicate(timeout=15)
except TimeoutExpired:
proc.kill()
outs, errs = proc.communicate()
For deadlocks they have:
- subprocess.Popen.wait warns about deadlock
- subprocess.Popen.communicate does not deadlock
Their documentation has 3 warnings about deadlocks: in wait, in communicate and just below stderr. The last one is in the red box. They also have async/await version of Popen, but I never used it.
I see. Even if it's: "do-the-best-what's-possible-to-launch-the-copy-of-self-even-if-that's-not_the-right-thing" would be useful to have as the ".current" constant (as the alternative is user doing the same "not ideal thing" by taking many non trivial steps ending having the same "non ideal" result).