Add attach to local runspace. #875

adamdriscoll · 2019-03-07T05:57:05Z

Adds support for attaching to a local runspace. This extends the existing support for attaching to a runspace in a remote process. This enables debugging for modules that create additional runspaces such as ThreadJob, PoshRSJob and UnivesalDashboard.

adamdriscoll · 2019-03-07T06:07:29Z

Not seeing this test failure locally. I am seeing other failures that aren't listed in AppVeyor that seem unrelated to my PR.

rkeithhill · 2019-03-09T02:50:48Z

src/PowerShellEditorServices.Protocol/DebugAdapter/AttachRequest.cs

@@ -21,5 +21,7 @@ public class AttachRequestArguments
        public string ProcessId { get; set; }

        public int RunspaceId { get; set; }
+
+        public string LocalRunspaceId { get; set; }


Just asking but ... do we really need two different fields for runspace id?

BTW if it would be more convenient if RunspaceId was a string, we change the type I think. We'd just update the VSCode debug attach code to stringify the runspace id number.

OK. I can make that change. That's pretty much the reason I added another field was that I didn't wanna mess with RunspaceId's type but I'm happy to adjust that to eliminate the new property.

I guess I was curious how this affects existing launch.json files. Is there something we need to do to ensure that they get upgraded in some way or will a number just work?

I just tried this. Seems to still work. It complains in the launch.json that the number should be a string but the attaching still works.

rkeithhill · 2019-03-09T02:59:01Z

src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

+                {
+                    runspaceId = attachParams.RunspaceId > 0 ? attachParams.RunspaceId : 1;
+                }
+                else if (int.TryParse(attachParams.LocalRunspaceId, out int localRunspaceId))


Seems like we could int.TryParse() on a string typed RunspaceId. In the normal "attach" case we'd just set it to "1". Your new debug config would set it to whatever the user picks from the runspace id pick list.

Sweet. Works for me. I will update these PRs sometime this weekend.

TylerLeonhardt · 2019-03-11T20:14:12Z

src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

@@ -417,16 +417,18 @@ protected void Stop()
                    return;
                }

-                await _editorSession.PowerShellContext.ExecuteScriptStringAsync(
+                if (attachParams.ProcessId != "current") {


Shouldn't need this if because the outer if tries to parse the ProcessId as an integer (L411):

if (processIdIsSet && int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))

Good catch. Updated the PR.

TylerLeonhardt · 2019-03-11T20:20:14Z

src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

+            int runspaceId = 1;
+            if (int.TryParse(attachParams.RunspaceId, out runspaceId))
+            {
+                runspaceId = runspaceId > 0 ? runspaceId : 1;


I would maybe factor this into the if

if (!int.TryParse(attachParams.RunspaceId, out runspaceId) || runspaceId <= 0) { Logger.Write( LogLevel.Error, $"Attach request failed, '{attachParams.RunspaceId}' is an invalid value for the processId."); await requestContext.SendErrorAsync( "A positive integer must be specified for the RunspaceId field."); return; }

TylerLeonhardt

Looking good! Just some general comments and questions.

src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

TylerLeonhardt · 2019-03-11T20:22:28Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+                var psCommand = new PSCommand();
+                psCommand.AddCommand("Get-Runspace");
+
+                var runspaces = await editorSession.PowerShellContext.ExecuteCommandAsync<PSObject>(psCommand);


can you replace var with the type since this is a tad more complicated than new Blah()

Also, instead of PSObject you should be able to do System.Management.Automation.Runspaces.Runspace and then you don't have to use dynamic below.

TylerLeonhardt · 2019-03-11T20:23:24Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+        {
+            var runspaceResponses = new List<GetRunspaceResponse>();
+
+            if (this.editorSession.PowerShellContext.LocalPowerShellVersion.Version.Major >= 5)


Was Get-Runspace added in 5?

That's a really good question. I just took that check from the process picker but I'm not sure when Debug-Runspace\Get-Runspace was added. I can try and find out.

Well looks like that answers our question :)

It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com>

…erShellEditorServices into runspace-attach

TylerLeonhardt

LGTM! I'm excited for this code path to see more usage. It's a very nice feature of the extension.

TylerLeonhardt · 2019-03-12T17:21:09Z

@adamdriscoll if you couldn't tell already, that test is flaky... I've been meaning to fix it, but I haven't had the time. I will address it as soon as I can.

restarted build.

TylerLeonhardt

just a couple nits really. Basically looks good.

TylerLeonhardt · 2019-03-12T22:34:07Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

@@ -1222,17 +1222,27 @@ public static string GetDecoratedSymbolName(SymbolReference symbolReference)
        }

        protected async Task HandleGetRunspaceRequestAsync(
-            object noParams,
+            object processId,


Does this have to be object? Or can we turn it into a string?

You're right. Changed to a string.

TylerLeonhardt · 2019-03-12T23:08:35Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

@@ -1246,6 +1256,12 @@ public static string GetDecoratedSymbolName(SymbolReference symbolReference)
                            });
                    }
                }
