1
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Could you explain it in more detail? How does the process killing work? What else it this doing? The description isn't very helpful. \$\endgroup\$ – t3chb0t Jul 10 '18 at 15:05
  • \$\begingroup\$ 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. \$\endgroup\$ – t3chb0t Jul 18 '18 at 14:38
  • \$\begingroup\$ 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. \$\endgroup\$ – hoffmale Jul 20 '18 at 14:48
1
\$\begingroup\$

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.

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.

\$\endgroup\$
0
\$\begingroup\$

According to the points you have mentioned I've changed some parts. Nice is that reading output is also possible during execution. That it's better usable for unit-testing it isn't a static method.

Are there any suggestions to improve it further?

[Serializable]
public class ProcessSettings
{
    public string FileName { get; set; }
    public string Arguments { get; set; } = "";
    public string WorkingDirectory { get; set; } = "";
    public string InputText { get; set; } = null;
    public int Timeout_milliseconds { get; set; } = -1;
    public bool ReadOutput { get; set; }
    public bool ShowWindow { get; set; }
    public bool KeepWindowOpen { get; set; }
    public bool StartAsAdministrator { get; set; }
    public CancellationToken CancellationToken { get; set; }
}

public class ProcessOutputReader   // to get the output while executing instead only as result at the end
{
    public event TextEventHandler OutputChanged;
    public event TextEventHandler OutputErrorChanged;
    public void UpdateOutput(string text)
    {
        OutputChanged?.Invoke(this, new TextEventArgs(text));
    }
    public void UpdateOutputError(string text)
    {
        OutputErrorChanged?.Invoke(this, new TextEventArgs(text));
    }
    public delegate void TextEventHandler(object sender, TextEventArgs e);
    public class TextEventArgs : EventArgs
    {
        public string Text { get; }
        public TextEventArgs(string text) { Text = text; }
    }
}

[Serializable]
public class ProcessResult
{
    public string Output { get; set; }
    public string OutputError { get; set; }
    public int ExitCode { get; set; }
    public bool WasKilled { get; set; }
    public bool WasSuccessful { get; set; }
}

public class ProcessStarter
{
    public ProcessResult Execute(ProcessSettings settings, ProcessOutputReader outputReader = null)
    {
        var task = ExecuteAsync(settings, outputReader);
        task.Wait();
        return task.Result;
    }

    public async Task<ProcessResult> ExecuteAsync(ProcessSettings settings, ProcessOutputReader outputReader = null)
    {
        if (settings.FileName == null) throw new ArgumentNullException(nameof(ProcessSettings.FileName));
        if (settings.Arguments == null) throw new ArgumentNullException(nameof(ProcessSettings.Arguments));

        var cmdSwitches = "/Q " + (settings.KeepWindowOpen ? "/K" : "/C");

        var arguments = $"{cmdSwitches} {settings.FileName} {settings.Arguments}";
        var startInfo = new ProcessStartInfo("cmd", arguments)
        {
            UseShellExecute = false,
            RedirectStandardOutput = settings.ReadOutput,
            RedirectStandardError = settings.ReadOutput,
            RedirectStandardInput = settings.InputText != null,
            CreateNoWindow = !(settings.ShowWindow || settings.KeepWindowOpen),
        };
        var output = new StringBuilder();
        var error = new StringBuilder();
        if (!settings.ReadOutput)
        {
            output.AppendLine($"Enable {nameof(ProcessSettings.ReadOutput)} to get Output");
        }
        if (settings.StartAsAdministrator)
        {
            startInfo.Verb = "runas";
            startInfo.UseShellExecute = true;  // Verb="runas" only possible with ShellExecute=true.
            startInfo.RedirectStandardOutput = startInfo.RedirectStandardError = startInfo.RedirectStandardInput = false;
            output.AppendLine("Output couldn't be read when started as Administrator");
        }
        if (!String.IsNullOrWhiteSpace(settings.WorkingDirectory))
        {
            startInfo.WorkingDirectory = settings.WorkingDirectory;
        }
        var result = new ProcessResult();
        var taskCompletionSourceProcess = new TaskCompletionSource<bool>();
        using (var process = new Process { StartInfo = startInfo, EnableRaisingEvents = true })
        {
            process.OutputDataReceived += (sender, e) =>
            {
                if (e?.Data != null)
                {
                    output.AppendLine(e.Data);
                    outputReader?.UpdateOutput(e.Data);
                }
            };
            process.ErrorDataReceived += (sender, e) =>
            {
                if (e?.Data != null)
                {
                    error.AppendLine(e.Data);
                    outputReader?.UpdateOutputError(e.Data);
                }
            };
            process.Exited += (sender, e) =>
            {
                try { (sender as Process)?.WaitForExit(); } catch (InvalidOperationException) { }
                taskCompletionSourceProcess.TrySetResult(false);
            };

            try
            {
                process.Start();
            }
            catch (System.ComponentModel.Win32Exception ex)
            {
                if (ex.NativeErrorCode == 1223)
                {
                    error.AppendLine("AdminRights request Canceled by User!! " + ex);
                    taskCompletionSourceProcess.SetException(ex);
                }
                else
                {
                    error.AppendLine("Win32Exception thrown: " + ex);
                    taskCompletionSourceProcess.SetException(ex);
                }
            }
            catch (Exception ex)
            {
                error.AppendLine("Exception thrown: " + ex);
                taskCompletionSourceProcess.SetException(ex);
            }
            if (startInfo.RedirectStandardOutput)
                process.BeginOutputReadLine();
            if (startInfo.RedirectStandardError)
                process.BeginErrorReadLine();
            if (startInfo.RedirectStandardInput)
                process.StandardInput.WriteLine(settings.InputText);

            if (settings.CancellationToken != default(CancellationToken))
                settings.CancellationToken.Register(() => taskCompletionSourceProcess.TrySetResult(true));
            if (settings.Timeout_milliseconds > 0)
                new CancellationTokenSource(settings.Timeout_milliseconds).Token.Register(() => taskCompletionSourceProcess.TrySetResult(true));

            var taskProcess = taskCompletionSourceProcess.Task;
            await taskProcess.ConfigureAwait(false);
            if (taskProcess.Result == true) // process was cancelled by token or timeout
            {
                if (!process.HasExited)
                {
                    result.WasKilled = true;
                    error.AppendLine("Process was cancelled!");
                    try
                    {
                        process.CloseMainWindow();
                        await Task.Delay(10).ConfigureAwait(false);
                        if (!process.HasExited)
                        {
                            process.Kill();
                        }
                    }
                    catch { }
                }
            }
            result.ExitCode = process.ExitCode;
            process.Close();
        }
        if (result.ExitCode == -1073741510 && !result.WasKilled)
        {
            error.AppendLine($"Process exited by user!");
        }
        result.WasSuccessful = !result.WasKilled && result.ExitCode == 0;
        result.Output = output.ToString();
        result.OutputError = error.ToString();
        return result;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.