Enable language and debugging features to use remote and attached runspaces #330

daviwil · 2016-12-30T00:21:10Z

Still finishing this up, needs some polish and minor refactoring here and there. Also need to enable remote file editing, but that should be pretty easy now.

@rkeithhill, take a look when you have some time, I'd certainly appreciate a second pair of eyes on the debugging changes!

This change adds support for remote sessions (PSv4+) and attaching to processes on the same machine (PSv5+).

rkeithhill · 2016-12-30T02:06:11Z

src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs

+                {
+                    // Execute the Debug-Runspace command but don't await it because it
+                    // will block the debug adapter initialization process.
+                    Task nonAwaitedTask =


Do you need to create this variable? It doesn't seem to be used later.

Yeah, the C# compiler gives a warning if you call an async method without doing something with the task. I've been using this trick to silence the message in cases where I don't actually want to await the task.

rkeithhill · 2016-12-30T02:15:39Z

src/PowerShellEditorServices/Debugging/DebugService.cs

@@ -82,10 +89,31 @@ public DebugService(PowerShellContext powerShellContext)

            if (breakpoints.Length > 0)
            {
+                // Make sure we're using the remote script path
+                string scriptPath = scriptFile.FilePath;
+                if (this.powerShellContext.CurrentRunspace.Locale == RunspaceLocale.Remote)


Locale might be confused with i18n "locale". Or perhaps you don't have control over that name.

Yep, that's one of the things I'm going to change, will say more in your other comment.

rkeithhill · 2016-12-30T02:18:44Z

src/PowerShellEditorServices/Debugging/DebugService.cs

@@ -612,16 +640,16 @@ public VariableScope[] GetVariableScopes(int stackFrameId)
                new VariableContainerDetails(this.nextVariableId++, "Scope: " + scope);
            this.variables.Add(scopeVariableContainer);

-            var results = await this.powerShellContext.ExecuteCommand<PSVariable>(psCommand, sendErrorToHost: false);
+            var results = await this.powerShellContext.ExecuteCommand<PSObject>(psCommand, sendErrorToHost: false);


So when querying variables in a remote runspace, I take it we don't get back PSVariable types but PSObject instead?

Remote/attached runspaces return some concrete type that looks like a PSVariable but isn't the same actual type, thus I can't pull them back out as PSVariable. Same thing happened with the CallStackFrames. Not sure why it was done this way, but probably because remote sessions were added later and the types weren't designed to be remotable in the beginning.

rkeithhill · 2016-12-30T02:23:31Z

src/PowerShellEditorServices/Debugging/DebugService.cs

+            // process.  Without the intermediate variable Get-PSCallStack inexplicably returns
+            // an array of strings containing the formatted output of the CallStackFrame list.
+            psCommand.AddScript("$global:__psEditorServices_CallStack = Get-PSCallStack; $global:__psEditorServices_CallStack");
+


I think I create variables like this for the hit count impl. We should probably create a string const somewhere like PsesGlobalVariableNamePrefix = "__psEditorServices_";

Yep, I stole your naming pattern :) I'll add a const string for that.

rkeithhill · 2016-12-30T02:24:08Z

src/PowerShellEditorServices/Debugging/DebugService.cs


-            var results = await this.powerShellContext.ExecuteCommand<CallStackFrame>(psCommand);
+            // This glorious hack ensures that Get-PSCallStack returns a list of CallStackFrame
+            // objects (or "deserialized" CallStackFrames) when attached to a runspace in another


+1 At least you know where the responsible parties sit. :-)

Haha, yep! I'll be asking some questions next week ;)

rkeithhill · 2016-12-30T02:29:02Z

src/PowerShellEditorServices/Debugging/StackFrameDetails.cs

-                FunctionName = callStackFrame.FunctionName,
-                LineNumber = callStackFrame.Position.StartLineNumber,
-                ColumnNumber = callStackFrame.Position.StartColumnNumber,
+                ScriptPath = (callStackFrameObject.Properties["ScriptName"].Value as string) ?? NoFileScriptPath,


