Merge pull request #1596 from microsoft/dev/waan/2987836

Fix UI-thread hang when resuming the target during launch

Author: WardenGnaw

src/MIDebugEngine/AD7.Impl/AD7Engine.cs

@@ -319,28 +319,24 @@ public int ContinueFromSynchronousEvent(IDebugEvent2 eventObject)
             {
                 if (eventObject is AD7ProgramCreateEvent)
                 {
-                    Exception exception = null;
-
                     try
                     {
                         _engineCallback.OnLoadComplete();
-                        // At this point breakpoints and exception settings have been sent down, so we can resume the target
-                        _pollThread.RunOperation(() =>
-                        {
-                            return _debuggedProcess.ResumeFromLaunch();
-                        });
+
+                        // Resume the target on the worker thread without blocking the UI thread this runs on.
+                        // Resume faults are reported via onError, since the SDM drops errors returned from here.
+                        _pollThread.PostAsyncOperation(
+                            () => _debuggedProcess.ResumeFromLaunch(),
+                            (exception) =>
+                            {
+                                SendStartDebuggingError(exception);
+                                _debuggedProcess.Terminate();
+                            });
                     }
                     catch (Exception e)
                     {
-                        exception = e;
-                        // Return from the catch block so that we can let the exception unwind - the stack can get kind of big
-                    }
-
-                    if (exception != null)
-                    {
-                        // If something goes wrong, report the error and then stop debugging. The SDM will drop errors
-                        // from ContinueFromSynchronousEvent, so we want to deal with them ourself.
-                        SendStartDebuggingError(exception);
+                        // Report synchronous failures ourselves, since the SDM drops errors returned from here.
+                        SendStartDebuggingError(e);
                         _debuggedProcess.Terminate();
                     }
 

src/MIDebugEngine/Engine.Impl/OperationThread.cs

@@ -28,6 +28,7 @@ internal class WorkerThread : IDisposable
         private readonly ManualResetEvent _runningOpCompleteEvent; // fired when either m_syncOp finishes, or the kick off of m_async
         private readonly Object _eventLock = new object(); // Locking on an event directly can cause Mono to stop responding.
         private readonly Queue<Operation> _postedOperations; // queue of fire-and-forget operations
+        private readonly Queue<(AsyncOperation Operation, Action<Exception> OnError)> _postedAsyncOperations; // queue of fire-and-forget async operations
 
         public event EventHandler<Exception> PostedOperationErrorEvent;
 
@@ -37,6 +38,13 @@ private class OperationDescriptor
             /// Delegate that was added via 'RunOperation'. Is of type 'Operation' or 'AsyncOperation'
             /// </summary>
             public readonly Delegate Target;
+
+            /// <summary>
+            /// Handler invoked if the operation faults. For async operations, this may be invoked on a non-worker thread.
+            /// Only set for PostAsyncOperation.
+            /// </summary>
+            public Action<Exception> ErrorHandler;
+
             public ExceptionDispatchInfo ExceptionDispatchInfo;
             public Task Task;
             private bool _isStarted;
@@ -81,6 +89,7 @@ public WorkerThread(Logger logger)
             _opSet = new AutoResetEvent(false);
             _runningOpCompleteEvent = new ManualResetEvent(true);
             _postedOperations = new Queue<Operation>();
+            _postedAsyncOperations = new Queue<(AsyncOperation Operation, Action<Exception> OnError)>();
 
             _thread = new Thread(new ThreadStart(ThreadFunc));
             _thread.Name = "MIDebugger.PollThread";
@@ -121,6 +130,30 @@ public void RunOperation(string text, CancellationTokenSource canTokenSource, As
             SetOperationInternalWithProgress(op, text, canTokenSource);
         }
 
+        /// <summary>
+        /// Queue an async operation to run on the worker thread and return immediately, without waiting for the
+        /// operation to start or finish. Posted async operations run one at a time, in the order posted, and
+        /// serialize behind any in-flight operation. Faults are reported to <paramref name="onError"/>.
+        /// </summary>
+        public void PostAsyncOperation(AsyncOperation op, Action<Exception> onError)
+        {
+            if (op == null)
+                throw new ArgumentNullException(nameof(op));
+            if (onError == null)
+                throw new ArgumentNullException(nameof(onError));
+
+            if (_isClosed)
+                throw new ObjectDisposedException("WorkerThread");
+
+            lock (_postedAsyncOperations)
+            {
+                if (_isClosed)
+                    throw new ObjectDisposedException("WorkerThread");
+
+                _postedAsyncOperations.Enqueue((op, onError));
+                _opSet.Set();
+            }
+        }
 
         public void Close()
         {
@@ -150,6 +183,22 @@ public void Close()
                     }
                 }
             }
