Skip to content
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.

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

Fix deadlock in ProcessUtil #1457

Merged
merged 4 commits into from
Dec 19, 2023
Merged
Changes from 2 commits
Commits
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
56 changes: 25 additions & 31 deletions src/Aspire.Hosting/Dcp/Process/ProcessUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public static (Task<ProcessResult>, IAsyncDisposable) Run(ProcessSpec processSpe
process.StartInfo.Environment[key] = value;
}

var processEventLock = new object();
var startupComplete = new ManualResetEventSlim(false);
JamesNK marked this conversation as resolved.
Show resolved Hide resolved

// Note: even though the child process has exited, its children may be alive and still producing output.
// See https://github.com/dotnet/runtime/issues/29232#issuecomment-1451584094 for how this might affect waiting for process exit.
Expand All @@ -49,67 +49,61 @@ public static (Task<ProcessResult>, IAsyncDisposable) Run(ProcessSpec processSpe
{
process.OutputDataReceived += (_, e) =>
{
lock (processEventLock)
startupComplete.Wait();
JamesNK marked this conversation as resolved.
Show resolved Hide resolved

if (e.Data == null || process.HasExited)
{
if (e.Data == null || process.HasExited)
{
return;
}
return;
}

processSpec.OnOutputData.Invoke(e.Data); // not holding the event lock
processSpec.OnOutputData.Invoke(e.Data);
};
}

if (processSpec.OnErrorData != null)
{
process.ErrorDataReceived += (_, e) =>
{
lock (processEventLock)
startupComplete.Wait();
JamesNK marked this conversation as resolved.
Show resolved Hide resolved
if (e.Data == null || process.HasExited)
{
if (e.Data == null || process.HasExited)
{
return;
}
return;
}

processSpec.OnErrorData.Invoke(e.Data); // not holding the event lock
processSpec.OnErrorData.Invoke(e.Data);
};
}

var processLifetimeTcs = new TaskCompletionSource<ProcessResult>();

process.Exited += (_, e) =>
{
lock (processEventLock)
startupComplete.Wait();
JamesNK marked this conversation as resolved.
Show resolved Hide resolved

if (processSpec.ThrowOnNonZeroReturnCode && process.ExitCode != 0)
{
if (processSpec.ThrowOnNonZeroReturnCode && process.ExitCode != 0)
{
processLifetimeTcs.TrySetException(new InvalidOperationException(
$"Command {processSpec.ExecutablePath} {processSpec.Arguments} returned non-zero exit code {process.ExitCode}"));
}
else
{
processLifetimeTcs.TrySetResult(new ProcessResult(process.ExitCode));
}
processLifetimeTcs.TrySetException(new InvalidOperationException(
$"Command {processSpec.ExecutablePath} {processSpec.Arguments} returned non-zero exit code {process.ExitCode}"));
}
else
{
processLifetimeTcs.TrySetResult(new ProcessResult(process.ExitCode));
}
};

try
{
AspireEventSource.Instance.ProcessLaunchStart(processSpec.ExecutablePath, processSpec.Arguments ?? "");

// Take the event lock to ensure that OnStart() is called before the lifetime task ends.
lock (processEventLock)
{
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
processSpec.OnStart?.Invoke(process.Id);
}
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
processSpec.OnStart?.Invoke(process.Id);
}
finally
{
startupComplete.Set(); // Allow output/error/exit handlers to start processing data.

AspireEventSource.Instance.ProcessLaunchStop(processSpec.ExecutablePath, processSpec.Arguments ?? "");
}

Expand Down
Loading