Starting a process asynchronous

Question

For executing a Process I've created an separate class which binds the possibilities together, for example reading output, start as Admin, catch Exceptions and also start all this asynchronous.

Are there any suggestions to improve it further?

public class ExternalProcess
{
 public string FileName { get; set; }
 public string Arguments { get; set; } = "";
 public string WorkingDirectory { get; set; } = "";
 public int Timeout_milliseconds { get; set; } = -1;
 public bool ReadOutput { get; set; }
 public bool ShowWindow { get; set; }
 public bool StartAsAdministrator { get; set; }
 public string Output { get; private set; } = "";
 public string OutputError { get; private set; } = "";
 public int ExitCode { get; private set; }
 public bool WasKilled { get; private set; }
 public bool WasSuccessful { get; private set; }
 public event EventHandler OutputChanged;
 public event EventHandler OutputErrorChanged;
 public bool Start(CancellationToken cancellationToken = default(CancellationToken))
 {
 var task = StartAsync(cancellationToken);
 task.Wait();
 return task.Result;
 }
 public async Task<bool> StartAsync(CancellationToken cancellationToken = default(CancellationToken))
 {
 if (String.IsNullOrWhiteSpace(FileName))
 {
 throw new ArgumentNullException(nameof(FileName));
 }
 //if (!File.Exists(FileName)) // removed because also commands could be executed (for example: ping)
 if (!ReadOutput)
 {
 Output = OutputError = $"Enable {nameof(ReadOutput)} to get Output ";
 }
 if (StartAsAdministrator)
 {
 ReadOutput = false; // Verb="runas" only possible with ShellExecute=true.
 Output = OutputError = "Output couldn't be read when started as Administrator ";
 }
 var useShellExecute = !ReadOutput; // true when started as admin, false for reading output
 if (!StartAsAdministrator && !ShowWindow)
 useShellExecute = false; // false for hiding the window
 using (var p = new Process
 {
 EnableRaisingEvents = true,
 StartInfo = new ProcessStartInfo()
 {
 FileName = FileName,
 Arguments = Arguments,
 UseShellExecute = useShellExecute,
 RedirectStandardOutput = ReadOutput,
 RedirectStandardError = ReadOutput,
 RedirectStandardInput = ReadOutput,
 CreateNoWindow = !ShowWindow,
 }
 })
 {
 if (StartAsAdministrator)
 {
 p.StartInfo.Verb = "runas";
 }
 if (!String.IsNullOrWhiteSpace(WorkingDirectory))
 {
 p.StartInfo.WorkingDirectory = WorkingDirectory;
 }
 if (ReadOutput)
 {
 p.OutputDataReceived += (sender, args) =>
 {
 if (!String.IsNullOrEmpty(args?.Data))
 {
 Output += args.Data + Environment.NewLine;
 OutputChanged?.Invoke(this, EventArgs.Empty);
 }
 };
 p.ErrorDataReceived += (sender, args) =>
 {
 if (!String.IsNullOrEmpty(args?.Data))
 {
 OutputError += args.Data + Environment.NewLine;
 OutputErrorChanged?.Invoke(this, EventArgs.Empty);
 }
 };
 }
 try
 {
 p.Start();
 }
 catch (System.ComponentModel.Win32Exception ex)
 {
 if (ex.NativeErrorCode == 1223)
 {
 OutputError += "AdminRights request Canceled by User!! " + ex;
 ExitCode = -1;
 WasSuccessful = false;
 return WasSuccessful;
 }
 else
 {
 OutputError += "Win32Exception thrown: " + ex;
 ExitCode = -1;
 WasSuccessful = false;
 throw;
 }
 }
 catch (Exception ex)
 {
 OutputError += "Exception thrown: " + ex;
 ExitCode = -1;
 WasSuccessful = false;
 throw;
 }
 if (ReadOutput)
 {
 // var writer = p.StandardInput; writer.WriteLine("");
 p.BeginOutputReadLine();
 p.BeginErrorReadLine();
 }
 CancellationTokenSource timeoutCancellationToken = null;
 if (Timeout_milliseconds > 0)
 {
 timeoutCancellationToken = new CancellationTokenSource(Timeout_milliseconds);
 }
 while (!p.HasExited && !cancellationToken.IsCancellationRequested && !(timeoutCancellationToken?.IsCancellationRequested ?? false))
 {
 await Task.Delay(10).ConfigureAwait(false); // configureAwait is that the task doesn't continue after the first await is done (prevent deadlock)
 }
 if (!p.HasExited)
 {
 WasKilled = true;
 OutputError += " Process was cancelled!";
 try
 {
 p.CloseMainWindow();
 int waitForKill = 30;
 do
 {
 Thread.Sleep(10);
 waitForKill--;
 } while (!p.HasExited && waitForKill > 0);
 if (!p.HasExited)
 {
 p.Kill();
 }
 }
 catch { }
 }
 ExitCode = p.ExitCode;
 p.Close();
 if (ExitCode == -1073741510)
 {
 OutputError += $"Process exited by user, exitcode: {ExitCode}!";
 }
 WasSuccessful = !WasKilled && ExitCode == 0;
 return WasSuccessful;
 }
 }
}

• t3chb0t: Could you explain it in more detail? How does the process killing work? What else it this doing? The description isn't very helpful.
• t3chb0t: Just one small warning: running Process in parallel is very tricky, see Occasionally not getting output from processes running in parallel - if you are going to do that, then it's much better to let each Process run in its own, new AppDomain that makes them completely independent.
• hoffmale: I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.

Answer 1

Ideally, you wouldn't use Task.Delay, and instead find a way to bind to Process event(s), and thereby create a brand new TaskCompletionSource. Also, you generally shouldn't return true/false, and should use exception throwing instead.

• see https://stackoverflow.com/a/15316668/887092 for TaskCompletionSource
• see https://stackoverflow.com/questions/4504170/why-is-my-processs-exited-method-not-being-called#4504208 for example of Process.Exited
• see https://softwareengineering.stackexchange.com/a/159097/78712 - there are still good cases where true/false should be returned (eg. while(reader.Read()))

Example of the start of a re-write, which highlights the 3 chages:

static Task MostBasicProcess()
{
 var t = new TaskCompletionSource<bool>(); //Using bool, because TaskCompletionSource needs at least one generic param
 var p = new Process();
 //TODO: more setup
 p.EnableRaisingEvents = true; //VERY important
 p.Exited += (object sender, EventArgs e) =>
 {
 ////TODO: Exceptions will go first, followed by `return;`
 //t.SetException();
 //TODO: Finally, if there are no problems, return successfully
 t.SetResult(true);
 };
 p.Start();
 //TODO: wrap .Start() in try-block and call t.SetException on error
 return t.Task; //We actually don't want the caller using the bool param, it's implicitly casted to plain Task.
}

Note: That exception processing should appear first to avoid too-much {} nesting. Also, you should really put all the code in a try-block.

Usage:

try
{
 await MostBasicProcess();
}
catch (Exception ex)
{
}

Of course, you need to adapt this to your broader needs.