From 657b4ac6d58d4a4e0bb45b60fdb853129723cd5f Mon Sep 17 00:00:00 2001 From: lewing Date: Thu, 18 Jun 2026 16:02:18 -0500 Subject: [PATCH] [wasm] WBT: replace pre-net11 Process pattern with ReadAllLinesAsync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses akoeplinger's review feedback on dotnet/runtime#129590: the Process APIs added in .NET 11 (#127106 Process.ReadAllLines/Async, #127210 RunAndCaptureText, etc.) replace the BeginOutputReadLine + DataReceivedEventHandler + StartAndWaitForExitAsync dance entirely, including the output-drain race the prior commit on this PR was trying to paper over with a sync WaitForExit() call. ToolCommand.ExecuteAsyncInternal now uses Process.ReadAllLinesAsync() which yields each redirected stdout/stderr line via an IAsyncEnumerable and only completes once both streams have hit EOF. A final WaitForExitAsync() then ensures Process.ExitCode is safe to read. The custom StartAndWaitForExitAsync extension and its companion RunProcessAndWaitForExit helper in ProcessExtensions.cs are no longer used anywhere in the wasm tree and the file is removed. The legacy DataReceivedEventHandler events on ToolCommand (OutputDataReceived / ErrorDataReceived) had no in-tree subscribers — the public Action-shaped WithOutputDataReceived / WithErrorDataReceived methods are preserved and now wire directly to internal per-line callbacks. Net: -62 lines, no behavior loss, race fixed by the BCL rather than by us. Validation: NonWasmConsoleBuild_WithoutWorkload theory (6 cases including the Release+WasmBuildNative=true variant that motivated this PR) all pass locally on macOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Common/ProcessExtensions.cs | 55 --------------- .../Wasm.Build.Tests/Common/ToolCommand.cs | 69 +++++++++---------- 2 files changed, 31 insertions(+), 93 deletions(-) delete mode 100644 src/mono/wasm/Wasm.Build.Tests/Common/ProcessExtensions.cs diff --git a/src/mono/wasm/Wasm.Build.Tests/Common/ProcessExtensions.cs b/src/mono/wasm/Wasm.Build.Tests/Common/ProcessExtensions.cs deleted file mode 100644 index b38079e2452543..00000000000000 --- a/src/mono/wasm/Wasm.Build.Tests/Common/ProcessExtensions.cs +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System; -using System.Collections.Generic; -using System.Diagnostics; -using System.IO; -using System.Runtime.InteropServices; -using System.Threading.Tasks; - -namespace Wasm.Build.Tests -{ - internal static class ProcessExtensions - { - private static int RunProcessAndWaitForExit(string fileName, string arguments, TimeSpan timeout, out string stdout) - { - var startInfo = new ProcessStartInfo - { - FileName = fileName, - Arguments = arguments, - RedirectStandardOutput = true, - UseShellExecute = false - }; - - var process = Process.Start(startInfo); - - stdout = null; - if (process.WaitForExit((int)timeout.TotalMilliseconds)) - { - stdout = process.StandardOutput.ReadToEnd(); - return process.ExitCode; - } - - process.Kill(entireProcessTree: true); - // sigkill - 128+9(sigkill) - return 137; - } - - public static Task StartAndWaitForExitAsync(this Process subject) - { - var taskCompletionSource = new TaskCompletionSource(); - - subject.EnableRaisingEvents = true; - - subject.Exited += (s, a) => - { - taskCompletionSource.SetResult(null); - }; - - subject.Start(); - - return taskCompletionSource.Task; - } - } -} diff --git a/src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs b/src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs index ed26c62f07e8c5..7eb8b4ee46fdb4 100644 --- a/src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs +++ b/src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs @@ -23,9 +23,11 @@ public class ToolCommand : IDisposable public Dictionary Environment { get; } = new Dictionary(); - public event DataReceivedEventHandler? ErrorDataReceived; - - public event DataReceivedEventHandler? OutputDataReceived; + // Per-line callbacks used by ExecuteAsyncInternal. Wired via WithOutputDataReceived / + // WithErrorDataReceived. Replaces the older `event DataReceivedEventHandler` pair that + // were tied to the pre-net11 Process.BeginOutputReadLine pattern. + private Action? _onOutputLine; + private Action? _onErrorLine; public string? WorkingDirectory { get; set; } @@ -61,13 +63,13 @@ public ToolCommand WithEnvironmentVariables(IDictionary? extraEn public ToolCommand WithOutputDataReceived(Action handler) { - OutputDataReceived += (_, args) => handler(args.Data); + _onOutputLine += handler; return this; } public ToolCommand WithErrorDataReceived(Action handler) { - ErrorDataReceived += (_, args) => handler(args.Data); + _onErrorLine += handler; return this; } @@ -111,42 +113,36 @@ private async Task ExecuteAsyncInternal(string executable, string { var output = new List(); CurrentProcess = CreateProcess(executable, args); - DataReceivedEventHandler errorHandler = (s, e) => - { - if (e.Data == null || isDisposed) - return; - - string msg = $"[{_label}] {e.Data}"; - output.Add(msg); - _testOutput.WriteLine(msg); - ErrorDataReceived?.Invoke(s, e); - }; - - DataReceivedEventHandler outputHandler = (s, e) => - { - if (e.Data == null || isDisposed) - return; - - string msg = $"[{_label}] {e.Data}"; - output.Add(msg); - _testOutput.WriteLine(msg); - OutputDataReceived?.Invoke(s, e); - }; - - CurrentProcess.ErrorDataReceived += errorHandler; - CurrentProcess.OutputDataReceived += outputHandler; try { - var completionTask = CurrentProcess.StartAndWaitForExitAsync(); - CurrentProcess.BeginOutputReadLine(); - CurrentProcess.BeginErrorReadLine(); - await completionTask; + CurrentProcess.Start(); + + // Process.ReadAllLinesAsync (added in .NET 11) yields each redirected stdout/stderr + // line as it arrives and completes once both streams have hit EOF. The streams + // close when the child process closes its pipe handles — typically (but not always) + // observable before Exited fires. We still call WaitForExitAsync after the loop so + // CurrentProcess.ExitCode is safe to read. + await foreach (ProcessOutputLine line in CurrentProcess.ReadAllLinesAsync().ConfigureAwait(false)) + { + if (isDisposed) + break; + + string msg = $"[{_label}] {line.Content}"; + output.Add(msg); + _testOutput.WriteLine(msg); + + if (line.StandardError) + _onErrorLine?.Invoke(line.Content); + else + _onOutputLine?.Invoke(line.Content); + } + + await CurrentProcess.WaitForExitAsync().ConfigureAwait(false); } catch (Exception ex) { - // If process start (inside of StartAndWaitForExitAsync) fails, - // the `Process` object is in a state "don't touch me" + // If process start fails, the `Process` object is in a state "don't touch me" // (calling almost everything results in "No process associated with this object"), // therefore we just set it to null to avoid hiding the root exception. CurrentProcess = null; @@ -155,9 +151,6 @@ private async Task ExecuteAsyncInternal(string executable, string throw; } - CurrentProcess.ErrorDataReceived -= errorHandler; - CurrentProcess.OutputDataReceived -= outputHandler; - RemoveNullTerminator(output); return new CommandResult(