dotnet / dotnet/sdk

Take advantage of the new Process APIs

Open
#53,916 0 comments 0 reactions 0 assignees View on GitHub
untriaged
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.