Add Debug Runspace command #1782

adamdriscoll · 2019-03-07T05:58:26Z

PR Summary

This enables support for attaching to local runspaces. This adds a new command that allows the user to select a local runspace and attach to it. This is similar to attaching to a remote process and uses the same PSES request.

The accompanying PR in PSES is here: PowerShell/PowerShellEditorServices#875

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

[ x] PR has a meaningful title
[ x] Summarized changes
[ -] PR has tests
[x ] This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

rkeithhill · 2019-03-09T00:31:00Z

package.json

@@ -36,7 +36,8 @@
    "onCommand:PowerShell.ShowSessionConsole",
    "onCommand:PowerShell.ShowSessionMenu",
    "onCommand:PowerShell.RestartSession",
-    "onView:PowerShellCommands"
+    "onView:PowerShellCommands",
+    "onCommand:PowerShell.PickRunspace"


Minor nit, could you sort this up by the other "Pick" command?

rkeithhill · 2019-03-09T00:36:37Z

package.json

@@ -407,6 +409,17 @@
              "request": "launch",
              "cwd": ""
            }
+          },
+          {
+            "label": "PowerShell: Debug Runspace",


Should we mention this applies to the "current" PSIC session. In other places, we refer to this as the "Interactive Session". Also, since this is an "attach" it probably should be named something like PowerShell Attach Interactive Session Runspace. Or maybe I'm misunderstanding the "current" process id value?

Current was just a way to signal to PSES that we wanna attach to the current process and not some other remote process. I kinda wanted to avoid this all together and just set the process ID to the current PSES process ID but couldn't find a nice clean way to do that. We can call this whatever makes sense. If interactive session makes more sense, let's do it.

rkeithhill · 2019-03-09T00:37:42Z

package.json

@@ -495,6 +513,13 @@
            "type": "powershell",
            "request": "launch",
            "cwd": ""
+          },
+          {
+            "name": "PowerShell Debug Runspace",


Should be same as what decide to use for a name on line 414.

TylerLeonhardt · 2019-03-10T21:04:35Z

Hey @adamdriscoll! This is awesome. I'd love to help UD customers and the likes.

Just so you know, I just merged in #1775 and PowerShell/PowerShellEditorServices#871 which changes the way the attach launch config works.

You might have to do some slight refactoring to address that - apologies for making you do that!

FWIW, I've moved the logic for prompting for the process id to here.

I think this will allow you to set processId to current and work just fine with the logic you've got in PSES.

Let me know if you need anything.

adamdriscoll · 2019-03-10T23:46:13Z

@TylerLeonhardt It's all good. I'll take a look at the changes and get this PR refactored.

adamdriscoll · 2019-03-11T13:41:18Z

@TylerLeonhardt Merged your changes. Had to only make slight changes to this PR and the PSES PR.

TylerLeonhardt · 2019-03-11T20:31:04Z

package.json

@@ -447,9 +460,9 @@
                "default": null
              },
              "runspaceId": {
-                "type": "number",
+                "type": "string",


I really wish it were possible to do like string | int for this.

actually... it looks like you can do:

"type":["string","number"]

@adamdriscoll I think you maybe missed this?

Yep. In there now.

TylerLeonhardt · 2019-03-11T20:32:15Z

package.json

@@ -491,13 +504,20 @@
            "name": "PowerShell Attach to Host Process",
            "type": "PowerShell",
            "request": "attach",
-            "runspaceId": 1
+            "runspaceId": "1"


I wonder if we should go ahead and make this ${command:PickRunspace} now?

thoughts @rkeithhill @rjmholt?

So would the workflow be 1) query PSES for host processes 2) user picks host process, 3) query PSES for runspaces in the selected host process and 4) user picks runspace? If so, that sounds pretty good to me. Although I have a feeling the vast majority of time the user will pick runspace 1. If they get tired of the prompt, I guess then could then hard-code 1 into the debug config.

Exactly! Ok. Lets do that. @adamdriscoll can you follow #1775 and implement it in a similar fashion?

Ideally, the user should not have to put anything for runspaceId and it will prompt them for it.

After starting to implement this I realized I needed to update the request to pass in a process ID for the request runspace command. I kinda got that working but am seeing weird stuff where its complaining about null values and stating the "Attach to Host Process" debugger is already running when I cancel out of the pickers.

I will revisit tomorrow but it's not as simple as I thought at first.

Let me know if there's anything you need from us :)

I got this working. Updated this PR and the PSES PR to support process ID and to pop up the picker in the same place as the process picker.

TylerLeonhardt

Looking good!

TylerLeonhardt · 2019-03-11T20:35:58Z

src/features/DebugSession.ts

