Fix debugging with PSES pipeline thread changes #1574

rjmholt · 2021-09-14T23:46:42Z

This PR moves the host REPL implementation off of its own thread and back into the host itself for a synchronous implementation. This makes the host more monolithic, but because of the required coupling of the internals of the host this is pretty hard to avoid (and trying to avoid it introduces other complexity). There's still some complexity here, both because the design itself is pretty big and hard to juggle, but also because some interfaces are deliberately separated out to try and not expose the host to everything.

With the REPL integrated back in, hooking up the debugger is simpler and is now done.

rjmholt · 2021-09-15T02:53:57Z

src/PowerShellEditorServices/Server/PsesDebugServer.cs

+                        // Ensure the debugger mode is set correctly
+                        _debugContext.EnableDebugMode();


This is required for remote debugging to work

rjmholt · 2021-09-15T02:54:58Z

src/PowerShellEditorServices/Services/DebugAdapter/BreakpointService.cs

@@ -44,7 +46,7 @@ internal class BreakpointService

        public async Task<List<Breakpoint>> GetBreakpointsAsync()
        {
-            if (BreakpointApiUtils.SupportsBreakpointApis)
+            if (_editorServicesHost.CurrentRunspace.PowerShellVersionDetails.Version >= s_minimumBreakpointApiVersion)


I'm not entirely sure if this logic is correct -- ideally it should depend on the client not the remote. I'll need to try out the old logic and see if that works before we merge

BreakpointApiUtils.SupportsBreakpointApis should eventually be fixed to answer this question correctly.

rjmholt · 2021-09-15T02:57:07Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugEventHandlerService.cs

-            else if (
-                e.ChangeAction == RunspaceChangeAction.Exit && false)
-            //    _powerShellContextService.IsDebuggerStopped)
+            switch (e.ChangeAction)


This is just a style change

rjmholt · 2021-09-15T02:57:20Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugEventHandlerService.cs

            }
        }

-        private void PowerShellContext_DebuggerResumed(object sender, DebuggerResumeAction e)
+        private void PowerShellContext_DebuggerResuming(object sender, DebuggerResumingEventArgs e)


Should probably rename this method

rjmholt · 2021-09-15T02:57:43Z

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

@@ -408,7 +408,7 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str
            // Evaluate the expression to get back a PowerShell object from the expression string.
            // This may throw, in which case the exception is propagated to the caller
            PSCommand evaluateExpressionCommand = new PSCommand().AddScript(value);
-            object expressionResult = (await _executionService.ExecutePSCommandAsync<object>(evaluateExpressionCommand, new PowerShellExecutionOptions(), CancellationToken.None)).FirstOrDefault();
+            object expressionResult = (await _executionService.ExecutePSCommandAsync<object>(evaluateExpressionCommand, CancellationToken.None)).FirstOrDefault();


Needs a ConfigureAwait(false)

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

rjmholt · 2021-09-15T03:44:48Z

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

