[Review, 3rd] SF-0007: Subprocess

[FranzBusch]

I have been thinking a lot about this proposal over the past days and the evolution of this proposal over the various pitches and reviews. All the engagement throughout shows how important this is to many Swift contributors and that it fills a gap in the current ecosystem. During my various review comments I had one constant train of thought: "How can we be sure this works after review on all platforms?" I was never able to answer that myself because I was lacking the knowledge of how processes are handled on platforms like Windows or potentially future Swift supported platforms.

When I saw that this is pitched now to be created in a separate repository outside of Foundation I was incredibly happy since it allowed us to iterate on it more and not tie it to the high API and ABI bar that Foundation is upheld to. I think we should actually go one step further and position this review as a round of feedback to get an initial package out there but fully accept that this package needs time to incubate and be battle tested before we can commit to the APIs. I fully expect that if we start to use any API in production use-cases we will find small and potentially large bits that need to change.

@icharleshu

This is an intentional choice. IMO it's more important to allow Subprocess to be more extensible and allow escape hatches than hiding all the platform specific details. All the "leaks" mentioned so far follows progressive disclosure philosophy because developers won't have to deal with any of the platform details untiles they are specifically seeking advanced custom behavior.

I have been looking through the updated implementation and re-read the proposal and I disagree that this is progressive disclosure. To me this looks rather that we are lacking core primitive abstractions that Subprocess is trying to invent here. The reason why I'm thinking this way is two-fold:

• With the proposed APIs the underlying implementation is forced to make synchronization decisions that are bad for performance. A lot of the code is using mutexes to protect the file descriptors or an actor in the case of the writer. This shouldn't be necessary really. At any point in time we should have a clear idea who the owner of the file descriptor is and make it possible to do the I/O completely lock/actor free.
• @johannesweiss mentioned this regarding async creation/closure of the file descriptors already. I agree that subprocess is not in the game of defining FileDescriptor APIs but it shows that the we don't have proper APIs right now. swift-system only provides synchronous APIs and they are not correct working with things like IO_URING.

Now the previous iteration of this proposal didn't have this problem since it hide all of these details in the high level run methods allowing us to correctly implement it below or move the backing implementation to things like IO_URING if we wanted to. The updated proposal is exposing so many implementation details which opens up a completely new problems spaces that ought to be solve by lower level building blocks.

I personally think we should take a step back and reduce the complexity we added with the new protocols here and instead overload the run method again. I would even go as far and drop a few of the overloads that the run method had even if that requires a bit more boilerplatey code.