Change runDetached to return a ProcessHandle instead of a ProcessIdentifier

[jakepetroules]

Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns.

This patch now returns a non-copyable ProcessHandle, which provides access to both the ProcessIdentifier and the underlying platform handles (like Execution), while also guaranteeing that the underlying platform handles are released once the ProcessHandle goes out of scope. In effect, the platform handles are now released at the end of the runDetached caller's scope, rather than just before runDetached itself returns to the caller, solving the Windows-specific race condition.

This requires new API because ProcessIdentifier is Hashable and Codable and can't reasonably be made non-Copyable. Similarly, it's likely undesirable to use Execution as the return type because it too would need to be made non-Copyable to prevent the resource leak and there is at least one example in existing tests which requires Execution to be Copyable.

Closes #94

[jakepetroules]

@compnerd Any thoughts on this or the associated GitHub issue?

[compnerd]

Exposing hProcess is a very powerful construct, so I think that is very useful. I think that this basically is the ultimate escape valve - no matter any functionality that we may miss, the user should be able to workaround that.

I do wonder if we need to worry about the resource leak as the CloseHandle is required whereas on Unix, there is no release for the pid that is returned. Can we use a wrapper type that handles the lifetime and performs a CloseHandle in the case that the reference is lost?

[jakepetroules]

I thought about that...

I think we'd need to make Execution non-Copyable and add a deinit that calls CloseHandle? I seem to recall there was previous discussion about doing making Execution non-Copyable, but it didn't happen, maybe @iCharlesHu can provide some info on why that path wasn't chosen at the time?

And then I wonder if we would want some method like "takeHandle" which would allow the client to explicitly take ownership of the HANDLE away from the Execution.

[iCharlesHu]

I think we should start a discussion about this API change on the Swift forums. The current approach, which involves returning Execution, was actually one of my initial designs. However, it was specifically rejected due to the issue raised by @compnerd: we don’t want to return something and rely on the user to close it. This is the reason why we opted for the closure-based approach.

In my opinion, the only viable way to support this API on Windows is through a with-style closure, allowing us to close the HANDLE after the closure returns. However, this approach defeats the purpose of this API, which was intended to be a straightforward wrapper around the platform spawning API. Consequently, I think we have three options: 1) use the with-style API exclusively on Windows; 2) use the with-style API on all platforms; 3) omit this API on Windows since it’s not safe to offer it. I’m not sure about the most appropriate approach, and given the significant nature of this API change, I think we should discuss it on the forum to gather more feedback.

[jakepetroules]

However, it was specifically rejected due to the issue raised by @compnerd: we don’t want to return something and rely on the user to close it.

Doesn't ~Copyable + deinit allow us to close the handle without requiring the user to do so?

Also, I would argue that returning a numeric pid on Unix is still a resource leak of sorts, since the zombie will remain in the process table until the user calls waitid on it. So how's that much different from failing to call CloseHandle on Windows?

[jakepetroules]

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

https://github.com/swiftlang/swift-build/pull/584/files#diff-82ce9e4dd6439d5eba2c40db05433107a2fcf9d7434aa1cec546f0163e755aee

[jakepetroules]

@iCharlesHu I ended up splitting part of this change into #101 (which also introduces new API but is independently useful of these runDetached-specific issues), and now instead of returning Execution, runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released (so there can be no resource leaks) while still solving the underlying race condition since the caller now has access to the platform handles from the return value of runDetached.

Happy to still start a thread on the Swift forums if you think it's needed but hopefully this removes the resource leakage controversy in an elegant way!

[FranzBusch]

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

FWIW, this was my initial feedback as well. I don't see a need for this API as well since users can just spawn a task to do this detached work. I do recall that @iCharlesHu had a good point for why we need it anyways.

runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released

I'm a bit worried about this in general but not too familiar with Windows. The reason we see very little ~Copyable adoption for resources yet is that teardown of most resources is often throwing and/or asynchronous which both are not representable with ~Copyable types. For this reason we normally opt for with-style methods when handling resources.

[jakepetroules]

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

FWIW, this was my initial feedback as well. I don't see a need for this API as well since users can just spawn a task to do this detached work. I do recall that @iCharlesHu had a good point for why we need it anyways.

runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released

I'm a bit worried about this in general but not too familiar with Windows. The reason we see very little ~Copyable adoption for resources yet is that teardown of most resources is often throwing and/or asynchronous which both are not representable with ~Copyable types. For this reason we normally opt for with-style methods when handling resources.

This is a good point. CloseHandle can fail, but should only be in cases where you've given an invalid handle or tried to close the same one multiple times, both of which are programmer error and can be precondition'd. I'm not sure if there could be platforms in the future where this is not the case (Fuschia or something?), but for now any current and speculative API calls in teardown would be CloseHandle on Windows and close on POSIX (for process file descriptors, which Linux and FreeBSD both have).

Indeed if we could simply remove runDetached entirely, that would solve the problem. I'm interested in hearing @iCharlesHu's original reasoning for why we needed it, or if we could instead express whatever that was with the closure-based API.

[iCharlesHu]

This API change needs to be discussed.

[itingliu]

@swift-ci please test