+    private getLanguageClientResolve: (value?: LanguageClient | Thenable<LanguageClient>) => void;
+
+    constructor() {
+


nit: you can probably get rid of this newline

TylerLeonhardt · 2019-03-11T20:44:57Z

src/features/DebugSession.ts

+        return this.languageClient.sendRequest(GetRunspaceRequestType, null).then((runspaces) => {
+            const items: IRunspaceItem[] = [];
+
+            for (const p in runspaces) {


you can probably switch this to a for...of loop then you don't have to index, you can just do:

for (const runspace of runspaces)

and runspace is an IRunspaceItem

Latest commit fixes this. There was actually a mismatch in the PSES response type and the VS Code response type.

TylerLeonhardt · 2019-03-11T20:55:19Z

package.json

                "description": "Optional: The ID of the runspace to debug in the attached process.  Defaults to 1.  Works only on PowerShell 5 and above.",
-                "default": 1
+                "default": "1"


One of the things I did in #1775 was made the default for processId to be null and then prompt for it when resolving the debug config:

vscode-powershell/src/features/DebugSession.ts

Lines 81 to 83 in 1db2283

if (!config.customPipeName && !config.processId) {

config.processId = await vscode.commands.executeCommand("PowerShell.PickPSHostProcess");

}

@rkeithhill @rjmholt if we want to make prompting for PickRunspace to be the default, it should probably be implemented in a similar fashion.

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

package.json

+              "type": "PowerShell",
+              "request": "attach",
+              "processId": "current",
+              "runspaceId": "^\"\\${command:PickRunspace}\""


I don't think we need this line?

TylerLeonhardt

LGTM!

TylerLeonhardt · 2019-03-13T22:48:53Z

src/features/DebugSession.ts

+
+            for (const runspace of response) {
+                // Skip default runspace
+                if (runspace.id === 1 || runspace.name === "PSAttachRunspace") {


I totally just realized that this should only be filtered out IF in the current process...

If we're in a different process, we probably want to attach to runspace 1.

@adamdriscoll can you fix this up?

Yep! Will get another PR in.

* Add Debug Runspace command * Update package.json based on feedback. * Update response to match PSES response. * Change how we ask for RunspaceId. * Clean up.

Add Debug Runspace command

be326b5

TylerLeonhardt added the Triage label Mar 8, 2019

rkeithhill reviewed Mar 9, 2019

View reviewed changes

Update package.json based on feedback.

e7d72b5

Merge changes for custom pipe name.

e72d1fe

TylerLeonhardt reviewed Mar 11, 2019

View reviewed changes

adamdriscoll added 2 commits March 11, 2019 15:16

Update response to match PSES response.

35deb7a

Change how we ask for RunspaceId.

64bb51c

TylerLeonhardt reviewed Mar 12, 2019

View reviewed changes

Clean up.

0a31064

TylerLeonhardt approved these changes Mar 13, 2019

View reviewed changes

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

TylerLeonhardt reviewed Mar 13, 2019

View reviewed changes

TylerLeonhardt pushed a commit that referenced this pull request Mar 20, 2019

Add Debug Runspace command (#1782)

568edf4

* Add Debug Runspace command * Update package.json based on feedback. * Update response to match PSES response. * Change how we ask for RunspaceId. * Clean up.

TylerLeonhardt added Backport-Legacy and removed Triage labels Mar 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Debug Runspace command #1782

Add Debug Runspace command #1782

adamdriscoll commented Mar 7, 2019

rkeithhill Mar 9, 2019

rkeithhill Mar 9, 2019

adamdriscoll Mar 9, 2019

rkeithhill Mar 9, 2019

TylerLeonhardt commented Mar 10, 2019

adamdriscoll commented Mar 10, 2019

adamdriscoll commented Mar 11, 2019

TylerLeonhardt Mar 11, 2019

TylerLeonhardt Mar 11, 2019

TylerLeonhardt Mar 12, 2019

adamdriscoll Mar 13, 2019

TylerLeonhardt Mar 11, 2019

rkeithhill Mar 11, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 12, 2019

TylerLeonhardt Mar 12, 2019

adamdriscoll Mar 12, 2019

TylerLeonhardt left a comment

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt Mar 11, 2019

adamdriscoll Mar 11, 2019

TylerLeonhardt Mar 11, 2019

TylerLeonhardt Mar 12, 2019

adamdriscoll Mar 13, 2019

TylerLeonhardt left a comment

TylerLeonhardt Mar 13, 2019

TylerLeonhardt Mar 13, 2019

adamdriscoll Mar 15, 2019

		private getLanguageClientResolve: (value?: LanguageClient \| Thenable<LanguageClient>) => void;

		constructor() {

	if (!config.customPipeName && !config.processId) {
	config.processId = await vscode.commands.executeCommand("PowerShell.PickPSHostProcess");
	}

Add Debug Runspace command #1782

Add Debug Runspace command #1782

Conversation

adamdriscoll commented Mar 7, 2019

PR Summary

PR Checklist

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 commented Mar 10, 2019

adamdriscoll commented Mar 10, 2019

adamdriscoll commented Mar 11, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment