Skip to content

refactor: modernize RunProcessAsync with WaitForExitAsync #1790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JamieMagee wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −39
Open

refactor: modernize RunProcessAsync with WaitForExitAsync #1790

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 29 additions & 39 deletions src/Microsoft.ComponentDetection.Common/CommandLineInvocationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,24 +125,21 @@ public async Task<CommandLineExecutionResult> ExecuteCommandAsync(string command
return await this.ExecuteCommandAsync(command, additionalCandidateCommands, workingDirectory: null, CancellationToken.None, parameters);
}

private static Task<CommandLineExecutionResult> RunProcessAsync(string fileName, string parameters, DirectoryInfo workingDirectory = null)
{
return RunProcessAsync(fileName, parameters, workingDirectory, CancellationToken.None);
}

private static Task<CommandLineExecutionResult> RunProcessAsync(string fileName, string parameters, DirectoryInfo workingDirectory = null, CancellationToken cancellationToken = default)
private static async Task<CommandLineExecutionResult> RunProcessAsync(
string fileName,
string parameters,
DirectoryInfo workingDirectory = null,
CancellationToken cancellationToken = default)
{
var tcs = new TaskCompletionSource<CommandLineExecutionResult>();

if (fileName.EndsWith(".cmd") || fileName.EndsWith(".bat"))
if (fileName.EndsWith(".cmd", StringComparison.OrdinalIgnoreCase) || fileName.EndsWith(".bat", StringComparison.OrdinalIgnoreCase))
{
// If a script attempts to find its location using "%dp0", that can return the wrong path (current
// working directory) unless the script is run via "cmd /C". An example is "ant.bat".
parameters = $"/C {fileName} {parameters}";
fileName = "cmd.exe";
}

var process = new Process
using var process = new Process
{
StartInfo =
{
Expand All @@ -153,52 +150,45 @@ private static Task<CommandLineExecutionResult> RunProcessAsync(string fileName,
RedirectStandardError = true,
RedirectStandardOutput = true,
},
EnableRaisingEvents = true,
};

if (workingDirectory != null)
{
process.StartInfo.WorkingDirectory = workingDirectory.FullName;
}

var errorText = string.Empty;
var stdOutText = string.Empty;
process.Start();

var t1 = new Task(() =>
{
errorText = process.StandardError.ReadToEnd();
});
var t2 = new Task(() =>
{
stdOutText = process.StandardOutput.ReadToEnd();
});
// Read both streams concurrently to avoid deadlocks if either fills its buffer.
var stdOutTask = process.StandardOutput.ReadToEndAsync(cancellationToken);
var stdErrTask = process.StandardError.ReadToEndAsync(cancellationToken);
Comment on lines +163 to +164
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadToEndAsync(cancellationToken) can throw OperationCanceledException if the token is canceled after WaitForExitAsync completes but before the stream-read tasks finish. That can cause this method to surface cancellation even though the process already exited successfully. Consider not passing the external cancellation token to the output/error reads (or switch to a separate token that is only canceled when you actually intend to abort the read), and rely on cancellation to kill the process instead.

This issue also appears on line 166 of the same file.

Suggested change
var stdOutTask = process.StandardOutput.ReadToEndAsync(cancellationToken);
var stdErrTask = process.StandardError.ReadToEndAsync(cancellationToken);
// Do not pass the external cancellation token here: cancellation is handled by
// WaitForExitAsync below, which kills the process. Once the process exits, these
// reads should be allowed to complete so we do not surface cancellation after a
// successful process exit.
var stdOutTask = process.StandardOutput.ReadToEndAsync();
var stdErrTask = process.StandardError.ReadToEndAsync();

Copilot uses AI. Check for mistakes.

process.Exited += (sender, args) =>
try
{
Task.WaitAll(t1, t2);
tcs.TrySetResult(new CommandLineExecutionResult { ExitCode = process.ExitCode, StdErr = errorText, StdOut = stdOutText });
process.Dispose();
};

process.Start();
t1.Start();
t2.Start();

cancellationToken.Register(() =>
await process.WaitForExitAsync(cancellationToken);
}
catch (OperationCanceledException)
{
try
{
process.Kill();
process.Kill(entireProcessTree: true);
}
catch (InvalidOperationException)
{
// swallow invalid operations, which indicate that there is no process associated with
// the process object, and therefore nothing to kill
// https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.kill?view=net-8.0#system-diagnostics-process-kill
return;
// Process already exited.
}
});

return tcs.Task;
throw;
}

var stdOut = await stdOutTask;
var stdErr = await stdErrTask;

return new CommandLineExecutionResult
{
ExitCode = process.ExitCode,
StdOut = stdOut,
StdErr = stdErr,
};
}
}
Loading