Dispose of process streams if they are not referenced by the user. (#26204)

If the process pipes for stdin/stdout/stderr are not touched by the user, dispose of them.

Partial fix to #25962

Author: strajkovmsft

src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs

@@ -67,6 +67,8 @@ public partial class Process : Component
 
         private static object s_createProcessLock = new object();
 
+        private bool _standardInputAccessed;
+
         private StreamReadMode _outputStreamReadMode;
         private StreamReadMode _errorStreamReadMode;
 
@@ -691,6 +693,7 @@ public StreamWriter StandardInput
                     throw new InvalidOperationException(SR.CantGetStandardIn);
                 }
 
+                _standardInputAccessed = true;
                 return _standardInput;
             }
         }
@@ -846,18 +849,45 @@ public void Close()
                 _machineName = ".";
                 _raisedOnExited = false;
 
-                //Don't call close on the Readers and writers
-                //since they might be referenced by somebody else while the 
-                //process is still alive but this method called.
-                _standardOutput = null;
-                _standardInput = null;
-                _standardError = null;
+                // Only call close on the streams if the user cannot have a reference on them.
+                // If they are referenced it is the user's responsibility to dispose of them.
+                try 
+                {
+                    if (_standardOutput != null && (_outputStreamReadMode == StreamReadMode.AsyncMode || _outputStreamReadMode == StreamReadMode.Undefined))
+                    {
+                        if (_outputStreamReadMode == StreamReadMode.AsyncMode)
+                        {
+                            _output.CancelOperation();
+                        }
+                        _standardOutput.Close();
+                    }
 
-                _output = null;
-                _error = null;
+                    if (_standardError != null && (_errorStreamReadMode == StreamReadMode.AsyncMode || _errorStreamReadMode == StreamReadMode.Undefined))
+                    {
+                        if (_errorStreamReadMode == StreamReadMode.AsyncMode)
+                        {
+                            _error.CancelOperation();
+                        }
+                        _standardError.Close();
+                    }
+
+                    if (_standardInput != null && !_standardInputAccessed)
+                    {
+                        _standardInput.Close();
+                    }
+                }
+                finally 
+                {
+                    _standardOutput = null;
+                    _standardInput = null;
+                    _standardError = null;
 
-                CloseCore();
-                Refresh();
+                    _output = null;
+                    _error = null;
+
+                    CloseCore();
+                    Refresh();
+                }
             }
         }
 

src/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs

@@ -228,6 +228,57 @@ public void TestManyOutputLines()
             Assert.Equal(ExpectedLineCount + 1, totalLinesReceived);
         }
 
+        [Fact]
+        public void TestClosingStreamsAsyncDoesNotThrow()
+        {
+            Process p = CreateProcessPortable(RemotelyInvokable.WriteLinesAfterClose);
+            p.StartInfo.RedirectStandardOutput = true;
+            p.StartInfo.RedirectStandardError = true;
+
+            // On netfx, the handler is called once with the Data as null, even if the process writes nothing to the pipe.
+            // That behavior is documented here https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.datareceivedeventhandler
+            p.OutputDataReceived += (s, e) => { Assert.True(PlatformDetection.IsFullFramework && e.Data == null, "OutputDataReceived called after closing the process"); };
+            p.ErrorDataReceived += (s, e) => { Assert.True(PlatformDetection.IsFullFramework && e.Data == null, "ErrorDataReceived called after closing the process"); };
+
+            p.Start();
+            p.BeginOutputReadLine();
+            p.BeginErrorReadLine();
+
+            p.Close();
+            RemotelyInvokable.FireClosedEvent();
+        }
+
+        [Fact]
+        public void TestClosingStreamsUndefinedDoesNotThrow()
+        {
+            Process p = CreateProcessPortable(RemotelyInvokable.WriteLinesAfterClose);
+            p.StartInfo.RedirectStandardOutput = true;
+            p.StartInfo.RedirectStandardError = true;
+
+            p.Start();
+            p.Close();
+            RemotelyInvokable.FireClosedEvent();
+        }
+
+        [Fact]
+        public void TestClosingSyncModeDoesNotCloseStreams()
+        {
+            Process p = CreateProcessPortable(RemotelyInvokable.WriteLinesAfterClose);
+            p.StartInfo.RedirectStandardOutput = true;
+            p.StartInfo.RedirectStandardError = true;
+
+            p.Start();
+
+            var output = p.StandardOutput;
+            var error = p.StandardError;
+
+            p.Close();
+            RemotelyInvokable.FireClosedEvent();
+
+            output.ReadToEnd();
+            error.ReadToEnd();
+        }
+
         [Fact]
         public void TestStreamNegativeTests()
         {

src/System.Diagnostics.Process/tests/RemotelyInvokable.cs

@@ -22,6 +22,8 @@ internal static class RemotelyInvokable
         public static readonly int SuccessExitCode = 42;
         public const int WaitInMS = 30 * 1000;
         public const string TestConsoleApp = "System.Diagnostics.Process.Tests";
+        public static event EventHandler ClosedEvent;
+
 
         public static string DummyUapCmd()
         {
@@ -161,6 +163,26 @@ public static int Write144Lines()
             return SuccessExitCode;
         }
 
+        public static int WriteLinesAfterClose()
+        {
+            ClosedEvent += (s, e) =>
+            {
+                Console.WriteLine("This is a line to output.");
+                Console.Error.WriteLine("This is a line to error.");
+            };
+            return SuccessExitCode;
+        }
+
+        public static string WriteLinesAfterCloseUapCmd()
+        {
+            ClosedEvent += (s, e) =>
+            {
+                // Finish the pause
+                Console.WriteLine();
+            };
+            return "(pause > nul) & (echo This is a line to output) & (echo This is a line to error 1>&2)";
+        }
+
         public static string ConcatThreeArgumentsUapCmd(string one, string two, string three)
         {
             return $"echo {string.Join(",", one, two, three)} & exit {SuccessExitCode}";
@@ -182,5 +204,10 @@ public static int SelfTerminate()
             Process.GetCurrentProcess().Kill();
             throw new ShouldNotBeInvokedException();
         }
+
+        public static void FireClosedEvent()
+        {
+            ClosedEvent?.Invoke(null, EventArgs.Empty);
+        }
     }
 }