+        }
+
+        /*
+        private void PopOrReinitializeRunspace()


I wrote this as a sort of last ditch attempt to stop the extension from falling over when everything goes wrong. The code currently isn't compatible with the new implementation, so we should review it and decide whether it's worth keeping in any way or should just be gotten rid of

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

src/PowerShellEditorServices/Services/Extension/ExtensionService.cs

PaulHigin · 2021-09-15T17:21:13Z

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

+    /// This behavior is unlikely to change and ensuring its correctness at our layer is likely to be costly.
+    /// See https://stackoverflow.com/q/26472251.
+    /// </remarks>
+    internal class BlockingConcurrentDeque<T>


I don't see where this is used anywhere in this PR.
Also, should this not have a Dispose() method to clean up collections, event objects?
It is not clear to me what this is for, it would be nice to have comments illustrating its use.

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

rjmholt

Need to add copyright headers

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

SeeminglyScience · 2021-10-05T14:35:46Z

Looking great!

I did an initial run of feedback. It's mostly just a few missing ConfigureAwait's but it may be best to just ignore those here and hit it with an analyzer later.

Hopefully the "suggestions" work, haven't tried to add them from the VSCode PR extension yet.

I'll also need to spend some time with the build and see how it all feels, but I'm loving the changes and it all looks very well done. Great work Rob!

SeeminglyScience

Hmm, it didn't submit the review with the comment. 🤷

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

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

SeeminglyScience · 2021-09-24T14:02:25Z

src/PowerShellEditorServices/Services/PowerShell/Console/ConsoleReadLine.cs

-        public SecureString ReadSecureLine() => ReadSecureLineAsync(CancellationToken.None).GetAwaiter().GetResult();
+        public string ReadLine(CancellationToken cancellationToken)
+        {
+            return _psesHost.InvokeDelegate<string>(representation: "ReadLine", new ExecutionOptions { MustRunInForeground = true }, InvokePSReadLine, cancellationToken);


Consider making the delegate creation here static (assuming we can pass state in one of these parameters).

e.g.

static (@this, ct) => @this.InvokeReadLine(ct)

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

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

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

src/PowerShellEditorServices/Services/PowerShell/Utility/PowerShellExtensions.cs

andyleejordan · 2021-10-05T16:02:55Z

I did an initial run of feedback. It's mostly just a few missing ConfigureAwait's but it may be best to just ignore those here and hit it with an analyzer later.

FYI the analyzer is enabled so if this is somewhat up to date with master them missing should cause a compiler failure.

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

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

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

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

src/PowerShellEditorServices/Services/Workspace/RemoteFileManagerService.cs

src/PowerShellEditorServices/Utility/IsExternalInit.cs

…eline thread

rjmholt · 2021-10-11T21:49:13Z

I'm going to merge this in now so we can continue work on the main branch, breaking out smaller work items into new PRs.

You can see the changes I've made to address comments in individual commits, which will be preserved in the work branch by a merge commit.

rjmholt requested review from andyleejordan, daxian-dbw, PaulHigin and SeeminglyScience September 14, 2021 23:47

rjmholt commented Sep 15, 2021

View reviewed changes

src/PowerShellEditorServices/Services/Extension/ExtensionService.cs Outdated Show resolved Hide resolved

PaulHigin suggested changes Sep 15, 2021

View reviewed changes

rjmholt commented Sep 15, 2021

View reviewed changes

src/PowerShellEditorServices/Services/PowerShell/Host/InternalHost.cs Outdated Show resolved Hide resolved

rjmholt mentioned this pull request Sep 15, 2021

VS Code Formatting Instability PowerShell/vscode-powershell#3565

Closed

4 tasks

rjmholt commented Sep 15, 2021

View reviewed changes

daxian-dbw reviewed Sep 15, 2021

View reviewed changes

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs Outdated Show resolved Hide resolved

daxian-dbw mentioned this pull request Sep 20, 2021

Exception thrown from CalculateWhereAndWhatToRender PowerShell/PSReadLine#2298

Closed

SeeminglyScience reviewed Oct 5, 2021

View reviewed changes

rjmholt added 15 commits October 5, 2021 15:13

Remove extraneous JS file

9e753a7

Debugging dependency injection

0f5f5e0

Get UI debugger running

744fd88

Resolve commented-out code

0c21f19

Improve queue implementation

4cc3f77

Add async error handling code

cdd4e3e

Rework execution queue to allow correct cancellation

248008a

TEMP: Need to fix cancellation from PSRL scope

a001af4

TEMP: Moving to monolithic host

b5b3325

WIP

5428be9

Move all basic functionality into a new synchronous internal host

d3f8e01

Restore host to simple working order

502e4bb

Add simple cancellation support

9ddfd9c

Ensure startup operations are performed before first prompt

942e7a6

Fix dependency issue

ec9517f

rjmholt added 14 commits October 8, 2021 14:36

Fix ThrowOnError configuration check

c2797d1

Ensure the DSC module import fails with an error

0f62b1d

Remove unneeded async/await

1a62125

Add comment explaining remote debug setting

0480ab6

Remove unused enum

1d1cab8

Add comment to execution options class

029539a

Add comment to synchronous task about synchronous results

03f18aa

Ensure F8 works

a07e7da

Add comment about not exiting the top level

25d38a1

Generalize exit handling

268eb5f

Add comment about remote prompt

15b16f0

Simplify nested PowerShell creation

1e98e9c

Use a more explicit return

d21efeb

Fix Ctrl-C detection

543dabe

andyleejordan requested changes Oct 9, 2021

View reviewed changes

rjmholt added 9 commits October 11, 2021 10:14

Rename debug event handlers

963fba1

Remove unneeded PSRL static ctor call - PSRL is now loaded on the pip…

f5b644e

…eline thread

Move execution service to an interface

905686c

Enhance remoting comment

0a69937

Turn on psedit registration in remote sessions

da786c8

Ensure failed remote file saves are logged

fb08b67

Add comment about IsExternalInit

1f52dde

Reinstate runspace cleanup logic

27a1735

Fix remote psedit registration

87022dc

rjmholt merged commit 7ec9bfe into PowerShell:async-ps-consumer Oct 11, 2021

rjmholt deleted the pipelinethread-fix-debugging branch October 11, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix debugging with PSES pipeline thread changes #1574

Fix debugging with PSES pipeline thread changes #1574

rjmholt commented Sep 14, 2021 •

edited

Loading

rjmholt Sep 15, 2021

rjmholt Sep 15, 2021

andyleejordan Oct 7, 2021

rjmholt Sep 15, 2021

rjmholt Sep 15, 2021

rjmholt Sep 15, 2021

rjmholt Sep 15, 2021

PaulHigin Sep 15, 2021

rjmholt left a comment

SeeminglyScience commented Oct 5, 2021

SeeminglyScience left a comment

SeeminglyScience Sep 24, 2021

andyleejordan commented Oct 5, 2021

rjmholt commented Oct 11, 2021

		// Ensure the debugger mode is set correctly
		_debugContext.EnableDebugMode();

Fix debugging with PSES pipeline thread changes #1574

Fix debugging with PSES pipeline thread changes #1574

Conversation

rjmholt commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

SeeminglyScience commented Oct 5, 2021

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Oct 5, 2021

rjmholt commented Oct 11, 2021

rjmholt commented Sep 14, 2021 •

edited

Loading