Merge pull request #1778 from PowerShell/andschwa/fix-debugger

Author: andyleejordan

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs

@@ -11,15 +11,16 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.PowerShell.EditorServices.Logging;
 using Microsoft.PowerShell.EditorServices.Services;
-using OmniSharp.Extensions.DebugAdapter.Protocol.Events;
-using OmniSharp.Extensions.JsonRpc;
-using Microsoft.PowerShell.EditorServices.Services.TextDocument;
-using OmniSharp.Extensions.DebugAdapter.Protocol.Requests;
-using OmniSharp.Extensions.DebugAdapter.Protocol.Server;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Context;
-using Microsoft.PowerShell.EditorServices.Services.PowerShell.Runspace;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution;
+using Microsoft.PowerShell.EditorServices.Services.PowerShell.Host;
+using Microsoft.PowerShell.EditorServices.Services.PowerShell.Runspace;
+using Microsoft.PowerShell.EditorServices.Services.TextDocument;
+using OmniSharp.Extensions.DebugAdapter.Protocol.Events;
+using OmniSharp.Extensions.DebugAdapter.Protocol.Requests;
+using OmniSharp.Extensions.DebugAdapter.Protocol.Server;
+using OmniSharp.Extensions.JsonRpc;
 
 namespace Microsoft.PowerShell.EditorServices.Handlers
 {
@@ -122,6 +123,9 @@ public LaunchAndAttachHandler(
 
         public async Task<LaunchResponse> Handle(PsesLaunchRequestArguments request, CancellationToken cancellationToken)
         {
+            // The debugger has officially started. We use this to later check if we should stop it.
+            ((PsesInternalHost)_executionService).DebugContext.IsActive = true;
+
             _debugEventHandlerService.RegisterEventHandlers();
 
             // Determine whether or not the working directory should be set in the PowerShellContext.
@@ -213,6 +217,9 @@ public async Task<LaunchResponse> Handle(PsesLaunchRequestArguments request, Can
 
         public async Task<AttachResponse> Handle(PsesAttachRequestArguments request, CancellationToken cancellationToken)
         {
+            // The debugger has officially started. We use this to later check if we should stop it.
+            ((PsesInternalHost)_executionService).DebugContext.IsActive = true;
+
             _debugStateService.IsAttachSession = true;
 
             _debugEventHandlerService.RegisterEventHandlers();

src/PowerShellEditorServices/Services/PowerShell/Debugging/PowerShellDebugContext.cs

@@ -94,7 +94,7 @@ public void EnableDebugMode()
             _psesHost.Runspace.Debugger.SetDebugMode(DebugModes.LocalScript | DebugModes.RemoteScript);
         }
 
-        public void Abort() => SetDebugResuming(DebuggerResumeAction.Stop, isDisconnect: true);
+        public void Abort() => SetDebugResuming(DebuggerResumeAction.Stop);
 
         public void BreakExecution() => _psesHost.Runspace.Debugger.SetDebuggerStepMode(enabled: true);
 
@@ -106,10 +106,12 @@ public void EnableDebugMode()
 
         public void StepOver() => SetDebugResuming(DebuggerResumeAction.StepOver);
 
-        public void SetDebugResuming(DebuggerResumeAction debuggerResumeAction, bool isDisconnect = false)
+        public void SetDebugResuming(DebuggerResumeAction debuggerResumeAction)
         {
-            // NOTE: We exit because the paused/stopped debugger is currently in a prompt REPL, and
-            // to resume the debugger we must exit that REPL.
+            // We exit because the paused/stopped debugger is currently in a prompt REPL, and to
+            // resume the debugger we must exit that REPL. If we're continued from 'c' or 's', this
+            // is already set and so is a no-op; but if the user clicks the continue or step button,
+            // then this came over LSP and we need to set it.
             _psesHost.SetExit();
 
             if (LastStopEventArgs is not null)
@@ -127,23 +129,31 @@ public void SetDebugResuming(DebuggerResumeAction debuggerResumeAction, bool isD
                 return;
             }
 
-            if (debuggerResumeAction is DebuggerResumeAction.Stop)
+            // If we're stopping (or disconnecting, which is the same thing in LSP-land), then we
+            // want to cancel any debug prompts, remote prompts, debugged scripts, etc. However, if
+            // the debugged script has exited normally (or was quit with 'q'), we still get an LSP
+            // notification that eventually lands here with a stop event. In this case, the debug
+            // context is NOT active and we do not want to cancel the regular REPL.
+            if (!_psesHost.DebugContext.IsActive)
             {
-                // If we're disconnecting we want to unwind all the way back to the default, local
-                // state. So we use UnwindCallStack here to ensure every context frame is cancelled.
-                if (isDisconnect)
-                {
-                    _psesHost.UnwindCallStack();
-                    return;
-                }
+                return;
+            }
 
-                _psesHost.CancelIdleParentTask();
+            // If the debugger is active and we're stopping, we need to unwind everything.
+            if (debuggerResumeAction is DebuggerResumeAction.Stop)
+            {
+                // TODO: We need to assign cancellation tokens to each frame, because the current
+                // logic results in a deadlock here when we try to cancel the scopes...which
+                // includes ourself (hence running it in a separate thread).
+                Task.Run(() => _psesHost.UnwindCallStack());
                 return;
             }
 
+            // Otherwise we're continuing or stepping (i.e. resuming) so we need to cancel the
+            // debugger REPL.
             if (_psesHost.CurrentFrame.IsRepl)
             {
-                _psesHost.CancelCurrentTask();
+                _psesHost.CancelIdleParentTask();
             }
         }
 
@@ -166,15 +176,14 @@ public void ProcessDebuggerResult(DebuggerCommandResults debuggerResult)
         {
             if (debuggerResult?.ResumeAction is not null)
             {
-                SetDebugResuming(debuggerResult.ResumeAction.Value);
-
-                // If a debugging command like `c` is specified in a nested remote
-                // debugging prompt we need to unwind the nested execution loop.
-                if (_psesHost.CurrentFrame.IsRemote)
+                // Since we're processing a command like 'c' or 's' remotely, we need to tell the
+                // host to stop the remote REPL loop.
+                if (debuggerResult.ResumeAction is not DebuggerResumeAction.Stop || _psesHost.CurrentFrame.IsRemote)
                 {
                     _psesHost.ForceSetExit();
                 }
 
+                SetDebugResuming(debuggerResult.ResumeAction.Value);
                 RaiseDebuggerResumingEvent(new DebuggerResumingEventArgs(debuggerResult.ResumeAction.Value));
 
                 // The Terminate exception is used by the engine for flow control

src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs

@@ -62,7 +62,7 @@ public override IReadOnlyList<TResult> Run(CancellationToken cancellationToken)
 
                 if (PowerShellExecutionOptions.WriteInputToHost)
                 {
-                    _psesHost.WriteWithPrompt(_psCommand, cancellationToken);
+                    _psesHost.UI.WriteLine(_psCommand.GetInvocationText());
                 }
 
                 return _pwsh.Runspace.Debugger.InBreakpoint

src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs

@@ -729,6 +729,19 @@ private void DoOneRepl(CancellationToken cancellationToken)
                 return;
             }
 
+            // TODO: We must remove this awful logic, it causes so much pain. The StopDebugContext()
+            // requires that we're not in a prompt that we're skipping, otherwise the debugger is
+            // "active" but we haven't yet hit a breakpoint.
+            //
+            // When a task must run in the foreground, we cancel out of the idle loop and return to
+            // the top level. At that point, we would normally run a REPL, but we need to
+            // immediately execute the task. So we set _skipNextPrompt to do that.
+            if (_skipNextPrompt)
+            {
+                _skipNextPrompt = false;
+                return;
+            }
+
             // We use the REPL as a poll to check if the debug context is active but PowerShell
             // indicates we're no longer debugging. This happens when PowerShell was used to start
             // the debugger (instead of using a Code launch configuration) via Wait-Debugger or
@@ -741,15 +754,6 @@ private void DoOneRepl(CancellationToken cancellationToken)
                 StopDebugContext();
             }
 
-            // When a task must run in the foreground, we cancel out of the idle loop and return to the top level.
-            // At that point, we would normally run a REPL, but we need to immediately execute the task.
-            // So we set _skipNextPrompt to do that.
-            if (_skipNextPrompt)
-            {
-                _skipNextPrompt = false;
-                return;
-            }
-
             try
             {
                 string prompt = GetPrompt(cancellationToken);
@@ -758,18 +762,22 @@ private void DoOneRepl(CancellationToken cancellationToken)
 
                 // If the user input was empty it's because:
                 //  - the user provided no input
-                //  - the readline task was canceled
-                //  - CtrlC was sent to readline (which does not propagate a cancellation)
+                //  - the ReadLine task was canceled
+                //  - CtrlC was sent to ReadLine (which does not propagate a cancellation)
                 //
-                // In any event there's nothing to run in PowerShell, so we just loop back to the prompt again.
-                // However, we must distinguish the last two scenarios, since PSRL will not print a new line in those cases.
+                // In any event there's nothing to run in PowerShell, so we just loop back to the
+                // prompt again. However, PSReadLine will not print a newline for CtrlC, so we print
+                // one, but we do not want to print one if the ReadLine task was canceled.
                 if (string.IsNullOrEmpty(userInput))
                 {
-                    if (cancellationToken.IsCancellationRequested || LastKeyWasCtrlC())
+                    if (LastKeyWasCtrlC())
                     {
                         UI.WriteLine();
                     }
-                    return;
+                    // Propogate cancellation if that's what happened, since ReadLine won't.
+                    // TODO: We may not need to do this at all.
+                    cancellationToken.ThrowIfCancellationRequested();
+                    return; // Task wasn't canceled but there was no input.
                 }
 
                 InvokeInput(userInput, cancellationToken);
@@ -783,10 +791,8 @@ private void DoOneRepl(CancellationToken cancellationToken)
             {
                 throw;
             }
-            catch (FlowControlException)
-            {
-                // Do nothing, a break or continue statement was used outside of a loop.
-            }
+            // Do nothing, a break or continue statement was used outside of a loop.
+            catch (FlowControlException) { }
             catch (Exception e)
             {
                 UI.WriteErrorLine($"An error occurred while running the REPL loop:{Environment.NewLine}{e}");
@@ -830,25 +836,14 @@ private string GetPrompt(CancellationToken cancellationToken)
             return prompt;
         }
 
-        /// <summary>
-        /// This is used to write the invocation text of a command with the user's prompt so that,
-        /// for example, F8 (evaluate selection) appears as if the user typed it. Used when
-        /// 'WriteInputToHost' is true.
-        /// </summary>
-        /// <param name="command">The PSCommand we'll print after the prompt.</param>
-        /// <param name="cancellationToken"></param>
-        public void WriteWithPrompt(PSCommand command, CancellationToken cancellationToken)
-        {
-            UI.Write(GetPrompt(cancellationToken));
-            UI.WriteLine(command.GetInvocationText());
-        }
-
         private string InvokeReadLine(CancellationToken cancellationToken)
         {
-            cancellationToken.ThrowIfCancellationRequested();
             try
             {
+                // TODO: If we can pass the cancellation token to ReadKey directly in PSReadLine, we
+                // can remove this logic.
                 _readKeyCancellationToken = cancellationToken;
+                cancellationToken.ThrowIfCancellationRequested();
                 return _readLineProvider.ReadLine.ReadLine(cancellationToken);
             }
             finally