+
+                if (processId != null && processId.ToString() != "current") {


Since you do this comparison a couple times, maybe do a:

bool isCurrentProcess = processId != null && processId.ToString() != "current";

and use that.

Works for me. Done.

TylerLeonhardt

LGTM! Excited to get this in!

* Add attach to local runspace. * Remove comment. * Use single runspaceId property. * Remove unnecessary processId check. * Factored if. * Update src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com> * Use runspace rather than dynamic. * Accept process ID for GetRunspaceRequest. * Clean up.

Add attach to local runspace.

d1ca49a

adamdriscoll requested review from rjmholt and rkeithhill as code owners March 7, 2019 05:57

adamdriscoll mentioned this pull request Mar 7, 2019

Add Debug Runspace command PowerShell/vscode-powershell#1782

Merged

Remove comment.

c794be4

TylerLeonhardt added the Triage label Mar 8, 2019

rkeithhill reviewed Mar 9, 2019

View reviewed changes

adamdriscoll added 2 commits March 8, 2019 23:08

Use single runspaceId property.

384b91a

Merge changes for custom pipe name.

75220a6

TylerLeonhardt reviewed Mar 11, 2019

View reviewed changes

Remove unnecessary processId check.

70a2e12

TylerLeonhardt reviewed Mar 11, 2019

View reviewed changes

Factored if.

cbe5b18

TylerLeonhardt reviewed Mar 11, 2019

View reviewed changes

TylerLeonhardt and others added 3 commits March 11, 2019 14:35

Update src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

28f3840

It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com>

Use runspace rather than dynamic.

1d79002

Merge branch 'runspace-attach' of https://github.com/adamdriscoll/Pow…

6466c44

…erShellEditorServices into runspace-attach

TylerLeonhardt approved these changes Mar 11, 2019

View reviewed changes

Accept process ID for GetRunspaceRequest.

015c1ef

TylerLeonhardt reviewed Mar 12, 2019

View reviewed changes

Clean up.

bb984a0

TylerLeonhardt approved these changes Mar 13, 2019

View reviewed changes

TylerLeonhardt merged commit 13d8f8a into PowerShell:master Mar 13, 2019

TylerLeonhardt added Backport-Legacy labels Mar 20, 2019

TylerLeonhardt removed Backport-Legacy labels Mar 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attach to local runspace. #875

Add attach to local runspace. #875

adamdriscoll commented Mar 7, 2019

adamdriscoll commented Mar 7, 2019

rkeithhill Mar 9, 2019

rkeithhill Mar 9, 2019

adamdriscoll Mar 9, 2019

adamdriscoll Mar 9, 2019

adamdriscoll Mar 9, 2019

rkeithhill Mar 9, 2019

adamdriscoll Mar 9, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt left a comment

TylerLeonhardt Mar 11, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt Mar 11, 2019

TylerLeonhardt left a comment

TylerLeonhardt commented Mar 12, 2019 •

edited

Loading

TylerLeonhardt left a comment

TylerLeonhardt Mar 12, 2019

adamdriscoll Mar 13, 2019

TylerLeonhardt Mar 12, 2019

adamdriscoll Mar 13, 2019

TylerLeonhardt left a comment

Add attach to local runspace. #875

Add attach to local runspace. #875

Conversation

adamdriscoll commented Mar 7, 2019

adamdriscoll commented Mar 7, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a 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

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Mar 12, 2019 • edited Loading

TylerLeonhardt left a 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

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Mar 12, 2019 •

edited

Loading