+
+            // Fail any queued async operations that will never run now that we are closed, instead of
+            // dropping them silently. The poll thread won't promote them once _isClosed is set.
+            while (true)
+            {
+                Action<Exception> onError;
+                lock (_postedAsyncOperations)
+                {
+                    if (_postedAsyncOperations.Count == 0)
+                        break;
+
+                    onError = _postedAsyncOperations.Dequeue().OnError;
+                }
+
+                InvokeErrorHandler(onError, new ObjectDisposedException("WorkerThread"));
+            }
         }
 
         internal void SetOperationInternal(Delegate op)
@@ -189,6 +238,7 @@ internal void SetOperationInternalWithProgress(AsyncProgressOperation op, string
                 }
             }
         }
+
         public void PostOperation(Operation op)
         {
             if (op == null)
@@ -212,71 +262,154 @@ public void PostOperation(Operation op)
 
         private bool TrySetOperationInternal(Delegate op)
         {
-            lock (_eventLock)
+            bool claimed = false;
+            try
             {
-                if (_isClosed)
-                    throw new ObjectDisposedException("WorkerThread");
-
-                if (_runningOp == null)
+                lock (_eventLock)
                 {
-                    _runningOpCompleteEvent.Reset();
+                    if (_isClosed)
+                        throw new ObjectDisposedException("WorkerThread");
 
-                    OperationDescriptor runningOp = new OperationDescriptor(op);
-                    _runningOp = runningOp;
+                    if (_runningOp == null)
+                    {
+                        _runningOpCompleteEvent.Reset();
 
-                    _opSet.Set();
+                        OperationDescriptor runningOp = new OperationDescriptor(op);
+                        _runningOp = runningOp;
 
-                    _runningOpCompleteEvent.WaitOne();
+                        _opSet.Set();
 
-                    Debug.Assert(runningOp.IsComplete, "Why isn't the running op complete?");
+                        _runningOpCompleteEvent.WaitOne();
+                        claimed = true;
 
-                    if (runningOp.ExceptionDispatchInfo != null)
-                    {
-                        runningOp.ExceptionDispatchInfo.Throw();
+                        Debug.Assert(runningOp.IsComplete, "Why isn't the running op complete?");
+
+                        if (runningOp.ExceptionDispatchInfo != null)
+                        {
+                            runningOp.ExceptionDispatchInfo.Throw();
+                        }
+
+                        return true;
                     }
+                }
 
-                    return true;
+                return false;
+            }
+            finally
+            {
+                // The running-op slot was just freed and _eventLock is now released. If promotion on the poll
+                // thread lost the TryEnter race against this method, re-arm _opSet so a queued async operation
+                // is promoted immediately instead of stranded until the next unrelated wakeup.
+                if (claimed && HasPostedAsyncOperation())
+                {
+                    _opSet.Set();
                 }
             }
-
-            return false;
         }
 
         private bool TrySetOperationInternalWithProgress(AsyncProgressOperation op, string text, CancellationTokenSource canTokenSource)
         {
             var waitLoop = new HostWaitLoop(text);
 
-            lock (_eventLock)
+            bool claimed = false;
+            try
             {
-                if (_isClosed)
-                    throw new ObjectDisposedException("WorkerThread");
-
-                if (_runningOp == null)
+                lock (_eventLock)
                 {
-                    _runningOpCompleteEvent.Reset();
+                    if (_isClosed)
+                        throw new ObjectDisposedException("WorkerThread");
 
-                    OperationDescriptor runningOp = new OperationDescriptor(new AsyncOperation(() => { return op(waitLoop); }));
-                    _runningOp = runningOp;
+                    if (_runningOp == null)
+                    {
+                        _runningOpCompleteEvent.Reset();
 
-                    _opSet.Set();
+                        OperationDescriptor runningOp = new OperationDescriptor(new AsyncOperation(() => { return op(waitLoop); }));
+                        _runningOp = runningOp;
 
-                    waitLoop.Wait(_runningOpCompleteEvent, canTokenSource);
+                        _opSet.Set();
 
-                    Debug.Assert(runningOp.IsComplete, "Why isn't the running op complete?");
+                        waitLoop.Wait(_runningOpCompleteEvent, canTokenSource);
+                        claimed = true;
 
-                    if (runningOp.ExceptionDispatchInfo != null)
-                    {
-                        runningOp.ExceptionDispatchInfo.Throw();
+                        Debug.Assert(runningOp.IsComplete, "Why isn't the running op complete?");
+
+                        if (runningOp.ExceptionDispatchInfo != null)
+                        {
+                            runningOp.ExceptionDispatchInfo.Throw();
+                        }
+
+                        return true;
                     }
+                }
 
-                    return true;
+                return false;
+            }
+            finally
+            {
+                // The running-op slot was just freed and _eventLock is now released. If promotion on the poll
+                // thread lost the TryEnter race against this method, re-arm _opSet so a queued async operation
+                // is promoted immediately instead of stranded until the next unrelated wakeup.
+                if (claimed && HasPostedAsyncOperation())
+                {
+                    _opSet.Set();
                 }
             }
-
-            return false;
         }
 
+        // Called on the poll thread to promote the next posted async operation into the running-op slot when it
+        // is free. Returns true if an operation was moved into the slot.
+        private bool TryStartPostedAsyncOperation()
+        {
+            Debug.Assert(IsPollThread(), "TryStartPostedAsyncOperation must run on the poll thread.");
+
+            // Cheap early-out so the poll loop does not contend on _eventLock when there is nothing to promote.
+            lock (_postedAsyncOperations)
+            {
+                if (_isClosed || _postedAsyncOperations.Count == 0)
+                    return false;
+            }
+
+            // Never block on _eventLock here: a client in TrySetOperationInternal holds it across
+            // _runningOpCompleteEvent.WaitOne() until its operation completes, and only the poll thread can
+            // complete that operation, so blocking here would deadlock. If a client is mid-set, skip promotion;
+            // it will be retried on a later poll-loop iteration or wakeup.
+            if (!Monitor.TryEnter(_eventLock))
+                return false;
+
+            try
+            {
+                if (_isClosed || _runningOp != null)
+                    return false;
+
+                (AsyncOperation Operation, Action<Exception> OnError) posted;
+                lock (_postedAsyncOperations)
+                {
+                    if (_postedAsyncOperations.Count == 0)
+                        return false;
+
+                    posted = _postedAsyncOperations.Dequeue();
+                }
+
+                _runningOpCompleteEvent.Reset();
+
+                // Unlike TrySetOperationInternal, no one waits for completion; faults are routed to the handler.
+                _runningOp = new OperationDescriptor(posted.Operation) { ErrorHandler = posted.OnError };
+
+                return true;
+            }
+            finally
+            {
+                Monitor.Exit(_eventLock);
+            }
+        }
 
+        private bool HasPostedAsyncOperation()
+        {
+            lock (_postedAsyncOperations)
+            {
+                return _postedAsyncOperations.Count > 0;
+            }
+        }
 
         // Thread routine for the poll loop. It handles calls coming in from the debug engine as well as polling for debug events.
         private void ThreadFunc()
@@ -292,6 +425,11 @@ private void ThreadFunc()
                 {
                     ranOperation = false;
 
+                    if (_runningOp == null)
+                    {
+                        TryStartPostedAsyncOperation();
+                    }
+
                     OperationDescriptor runningOp = _runningOp;
                     if (runningOp != null && !runningOp.IsStarted)
                     {
@@ -333,11 +471,20 @@ private void ThreadFunc()
 
                         if (!completeAsync)
                         {
+                            // Capture the fault before clearing the slot so a synchronous throw is still reported.
+                            Action<Exception> errorHandler = runningOp.ErrorHandler;
+                            ExceptionDispatchInfo exceptionDispatchInfo = runningOp.ExceptionDispatchInfo;
+
                             runningOp.MarkComplete();
 
                             Debug.Assert(_runningOp == runningOp, "How did m_runningOp change?");
                             _runningOp = null;
                             _runningOpCompleteEvent.Set();
+
+                            if (errorHandler != null && exceptionDispatchInfo != null)
+                            {
+                                InvokeErrorHandler(errorHandler, exceptionDispatchInfo.SourceException);
+                            }
                         }
                     }
 
@@ -389,8 +536,43 @@ internal void OnAsyncRunningOpComplete(Task t)
                 }
             }
             _runningOp.MarkComplete();
+
+            // Capture the fault before clearing the slot so it is routed to the handler, not discarded.
+            Action<Exception> errorHandler = _runningOp.ErrorHandler;
+            ExceptionDispatchInfo exceptionDispatchInfo = _runningOp.ExceptionDispatchInfo;
+
+            // Invoke the handler while the running-op slot is still held so it does not run concurrently
+            // with the next queued operation. This may still run on the task's completion thread.
+            if (errorHandler != null && exceptionDispatchInfo != null)
+            {
+                InvokeErrorHandler(errorHandler, exceptionDispatchInfo.SourceException);
+            }
+
             _runningOp = null;
             _runningOpCompleteEvent.Set();
+
+            lock (_postedAsyncOperations)
+            {
+                if (_postedAsyncOperations.Count > 0)
+                {
+                    _opSet.Set();
+                }
+            }
+        }
+
+        private void InvokeErrorHandler(Action<Exception> errorHandler, Exception exception)
+        {
+            try
+            {
+                errorHandler(exception);
+            }
+            catch (Exception e) when (ExceptionHelper.BeforeCatch(e, Logger, reportOnlyCorrupting: false))
+            {
+                if (PostedOperationErrorEvent != null)
+                {
+                    PostedOperationErrorEvent(this, e);
+                }
+            }
         }
 
         internal bool IsPollThread()