Fix infrequent hangs in test-runner. (#16793)

* Fix infrequent hangs in test-runner.

test-runner has been plagued with very infrequent hangs causing long
timeouts on CI. Turns out that there is a race in AsyncStreamReader
used by Process class for handling stdout/stderr when closing down
process. Since AsyncStreamReader::Dispose didn't make sure no pending
async read requests were still in flight before closing the stream
and underlying handle, it led to a race condition deadlocking the
complete process, hanging test-runner. A similar race has also been
observed but instead of causing a deadlock, it could also manifest as an
System.ObjectDisposedException when doing EndRead while another thread
is disposing the underlying stream. Hitting that race didn't deadlock
the process but failed to complete invoke of test-runner.

Fix is to better protect the async callback and the thread doing the
dispose call. In order to make sure we don't close underlying handle
while still having async reads in flight, fix also checks async result
for completion and if not completed when trying to dispose AsyncStreamReader
it will try to cancel outstanding pending IO and wait for completion
before closing the stream.

Since the issue has only been seen on Windows, the fix will try
to cancel pending IO and wait for completion before closing stream
on Windows. On other platforms, old behavior will be preserved, meaning
that the implementation will still be vunarable to race between pending
reads and close of stream, but since problem has not been observed on
other platforms and since there is no good way of cancel outstanding
blocking reads, old behavior is kept.

At some point in future we should look into replacing the implementation
of AsyncStreamReader and use cancelation tokens, but since we don't have
all corefx support around cancelation in Mono BCL, that currently won't
solve the issue, meaning that we need to bring in more classes. Since the
problem also occur in test-runner using the build BCL profile, even more
needs to be added to that profile in order to get similar support. But due
to the infrequency of the problem and that it has only been observed on
Windows, the isolated fix seems more reasonable.

* Build error fix.

* Review feedback.

* Fix type cast not working on linux builds.

* Adjust icall naming in icall-def.h.

1 files changed, 47 insertions, 14 deletions

diff --git a/mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs b/mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs
index 7be029f7e81..8260061e8d8 100644
--- a/mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs
+++ b/mcs/class/referencesource/System/services/monitoring/system/diagnosticts/AsyncStreamReader.cs
@@ -19,8 +19,9 @@ namespace System.Diagnostics {
     using System.IO;
     using System.Text;
     using System.Runtime.InteropServices;
-    using System.Threading;    
-    using System.Collections;    
+    using System.Threading;
+    using System.Collections;
+    using Microsoft.Win32.SafeHandles;
         
     internal delegate void UserCallBack(String data);
  
@@ -58,10 +59,10 @@ namespace System.Diagnostics {
 
         // Cache the last position scanned in sb when searching for lines.
         private int currentLinePos;
-		
 #if MONO
-		//users to coordinate between Dispose and BeginReadLine
-		private object syncObject = new Object ();
+        //users to coordinate between Dispose and ReadBuffer
+        private object syncObject = new Object ();
+        private IAsyncResult asyncReadResult = null;
 #endif
 
         internal AsyncStreamReader(Process process, Stream stream, UserCallBack callback, Encoding encoding) 
@@ -115,8 +116,31 @@ namespace System.Diagnostics {
             lock (syncObject) {
 #endif
             if (disposing) {
-                if (stream != null)
+                if (stream != null) {
+#if MONO
+                    if (asyncReadResult != null && !asyncReadResult.IsCompleted) {
+                        // Closing underlying stream when having pending async read request in progress is
+                        // not portable and racy by design. Try to cancel pending async read before closing stream.
+                        // We are still holding lock that will prevent new async read requests to queue up
+                        // before we have closed and invalidated the stream.
+                        if (stream is FileStream) {
+                            SafeHandle tmpStreamHandle = ((FileStream)stream).SafeFileHandle;
+                            while (!asyncReadResult.IsCompleted) {
+                                MonoIOError error;
+                                if (!MonoIO.Cancel (tmpStreamHandle, out error) && error == MonoIOError.ERROR_NOT_SUPPORTED) {
+                                    // Platform don't support canceling pending IO requests on stream. If an async pending read
+                                    // is still in flight when closing stream, it could trigger a race condition.
+                                    break;
+                                }
+
+                                // Wait for a short time for pending async read to cancel/complete/fail.
+                                asyncReadResult.AsyncWaitHandle.WaitOne (200);
+                            }
+                        }
+                    }
+#endif
                     stream.Close();
+                }
             }
             if (stream != null) {
                 stream = null;
@@ -151,7 +175,11 @@ namespace System.Diagnostics {
             
             if( sb == null ) {
                 sb = new StringBuilder(DefaultBufferSize);
+#if MONO
+                asyncReadResult = stream.BeginRead (byteBuffer, 0 , byteBuffer.Length,  new AsyncCallback (ReadBuffer), null);
+#else
                 stream.BeginRead(byteBuffer, 0 , byteBuffer.Length,  new AsyncCallback(ReadBuffer), null);
+#endif
             }
             else {
                 FlushMessageQueue();
@@ -169,12 +197,17 @@ namespace System.Diagnostics {
             
             try {
 #if MONO
-                var stream = this.stream;
-                if (stream == null)
-                byteLen = 0;
-                else
-#endif
+                lock (syncObject) {
+                    Debug.Assert (ar.IsCompleted);
+                    asyncReadResult = null;
+                    if (this.stream == null)
+                        byteLen = 0;
+                    else
+                        byteLen = stream.EndRead (ar);
+                }
+#else
                 byteLen = stream.EndRead(ar);
+#endif
             }
             catch (IOException ) {
                 // We should ideally consume errors from operations getting cancelled
@@ -242,10 +275,10 @@ retry_dispose:
                         byteLen = 0;
                         goto retry_dispose;
                     }
-#endif
-                stream.BeginRead(byteBuffer, 0 , byteBuffer.Length,  new AsyncCallback(ReadBuffer), null);
-#if MONO
+                    asyncReadResult = stream.BeginRead (byteBuffer, 0 , byteBuffer.Length,  new AsyncCallback(ReadBuffer), null);
                 }
+#else
+                stream.BeginRead(byteBuffer, 0 , byteBuffer.Length,  new AsyncCallback(ReadBuffer), null);
 #endif
             }
         }