I wonder if over time we will want two classes: LocalStackFrameDetails and RemoteStackFrameDetails. This class isn't that complicated now so it probably isn't worth it but if it creeps in complexity over time, maybe we reconsider?

Definitely, if there turns out to be a good reason for it we can make the change.

rkeithhill · 2016-12-30T02:30:06Z

src/PowerShellEditorServices/Language/LanguageService.cs

-        private async Task<SymbolReference> FindDeclarationForBuiltinCommand(
-            CommandInfo cmdInfo, 
+        private SymbolReference FindDeclarationForBuiltinCommand(
+            CommandInfo commandInfo, 


Spelled out is better. 👍

rkeithhill · 2016-12-30T02:42:49Z

src/PowerShellEditorServices/Session/PowerShellVersionDetails.cs

+        /// </summary>
+        /// <param name="runspace">The runspace for which version details will be gathered.</param>
+        /// <returns>A new PowerShellVersionDetails instance.</returns>
+        public static PowerShellVersionDetails GetVersionDetails(Runspace runspace)


I've finally started breaking down the code in the PowerShellContext class to put it where it belongs!

rkeithhill · 2016-12-30T02:44:11Z

src/PowerShellEditorServices/Session/RunspaceDetails.cs

+    /// <summary>
+    /// Specifies the possible types of a runspace.
+    /// </summary>
+    public enum RunspaceLocale


Aha, you do have control over this name. How about RunspaceLocation?

Yeah, I was trying to find a good name for it when I thought I needed a separate designation between "location" and whether the runspace was created or attached to. Turns out that Debug-Runspace merely just blocks the current runspace's pipeline and hooks into the other runspace's debugger, so no runspace change occurs.

I think I'm just going to collapse those two enums back into one which has 3 values, Local, LocalProcess, and Remote to reflect the 3 states that I can discern via runspace changes. I think Create vs Attach should be a debugger distinction since it really only matters for breakpoint management (cleaning up breakpoints when detaching, etc).

rkeithhill · 2016-12-30T02:45:17Z

src/PowerShellEditorServices/Utility/ThreadSynchronizationContext.cs

@@ -25,6 +25,11 @@ public class ThreadSynchronizationContext : SynchronizationContext

        #region Constructors

+        public ThreadSynchronizationContext()
+        {
+            var hash = this.GetHashCode();


What does this do?

Ahhh, I forgot to take that out. I was debugging a gnarly threading issue when I first added the "attach" debug mode; the old ThreadSynchronizationContext wasn't being cleared out properly so it broke future debugging sessions.

rkeithhill · 2016-12-30T02:48:29Z

src/PowerShellEditorServices/Workspace/Workspace.cs

+                    "RemoteFiles");
+
+            // The path generated by this code will look something like
+            // %TEMP%\PSES-[PID]\RemoteFiles\1205823508\computer-name\MyFile.ps1


Do you think we need the PID? One advantage of using a hash in the name is that if the file doesn't change, debugging it a second time doesn't create a new copy of the same file on my file system. Then again, PowerShell script files usually aren't very big and folks are probably used to cleaning their temp folder from time to time.

That's a good point, though the one thing I worry about is the locally cached file being out of date. For example, the user connects to a remote machine to debug a script running there, finds a bug, fixes it and re-deploys the script. They attempt to debug the same script again remotely but since we already have the cached file we show the old version.

Maybe I could send the local copy's timestamp to the server to determine whether we need to sync, but that might be a little overkill. I'm certainly interested to go that direction in the future if the need arises.

I guess this problem will exist with the current approach if PSES somehow ends up with the same PID after X launches, or even if they change the remote file while in the same editing session. Maybe I should proactively delete that remote files folder once the remote runspace session ends?

But if you're including the hash of the file's contents in the filename, any change would result in a new hash and hence a new file. Or maybe I'm misunderstanding the "hash slug".

This is a hash of the file's folder path (sans filename) on the remote machine so a file will always end up with the same "slug" (I should use a better name) when loaded in the future so long as its path remains the same.

rkeithhill

Wow! You've been busy. I'm super excited to try this out.

daviwil · 2016-12-30T16:07:27Z

I'm happy with how it's turned out so far. There's a few rough edges to smooth out but the experience is pretty solid at the moment. Fingers crossed that it works correctly on Linux/macOS ;)

daviwil · 2017-01-06T04:45:10Z

Finally got the changes finished up! Take a look at the latest commit if you're interested, I'll merge it tomorrow at some point. I'm feeling pretty good about the stability and experience here. Tomorrow I'll also send a PR for the extension changes to add the new debug configurations, etc.

daviwil · 2017-01-06T04:52:01Z

Weird test failures, not sure what I broke in some cases. I'll take a look tomorrow.

rkeithhill

LGTM.

rkeithhill · 2017-01-06T05:15:24Z

src/PowerShellEditorServices/Debugging/DebugService.cs

@@ -784,7 +849,7 @@ private bool AddToAutoVariables(PSVariable psvariable, string scope)
                        if (hitCount.HasValue)
                        {
                            string globalHitCountVarName =
-                                $"$global:__psEditorServices_BreakHitCounter_{breakpointHitCounter++}";
+                                $"$global:{PsesGlobalVariableNamePrefix}BreakHitCounter_{breakpointHitCounter++}";


rkeithhill · 2017-01-06T05:19:48Z

src/PowerShellEditorServices/Session/RunspaceDetails.cs

+    /// <summary>
+    /// Specifies the possible types of a runspace.
+    /// </summary>
+    public enum RunspaceLocation


daviwil · 2017-01-06T05:35:36Z

Managed to fix the test issues, now I just need to verify that the changes work correctly across the range of supported PS versions and I'll get it merged!

This change finishes support for Enter-PSSession and Enter-PSHostProcess by wiring up the AttachRequest so that both of these commands can be used to attach to a runspace in a process on a local or remote machine. It also enables the LaunchRequest to be executed without a script file path to enable an interactive debugging session experience.

This change adds a new RemoteFileManager class which manages files which are opened when a remote session is active. It handles the gathering of remote file contents and mapping remote files paths to a local cache. It also keeps track of the remote files which were opened in the editor and closes them once the remote session has been exited.

Saw a PR on this in the PowerShellPracticeAndStyle project. PoshCode/PowerShellPracticeAndStyle#72 I agree.

Add support for Enter-PSSession and Enter-PSHostProcess

ffe7c61

This change adds support for remote sessions (PSv4+) and attaching to processes on the same machine (PSv5+).

daviwil added this to the 0.9.0 milestone Dec 30, 2016

msftclas added the cla-not-required label Dec 30, 2016

rkeithhill reviewed Dec 30, 2016

View reviewed changes

rkeithhill approved these changes Dec 30, 2016

View reviewed changes

rkeithhill approved these changes Jan 6, 2017

View reviewed changes

daviwil force-pushed the daviwil/remote-sessions branch from 05a7976 to 931a35d Compare January 6, 2017 05:32

daviwil added 2 commits January 6, 2017 10:55

daviwil force-pushed the daviwil/remote-sessions branch from 931a35d to 22f17e3 Compare January 9, 2017 22:23

daviwil changed the title ~~WIP: Enable language and debugging features to use remote and attached runspaces~~ Enable language and debugging features to use remote and attached runspaces Jan 9, 2017

daviwil merged commit ffa27f8 into develop Jan 9, 2017

daviwil deleted the daviwil/remote-sessions branch January 9, 2017 22:40

TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019

Add space between param and (. (PowerShell#330)

8667d80

Saw a PR on this in the PowerShellPracticeAndStyle project. PoshCode/PowerShellPracticeAndStyle#72 I agree.

Enable language and debugging features to use remote and attached runspaces #330

Enable language and debugging features to use remote and attached runspaces #330

Uh oh!

Conversation

daviwil commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill left a comment

Choose a reason for hiding this comment

Uh oh!

daviwil commented Dec 30, 2016

Uh oh!

daviwil commented Jan 6, 2017

Uh oh!

daviwil commented Jan 6, 2017

Uh oh!

rkeithhill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Jan 6, 2017

Uh oh!

Uh oh!