Runtime: Proposal: Add Task-based Run API for System.Diagnostics.Process

[agocke]

The Exited event API is cumbersome for 99% of the way I use processes (run external command, get output when exited). I end up writing a Task-based wrapper every time. It would be great if we could just add an API for that.

Edit:

• [x] [WaitForExitAsync](https://github.com/dotnet/corefx/issues/34689)

Other similar issues:

• https://github.com/dotnet/corefx/issues/3483

[davidfowl]

Yes! I always end up writing this.

[stephentoub]

@agocke, can you propose what you'd want the API to look like? Are you asking for a method like:

``` C#
public Task WaitForExitAsync();

or one like:

``` C#
public static Task<Process> StartAndWaitForExitAsync(ProcessStartInfo startInfo);

or both or something else?

[svick]

Since this would be a convenience API, I think it:

• should be as convenient as possible
• doesn't need to be very efficient
• doesn't have to be able to do everything

Because of that, the API that I would like to see would be something like:

``` c#
public class Process
{
public static Task RunAsync(string fileName);
public static Task RunAsync(string fileName, string arguments);

// OR:
public static Task<ProcessResult> RunAsync(string fileName, string arguments = null);

}

public class ProcessResult
{
public ProcessResult(int exitCode, string standardOutput, string standardError);

public int ExitCode { get; }
public string StandardOutput { get; }
public string StandardError { get; }

}
```

This way, it's very easy to start a process, wait for it to exit and then get its output.

My questions:

• Are there any properties of ProcessStartInfo that are common enough that they would be missed here? (Process.Start() has overloads that accept userName, password and domain, but entering them doesn't seem to be a common use case to me.)
  
  
  
  • If yes, should there be an overload of RunAsync() that takes ProcessStartInfo? Or should RunAsync() have overloads with some additional parameters?
  
  
• Are there any properties of Process that would be missed on ProcessResult?
  
  
  
  • If yes, should ProcessResult just include a Process property?
  
  
  • Should ProcessResult have properties for FileName and Arguments? (If ProcessResult had the Process property, those would be accessible as e.g. .Process.StartInfo.FileName, but that's pretty long.)
  
  
• Is always capturing both StandardOutput and StandardError into a string acceptable? If not, how should it be controlled?

[agocke]

I like @svick's proposal. A few more arguments that could be useful: working directory, environment variables, and "standard in" text.

We have a Roslyn test helper that looks like this:

        public static ProcessResult Run(
            string fileName,
            string arguments,
            string workingDirectory = null,
            IEnumerable<KeyValuePair<string, string>> additionalEnvironmentVars = null,
            string stdInput = null)

[davidfowl]

Is always capturing both StandardOutput and StandardError into a string acceptable? If not, how should it be controlled?

I think these should be Streams/Writer/Reader instead of strings. The more I think about it, the more I think there needs to be 2 sets of methods:

• StartAsync, WaitForExistAsync
• StartAndWaitForExitAsync

[svick]

@davidfowl Why would StartAsync need to be async? If that was the convenience version that returns something like ProcessResult, wouldn't calling it StartAsync/Start be confusing, since the existing Start returns something different (Process)?

[MattKotsenas]

Hey everyone, I started to take a look at this issue, and I have an initial proposal and a few additional thoughts.

I think WaitForExitAsync is a pretty straightforward building block API that’s relatively easy to understand and use correctly. I have a version available here: feature/12039-process-waitforexitasync. I can PR it ASAP if people agree. Here's the proposed API:

public partial class Process
{
    public Task<bool> WaitForExitAsync(CancellationToken cancellationToken = default) { throw null; }
}

Rationale for the API is as follows:

• Method returns bool to determine if the process or exited or not, which matches WaitForExit semantics.
• Method takes a single, optional parameter, a CancellationToken, to support timeouts, which matches WaitForExit(int timeout) semantics.
• Method returns true if the process terminated before the CancellationToken is cancelled, false otherwise. The method does _not_ throw an OperationCanceledException.

Design notes:

• API doesn't expose an WaitForExitAsync(int timeout) overload since CancellationToken has a constructor that takes a timeout, and there's CancellationToken.TimeoutAfter().
• Because the method internally relies on the Exited event and there's a potential race between setting EnableRaisingEvents and the process exiting, I introduced a new instance of InvalidOperationException informing the caller to set EnableRaisingEvents to make this explicit. If this is deemed undesirable coupling I can move the set into this method, at the expense of possibly throwing and catching the InvalidOperationException from GetProcessHandle(), which I'd rather avoid if possible.

* The implementation could avoid an await in the case that the waited-on process has already exited. I declined to do that for the initial implementation to keep the logic clean for review. I can add that if deemed desirable.

@davidfowl, I’d like to know more about your thoughts around StartAsync and StartAndWaitForExitAsync? As @svick mentioned, Start and StartAsync seems confusing to me, since in both cases the process is just started and doesn’t block the thread.

I started playing around with a StartAndWaitForExitAsync, but I’ve run into a roadblock around how to handle input and output.

1. Doing something like @svick’s ProcessResult that returns Readers / Writers could result in deadlocks for large inputs, unless the API copies output to another buffer that we control, which feels bad for perf.
2. Looking at usage across places like StackOverflow, lots of people are using the input / output buffers as an ad-hoc communication protocol and need to interleave writes to StandardInput based on the results of the previous data read from StandardOutput. That means an API like this might be limited to simple "start-and-wait-without-interaction" scenarios, in which case it doesn’t gain much over process.Start(); await process.WaitForExitAsync();.

The best I’ve come up with so far is this feature/12039-process-startandwaitforexitasync.

public partial class Process
{
    public Task<Process> StartAndWaitForExitAsync(Action<string> onStandardOutputWrite = null, Action<string> onStandardErrorWrite = null, CancellationToken cancellationToken = default) { throw null; }
}

Rationale and design notes for the API is as follows:

• Method returns Process so the user can inspect the process and get information as needed (e.g. ExitCode). A new object such as ProcessResult could be introduced, but if cancellation is requested before the process terminates, the user needs access to the process to possibly take action (i.e. wait on it again or kill it). Additionally, making the ProcessResult hold the process requires ProcessResult to implement IDisposable, which seems like a bad tradeoff given that any information on the ProcessResult can also be gotten from the Process.
• By making the user responsible for collecting strings again we can avoid the buffer deadlock issues, at the expense of having optional parameters that have to align with the process’s StartInfo.
• Waiting semantics are delegated to WaitForExitAsync.
• Note that this API _forces_ processes with redirected output to use the async read pattern, but since the API returns a Process, users could attempt to access the underlying streams, which would be empty (and probably lead to confusion).

Again, I’m not super thrilled with this design, so if we feel it’s important I want to discuss what use cases we’re after. The Process class already has a fair bit of “call this before you use that or it breaks” style pitfalls that I’d like to avoid contributing to.

If there’s anything else, or if you have any questions, please let me know. Thanks!

[MattKotsenas]

Since this issue is pretty stale, I'm pinging @wtgodbe, @krwq, as area owners per this document to hopefully get the discussion started again.

If there's another more appropriate place or method to discuss this, please let me know. Thanks!

[krwq]

@MattKotsenas I don't see a point of returning a bool for WaitForExitAsync, I think Task<int> or just Task makes more sense.. If process has exited then Task will be completed otherwise it won't. If you want to finish waiting earlier then you request cancellation and still can check if cancellation happened earlier than normal completion. As for StartAsync I can see how in many cases you want to just run short-living process and get the exit code and get output and possibly write some output.

Possibly StartAsync could take StreamWriter/StreamReader or just Stream for each of stdout, stderr and stdin and basically give Process class sole control of them until it is done. I.e. if you pass Console.OpenStandardOutput() to that it would start printing to standard output - null would mean you don't want to redirect.

[MattKotsenas]

@krwq I agree that just Task makes the most sense for WaitForExitAsync; it more naturally follows idiomatic async / await coding styles. I've updated my branch feature/12039-process-waitforexitasync with the updated API signature and tests to show some example uses.

Now instead of checking the bool, the caller can explicity check process.HasExited for process status, and for cancellation either catch the OperationCanceledException or check the task's IsCanceled property.

To keep things simple, mind if we just focus on WaitForExitAsync for now, and come back to the "start and wait for exit async" once the first API is settled?

[krwq]

Sounds good to me. WaitForExitAsync is fairly agreed on while the StartAsync still has some open ends.

Let's perhaps create a separate issue for WaitForExitAsync and I'll update the first post here with the link to that.

[MattKotsenas]

@krwq Opened dotnet/corefx#34689 to split off WaitForExitAsync. Let me know if you need anything else. Thanks!

[GSPP]

I made a proposal for a more convenient process API a while ago: https://github.com/dotnet/corefx/issues/3483

There is a lot of overlap with svick's proposal but also some alternative ideas.

[krwq]

@GSPP thanks for linking to this issue, I've edited the first post so it doesn't get lost.