[Microsoft.Build] Fix XS build error

We observe the following error when building XS:

"Threadpool worker" tid=0x0x7f011bfff700 this=0x0x7f01394bfb30 thread handle 0x432 state : not waiting owns ()
  at <unknown> <0xffffffff>
  at (wrapper managed-to-native) System.Threading.WaitHandle.WaitAll_internal (System.Threading.WaitHandle[],int,bool) <0x0005d>
  at System.Threading.WaitHandle.WaitAll (System.Threading.WaitHandle[]) <0x00026>
  at Microsoft.Build.Utilities.ProcessWrapper.<Start>m__0 (object,System.EventArgs) <0x0008f>
  at System.Diagnostics.Process.OnExited () <0x000ed>
  at System.Diagnostics.Process.RaiseOnExited () <0x0009f>
  at System.Diagnostics.Process.CompletionCallback (object,bool) <0x00017>
  at System.Threading.RegisteredWaitHandle.DoCallBack (object) <0x00088>
  at System.Threading.QueueUserWorkItemCallback.WaitCallback_Context (object) <0x00058>
  at System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext,System.Threading.ContextCallback,object,bool) <0x001c6>
  at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext,System.Threading.ContextCallback,object,bool) <0x00020>
  at System.Threading.QueueUserWorkItemCallback.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem () <0x0004c>
  at System.Threading.ThreadPoolWorkQueue.Dispatch () <0x001d6>
  at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback () <0x00008>
  at (wrapper runtime-invoke) <Module>.runtime_invoke_bool (object,intptr,intptr,intptr) <0x0005a>

And the process output would contains a bunch of "_wapi_handle_ref: Attempting to ref unused handle 0x1234" and "_wapi_handle_unref_full: Attempting to unref unused handle 0x1234".

The expected behaviour is for WaitHandle.(WaitAll|WaitAny|WaitOne|SignalAndWait) and EventWaitHandle.Set to throw an ObjectDisposedException in case the WaitHandle (or one of them if calling WaitAll, WaitAny or SignalAndWait) have been disposed.

This error would be due to a race between ProcessWrapper.Dispose, which closes endEventOut and endEventErr, and the above Process.Wrapper.<Start>m__0 (the base.Exited) callback, which waits on endEventOut and endEventErr.

diff --git a/mcs/class/Microsoft.Build.Utilities/Microsoft.Build.Utilities/ProcessWrapper.cs b/mcs/class/Microsoft.Build.Utilities/Microsoft.Build.Utilities/ProcessWrapper.cs
index d78c1eba66d1ac49a666544dfcf9612a953fb5ae..1b9dd8a9d13415fa3dfab85df3809b6dea9751d2 100644 (file)
--- a/mcs/class/Microsoft.Build.Utilities/Microsoft.Build.Utilities/ProcessWrapper.cs
+++ b/mcs/class/Microsoft.Build.Utilities/Microsoft.Build.Utilities/ProcessWrapper.cs
@@ -13,6 +13,7 @@ namespace Microsoft.Build.Utilities
                ManualResetEvent endEventErr = new ManualResetEvent (false);
                ManualResetEvent endEventExit = new ManualResetEvent (false);
                bool done;
+               bool disposed;
                object lockObj = new object ();
 
                public ProcessWrapper ()
@@ -26,15 +27,23 @@ namespace Microsoft.Build.Utilities
                        base.EnableRaisingEvents = true;
 
                        base.Exited += (s, args) => {
-                               endEventExit.Set ();
+                               try {
+                                       endEventExit.Set ();
+                                       WaitHandle.WaitAll (new WaitHandle[] { endEventOut, endEventErr });
+                               } catch (ObjectDisposedException) {
+                                       return; // we already called Dispose
+                               }
 
-                               WaitHandle.WaitAll (new WaitHandle[] { endEventOut, endEventErr });
                                OnExited (this, EventArgs.Empty);
                        };
 
                        base.OutputDataReceived += (s, args) => {
                                if (args.Data == null) {
-                                       endEventOut.Set ();
+                                       try {
+                                               endEventOut.Set ();
+                                       } catch (ObjectDisposedException) {
+                                               return; // we already called Dispose
+                                       }
                                } else {
                                        ProcessEventHandler handler = OutputStreamChanged;
                                        if (handler != null)
@@ -44,7 +53,11 @@ namespace Microsoft.Build.Utilities
 
                        base.ErrorDataReceived += (s, args) => {
                                if (args.Data == null) {
-                                       endEventErr.Set ();
+                                       try {
+                                               endEventErr.Set ();
+                                       } catch (ObjectDisposedException) {
+                                               return; // we already called Dispose
+                                       }
                                } else {
                                        ProcessEventHandler handler = ErrorStreamChanged;
                                        if (handler != null)
@@ -71,24 +84,28 @@ namespace Microsoft.Build.Utilities
 
                protected override void Dispose (bool disposing)
                {
-                       lock (lockObj) {
-                               if (endEventOut == null)
-                                       return;
-                       }
+                       if (disposed)
+                               return;
 
                        if (!done)
                                ((IAsyncOperation)this).Cancel ();
 
+                       // if we race with base.Exited, we don't want to hang on WaitAll (endEventOut, endEventErr)
+                       endEventOut.Set ();
+                       endEventErr.Set ();
+
                        endEventOut.Close ();
                        endEventErr.Close ();
                        endEventExit.Close ();
 
+                       disposed = true;
+
                        base.Dispose (disposing);
                }
 
                void CheckDisposed ()
                {
-                       if (endEventOut == null)
+                       if (disposed)
                                throw new ObjectDisposedException ("ProcessWrapper");
                }