[vs18.8] Revert "Fix ToolTask hang when grandchild processes inherit pipe handles (#13351)"

azure-pipelines/vs-insertion.yml

@@ -61,7 +61,7 @@ variables:
     ${{ if not(eq(parameters.TargetBranch, 'auto')) }}:
       value: ${{ parameters.TargetBranch }}
     ${{ else }}:
-      value: 'main'
+      value: 'rel/insiders'
   - name:  TeamName
     value: msbuild
   - name: TeamEmail

documentation/wiki/ChangeWaves.md

@@ -41,7 +41,6 @@ Change wave checks around features will be removed in the release that accompani
 - [AbsolutePath.GetCanonicalForm optimization - avoid expensive Path.GetFullPath calls when paths don't need canonicalization](https://github.com/dotnet/msbuild/pull/13369)
 - [TaskHostTask forwards request-level global properties (e.g. MSBuildRestoreSessionId) to out-of-proc TaskHost in -mt mode](https://github.com/dotnet/msbuild/pull/13443)
 - [Fix ShouldTreatWarningAsError in OOP TaskHost checking wrong collection (WarningsAsMessages instead of WarningsAsErrors)](https://github.com/dotnet/msbuild/issues/11952)
-- [Fix ToolTask hang when tool spawns grandchild processes that inherit stdout/stderr pipe handles](https://github.com/dotnet/msbuild/issues/2981)
 
 ### 18.5
 - [FindUnderPath and AssignTargetPath tasks no longer throw on invalid path characters when using TaskEnvironment.GetAbsolutePath](https://github.com/dotnet/msbuild/pull/13069)

eng/Versions.props

@@ -3,7 +3,7 @@
   <Import Project="Version.Details.props" />
 
   <PropertyGroup>
-    <VersionPrefix>18.8.1</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind><!-- Keep next to VersionPrefix to create a conflict in forward-flow -->
+    <VersionPrefix>18.8.2</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind><!-- Keep next to VersionPrefix to create a conflict in forward-flow -->
     <PreReleaseVersionLabel>servicing</PreReleaseVersionLabel>
     <PackageValidationBaselineVersion>18.7.0-preview-26230-02</PackageValidationBaselineVersion>
     <AssemblyVersion>15.1.0.0</AssemblyVersion>

src/Utilities.UnitTests/ToolTask_Tests.cs

@@ -1112,68 +1112,6 @@ public void ToolTaskThatTimeoutAndRetry(int repeats, bool timeoutOnFirstExecutio
             }
         }
 
-        /// <summary>
-        /// Verifies that ToolTask does not hang when the tool process spawns a grandchild
-        /// process that inherits stdout/stderr pipe handles and outlives the tool.
-        /// This is a regression test for https://github.com/dotnet/msbuild/issues/2981.
-        /// </summary>
-        [Fact]
-        public void ToolTaskDoesNotHangWhenGrandchildInheritsPipeHandles()
-        {
-            using (MyTool t = new MyTool())
-            {
-                MockEngine3 engine = new MockEngine3();
-                t.BuildEngine = engine;
-
-                // cmd echoes "hello", then starts a background ping that inherits
-                // pipe handles. cmd exits immediately; ping outlives the 2s EOF timeout.
-                t.MockCommandLineCommands = NativeMethodsShared.IsWindows
-                    ? "/c echo hello & start /b ping -n 10 127.0.0.1 > nul"
-                    : "-c \"echo hello; sleep 10 &\"";
-
-                // Set a generous timeout - without the fix this would hang for the full ping duration
-                t.Timeout = 30000;
-
-                bool result = t.Execute();
-
-                // The tool should complete without hanging.
-                // The exit code may be non-zero depending on timing, but the key thing
-                // is that Execute() returns at all rather than hanging forever.
-                _output.WriteLine(engine.Log);
-                engine.Log.ShouldContain("hello");
-            }
-        }
-
-        /// <summary>
-        /// Verifies that ToolTask still captures all output from the tool process
-        /// even with the grandchild pipe fix enabled. This is a regression test for
-        /// https://github.com/dotnet/msbuild/issues/10378 where switching to
-        /// WaitForExit(int) caused output to be lost.
-        /// </summary>
-        [Fact]
-        public void ToolTaskCapturesAllOutputWithFix()
-        {
-            using (MyTool t = new MyTool())
-            {
-                MockEngine3 engine = new MockEngine3();
-                t.BuildEngine = engine;
-
-                // Echo multiple lines to verify all output is captured
-                t.MockCommandLineCommands = NativeMethodsShared.IsWindows ?
-                    "/c echo line1 & echo line2 & echo line3"
-                    : "-c \"echo line1; echo line2; echo line3\"";
-
-                bool result = t.Execute();
-
-                _output.WriteLine(engine.Log);
-
-                result.ShouldBeTrue();
-                engine.Log.ShouldContain("line1");
-                engine.Log.ShouldContain("line2");
-                engine.Log.ShouldContain("line3");
-            }
-        }
-
         /// <summary>
         /// A simple implementation of <see cref="ToolTask"/> to sleep for a while.
         /// </summary>

src/Utilities/ToolTask.cs

@@ -766,12 +766,6 @@ protected virtual int ExecuteTool(
             _standardErrorDataAvailable = new ManualResetEvent(false);
             _standardOutputDataAvailable = new ManualResetEvent(false);
 
-            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_6))
-            {
-                _standardOutputEOF = new ManualResetEvent(false);
-                _standardErrorEOF = new ManualResetEvent(false);
-            }
-
             _toolExited = new ManualResetEvent(false);
             _terminatedTool = false;
             _toolTimeoutExpired = new ManualResetEvent(false);
@@ -865,9 +859,6 @@ protected virtual int ExecuteTool(
                     _standardErrorDataAvailable.Dispose();
                     _standardOutputDataAvailable.Dispose();
 
-                    _standardOutputEOF?.Dispose();
-                    _standardErrorEOF?.Dispose();
-
                     _toolExited.Dispose();
                     _toolTimeoutExpired.Dispose();
 
@@ -1106,40 +1097,13 @@ private void TerminateToolProcess(Process proc, bool isBeingCancelled)
         /// process is still finishing up, this method waits until it is done.
         /// </summary>
         /// <remarks>
-        /// On both .NET Framework and modern .NET, the parameterless Process.WaitForExit() waits not
-        /// only for the process to exit, but also for stdout/stderr pipe EOF via
-        /// AsyncStreamReader.WaitUtilEOF() (Framework) or awaiting the EOF task (Core).
-        /// If the tool spawned child processes that inherited the pipe handles, the EOF wait blocks
-        /// forever even though the tool itself has exited — causing the entire build node to hang.
+        /// This method is a hack, but it needs to be called after both
+        /// Process.WaitForExit() and Process.Kill().
         /// </remarks>
         /// <param name="proc"></param>
-        private void WaitForProcessExit(Process proc)
+        private static void WaitForProcessExit(Process proc)
         {
-            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave18_6))
-            {
-                // Step 1: Wait for the process handle to be signaled.
-                // Use int.MaxValue to avoid blocking on pipe EOF,
-                // as Process.WaitForExit does not wait for EOF when any timeout is provided.
-                proc.WaitForExit(int.MaxValue);
-
-                // Step 2: Wait for the AsyncStreamReader to deliver all remaining data.
-                // When the pipe reaches EOF, AsyncStreamReader flushes its StringBuilder
-                // (delivering any final partial line) and sends Data=null via the callback.
-                // Our ReceiveStandardErrorOrOutputData handler signals the EOF events.
-                //
-                // Use a bounded timeout as a safety net for the grandchild case where
-                // EOF never arrives because grand child inherited the pipe and keeps it open.
-                const int eofTimeoutSec = 2;
-
-                WaitHandle[] eofEvents = [_standardOutputEOF, _standardErrorEOF];
-                WaitHandle.WaitAll(eofEvents, TimeSpan.FromSeconds(eofTimeoutSec));
-            }
-            else
-            {
-                // Legacy behavior: parameterless WaitForExit waits for pipe EOF.
-                // This can hang if grandchild processes hold pipe handles.
-                proc.WaitForExit();
-            }
+            proc.WaitForExit();
 
             // Process.WaitForExit() may return prematurely. We need to check to be sure.
             while (!proc.HasExited)
@@ -1305,54 +1269,38 @@ protected void ReceiveExitNotification(object sender, EventArgs e)
         /// <param name="dataAvailableSignal"></param>
         private void ReceiveStandardErrorOrOutputData(DataReceivedEventArgs e, Queue dataQueue, ManualResetEvent dataAvailableSignal)
         {
-            if (e.Data == null)
+            // NOTE: don't ignore empty string, because we need to log that
+            if (e.Data != null)
             {
-                // The AsyncStreamReader sends Data=null when the pipe reaches EOF.
-                // Signal the appropriate EOF event so WaitForProcessExit knows
-                // all data from this stream has been delivered.
-                ManualResetEvent eofEvent = (dataQueue == _standardErrorData) ? _standardErrorEOF : _standardOutputEOF;
-                if (eofEvent != null)
+                ErrorUtilities.VerifyThrow(dataQueue != null,
+                    "The data queue must be available.");
+
+                // synchronize access to the queue -- this is a producer-consumer problem
+                // NOTE: we lock the entire queue instead of using synchronized queue
+                // wrappers, because ManualResetEvents don't have ref counts, and it's
+                // difficult to discretely signal the availability of each instance of
+                // data in the queue -- so instead we let the consumer lock and empty
+                // the queue and reset the ManualResetEvent, before we add more data
+                // into the queue, and signal the ManualResetEvent again
+                lock (dataQueue.SyncRoot)
                 {
+                    dataQueue.Enqueue(e.Data);
+
+                    ErrorUtilities.VerifyThrow(dataAvailableSignal != null,
+                        "The signalling event must be available.");
+
+                    // signal the availability of data
+                    // NOTE: intentionally, do the signalling inside the lock, because
+                    // ManualResetEvents don't have ref counts, and we want to make sure
+                    // we don't signal the notification just before the consumer resets it
                     lock (_eventCloseLock)
                     {
                         if (!_eventsDisposed)
                         {
-                            eofEvent.Set();
+                            dataAvailableSignal.Set();
                         }
                     }
                 }
-
-                return;
-            }
-
-            // NOTE: don't ignore empty string, because we need to log that
-            ErrorUtilities.VerifyThrow(dataQueue != null, "The data queue must be available.");
-
-            // synchronize access to the queue -- this is a producer-consumer problem
-            // NOTE: we lock the entire queue instead of using synchronized queue
-            // wrappers, because ManualResetEvents don't have ref counts, and it's
-            // difficult to discretely signal the availability of each instance of
-            // data in the queue -- so instead we let the consumer lock and empty
-            // the queue and reset the ManualResetEvent, before we add more data
-            // into the queue, and signal the ManualResetEvent again
-            lock (dataQueue.SyncRoot)
-            {
-                dataQueue.Enqueue(e.Data);
-
-                ErrorUtilities.VerifyThrow(dataAvailableSignal != null,
-                    "The signalling event must be available.");
-
-                // signal the availability of data
-                // NOTE: intentionally, do the signalling inside the lock, because
-                // ManualResetEvents don't have ref counts, and we want to make sure
-                // we don't signal the notification just before the consumer resets it
-                lock (_eventCloseLock)
-                {
-                    if (!_eventsDisposed)
-                    {
-                        dataAvailableSignal.Set();
-                    }
-                }
             }
         }
 
@@ -1860,18 +1808,6 @@ private bool LogEnvironmentVariable(bool alreadyLoggedEnvironmentHeader, string
         /// </summary>
         private bool _eventsDisposed;
 
-        /// <summary>
-        /// Signalled when the stdout AsyncStreamReader reaches EOF (sends Data=null).
-        /// Used by WaitForProcessExit to know when all stdout data has been delivered.
-        /// </summary>
-        private ManualResetEvent _standardOutputEOF;
-
-        /// <summary>
-        /// Signalled when the stderr AsyncStreamReader reaches EOF (sends Data=null).
-        /// Used by WaitForProcessExit to know when all stderr data has been delivered.
-        /// </summary>
-        private ManualResetEvent _standardErrorEOF;
-
         /// <summary>
         /// List of name, value pairs to be passed to the spawned tool's environment.
         /// May be null.