NamedPipeConnectionInfo <= Enter-PSHostProcess #881

TylerLeonhardt · 2019-03-18T20:37:30Z

Before, we relied on Enter-PSHostProcess... I noticed that this wasn't working and instead the GeRunspace function would return results from the current runspace instead.

This switches to using NamedPipeConnectionInfo API way of invoking something on a remote runspace.

TylerLeonhardt · 2019-03-18T20:38:28Z

@SeeminglyScience let me know if you see anything wrong with this in the scope of the threading model... I think it will be fine since it's a totally separate runspace...

rkeithhill · 2019-03-18T21:13:54Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+                else
+                {
+                    var psCommand = new PSCommand().AddCommand("Get-Runspace");
+                    StringBuilder sb = new StringBuilder();


Use var here - channeling my inner @rjmholt. :-)

rkeithhill · 2019-03-18T21:18:12Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

                {
-                    foreach (var p in runspaces)
+                    // Create a remote runspace that we will invoke Get-Runspace in
+                    using(var rs = RunspaceFactory.CreateRunspace(new NamedPipeConnectionInfo(pid)))


Will this work with all supported versions of PowerShell?

Good catch... this use to be a runtime check using Enter-PSHostProcess but now it's an assembly load check...

This will work in PowerShell 5.1+ so it's not a problem in this PR.

But when I backport I need to ifdef this properly...

SeeminglyScience

LGTM :)

Just some optional nits

SeeminglyScience · 2019-03-19T23:38:12Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+                        rs.Open();
+                        ps.Runspace = rs;
+                        // returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.
+                        runspaces = ps.AddCommand("Get-Runspace").Invoke<PSObject>();


Suggested change

runspaces = ps.AddCommand("Get-Runspace").Invoke<PSObject>();

runspaces = ps.AddCommand("Microsoft.PowerShell.Utility\\Get-Runspace").Invoke<PSObject>();

SeeminglyScience · 2019-03-19T23:38:43Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

                }
+                else
+                {
+                    var psCommand = new PSCommand().AddCommand("Get-Runspace");


Suggested change

var psCommand = new PSCommand().AddCommand("Get-Runspace");

var psCommand = new PSCommand().AddCommand("Microsoft.PowerShell.Utility\\Get-Runspace");

SeeminglyScience · 2019-03-19T23:39:24Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+                if (int.TryParse(processId, out int pid))
                {
-                    foreach (var p in runspaces)
+                    // Create a remote runspace that we will invoke Get-Runspace in


Suggested change

// Create a remote runspace that we will invoke Get-Runspace in

// Create a remote runspace that we will invoke Get-Runspace in.

SeeminglyScience · 2019-03-19T23:39:42Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

-                            });
+                        rs.Open();
+                        ps.Runspace = rs;
+                        // returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.


Suggested change

// returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.

// Returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.

SeeminglyScience · 2019-03-19T23:40:13Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

-                IEnumerable<Runspace> runspaces = await editorSession.PowerShellContext.ExecuteCommandAsync<Runspace>(psCommand, sb);
-                if (runspaces != null)
+                // If the processId is a valid int, we need to run Get-Runspace within that process
+                // otherwise just use the current runspace


Suggested change

// otherwise just use the current runspace

// otherwise just use the current runspace.

SeeminglyScience · 2019-03-19T23:42:05Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

-                    await editorSession.PowerShellContext.ExecuteCommandAsync(exitCommand);
+            if (runspaces != null)
+            {
+                foreach (dynamic p in runspaces)


Consider using a more descriptive variable name like runspace or even pso.

Edit: didn't realize this was existing code, but maybe still worth fixing.

* backport #881 in a different way * simpler * switch back to reflecting * address feedback * more feedback * safe check

NamedPipeConnectionInfo <= Enter-PSHostProcess

f0cbeef

TylerLeonhardt requested review from SeeminglyScience, rjmholt and rkeithhill March 18, 2019 20:37

rkeithhill reviewed Mar 18, 2019

View reviewed changes

var

225a371

This was referenced Mar 18, 2019

WIP: Test cherry picks #882

Closed

WIP Cherry pick for legacy #883

Merged

SeeminglyScience approved these changes Mar 19, 2019

View reviewed changes

address comments

fba3022

TylerLeonhardt merged commit f3d32cd into PowerShell:master Mar 20, 2019

TylerLeonhardt deleted the dont-rely-on-enter-pshostprocess branch March 20, 2019 16:52

TylerLeonhardt added the Backport-Legacy label Mar 20, 2019

TylerLeonhardt added a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Mar 20, 2019

backport PowerShell#881 in a different way

e033118

TylerLeonhardt mentioned this pull request Mar 20, 2019

backport #881 in a different way #884

Merged

TylerLeonhardt added a commit that referenced this pull request Mar 21, 2019

backport #881 in a different way (#884)

a96b282

* backport #881 in a different way * simpler * switch back to reflecting * address feedback * more feedback * safe check

TylerLeonhardt added Backport-Complete and removed Backport-Legacy labels Mar 21, 2019

	runspaces = ps.AddCommand("Get-Runspace").Invoke<PSObject>();
	runspaces = ps.AddCommand("Microsoft.PowerShell.Utility\\Get-Runspace").Invoke<PSObject>();

	var psCommand = new PSCommand().AddCommand("Get-Runspace");
	var psCommand = new PSCommand().AddCommand("Microsoft.PowerShell.Utility\\Get-Runspace");

	// Create a remote runspace that we will invoke Get-Runspace in
	// Create a remote runspace that we will invoke Get-Runspace in.

	// returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.
	// Returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later.

	// otherwise just use the current runspace
	// otherwise just use the current runspace.

NamedPipeConnectionInfo <= Enter-PSHostProcess #881

NamedPipeConnectionInfo <= Enter-PSHostProcess #881

Uh oh!

Conversation

TylerLeonhardt commented Mar 18, 2019

Uh oh!

TylerLeonhardt commented Mar 18, 2019

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!

TylerLeonhardt Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience 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!

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!

SeeminglyScience Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TylerLeonhardt Mar 18, 2019 •

edited

Loading

SeeminglyScience Mar 19, 2019 •

edited

Loading