Take advantage of the new Process APIs
- Dominant language
- C#
- Stars
- 3.2k
- Forks
- 1.3k
- PR merge metrics
- PR metrics pending
Description
In this issue I am going to collect all the places where I believe SDK could benefit from using the new Process APIs (https://github.com/dotnet/runtime/issues/125838#issuecomment-4120569132).
When I am done editing it, I am going to tag people
### Process.StartAndForget (https://github.com/dotnet/runtime/pull/126078)
Plenty of C# devs assume that when a `Process` gets disposed it's also getting killed. It's not true and we are introducing a dedicated method that is simply going to spawn the process, fetch PID and dispose the resources to prevent resource leaks.
We need to search for usages similar to this one:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Cli/dotnet/Commands/Clean/FileBasedAppArtifacts/CleanFileBasedAppArtifactsCommand.cs#L141
and this one:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/RazorSdk/Tool/ServerProtocol/ServerConnection.cs#L399-L400
Where reference to process is not stored anywhere and/or not being used for anything and simply use the new API.
```diff
- Process.Start(startInfo);
+_ = Process.StartAndForget (startInfo);
```
**Update:** sent https://github.com/dotnet/sdk/pull/54093 to address this
### SafeProcessHandle.Signal (https://github.com/dotnet/runtime/pull/126313
`SafeProcessHandle` was extended with `Signal` method that allows for signaling the process.
So usages similar to this one:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Dotnet.Watch/Watch/Process/ProcessRunner.cs#L372
can be replaced with a call to `process.SafeHandle.Signal(posixSignal);` (in this particular case the exception handling needs to be different as well, as we the new process returns `false` if the process has already exited but throws `Win32Exception` in case it's running but we don't have the permissions to kill it (which should not be the case, since it's our own child process)
**Update:** done in https://github.com/dotnet/sdk/pull/53920.
### SafeProcessHandle.WaitForExitOrKillOnCancellationAsync (https://github.com/dotnet/runtime/issues/126293)
`SafeProcessHandle.WaitForExitOrKillOnCancellationAsync` will allow us to ensure the process is killed once the token is cancelled.
It could be used in places similar to this one:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Dotnet.Watch/Watch/Process/ProcessRunner.cs#L65-L76
(but in this particular case it can't be replaced 1:1 because `WaitForExitOrKillOnCancellationAsync` does not support graceful termination
### SafeProcessHandle.WaitForExit (https://github.com/dotnet/runtime/issues/126293)
`SafeProcessHandle.WaitForExit` is not going to wait for EOF, it's simply going to wait for the process to exit. So workarounds like [this](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Dotnet.Watch/Watch/Process/ProcessRunner.cs#L256-L317) are not going to be needed.
### ProcessStartInfo.KillOnParentExit (https://github.com/dotnet/runtime/issues/101985)
So far the API was implemented only for Windows (https://github.com/dotnet/runtime/pull/126699), but it will allow to get rid of `ProcessReaper` for .NET 11+ targets.
### ProcessStartInfo.StandardInputHandle (https://github.com/dotnet/runtime/pull/125848)
It's now possible to redirect standard handle to any `SafeFileHandle`, to usages like [this](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs#L116-L121) may consider writing the blob to a file and then redirecting input to a file.
### Process.Run[Async]
A high-level helper for just executing given process. We may use it places like [here](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Dotnet.Watch/HotReloadClient/Web/KestrelWebSocketServer.cs#L102-L107) and [here](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Cli/Microsoft.DotNet.Cli.Utils/Extensions/ProcessStartInfoExtensions.cs#L10-L27) and at the same time avoid resource leaks (the process will get killed on timeout)
### Process.RunAndCaptureTextOutput[Async]
Another high level helper that will do what [this](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Cli/Microsoft.DotNet.Cli.Utils/Extensions/ProcessStartInfoExtensions.cs#L29-L61) code does in a very performant way.
### Process.ReadAllText (https://github.com/dotnet/runtime/pull/126807)
Instead of trying to drain std out and err like [here](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs#L37-L39) the new API allows for reading both at the same time using a single thread without the risk of getting into deadlock when there is a lot of data written to error.
### Process.Kill(bool entireProcessTree)
We [may](https://github.com/dotnet/runtime/issues/126273) make the `Process.Kill()` much faster, but when reading the code I've realized that we are not passing `true` here:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/test/Microsoft.NET.Sdk.Publish.Tasks.Tests/EndToEnd/ProcessWrapper.cs#L77
**Update:** sent https://github.com/dotnet/sdk/pull/53919 to address this.
### UseShellExecute
It's a very old API, but I think it should be used [here](https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Cli/dotnet/Commands/Help/HelpCommand.cs#L47-L75) instead of trying to find the right `xdg-open` thing.
Also, when it's not set to `true`, it's impossible for the `Process.Start` to return `null`. So checks like this:
https://github.com/dotnet/sdk/blob/5be3937332d7d45a12cb8a95fdbfc67c8fb99513/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs#L105-L113
are invalid, because they are impossible.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.