EditorServiceHost: allow Tcp/NamedPipe/Stdio listeners #629

yatli · 2018-02-27T13:38:56Z

Refactored listeners and channels to be a bit more generic. The auxiliary scripts are also updated to support for example, Stdio server from outside.

yatli · 2018-02-27T13:43:48Z

oops, looks like some tests are failing? looking into it.

channel

yatli · 2018-02-28T14:38:06Z

AppVeyor timeout?

TylerLeonhardt · 2018-02-28T18:33:36Z

Thanks for this! Would you mind supplying the steps you used to verify/test this?

I'm interested in the "Stdio server from outside" part.

yatli · 2018-03-01T04:14:07Z

@tylerl0706 check this out: https://github.com/v-yadli/vimrc/blob/nvim/init.vim

here are the steps with my vimrc noise cancelled:

Install neovim
Use a plugin system (recommended) to install: junegunn/fzf.vim, autozimu/LanguageClient-neovim
Build PSES locally at ~\git\PowerShellEditorServices
Add the server startup command to the neovim language service client like this:

"LanguageServerProtocol setup
"required for operations modifying multiple buffers like rename.
set hidden
"setup startup sequences...
let g:LanguageClient_serverCommands = {
    \ 'haskell': ['hie', '--lsp'],
    \ 'python': ['pyls'],
    \ 'cs': ['~\.omnisharp\OmniSharp.exe'],
    \ 'ps1': ['powershell', '~\git\PowerShellEditorServices\module\Start-EditorServices.ps1', '-HostName', 'nvim', '-HostProfileId', '0', '-HostVersion', '1.0.0', '-EditorServicesVersion', '1.6.0', '-LogPath', 'pses.log.txt', '-LogLevel', 'Normal', '-BundledModulesPath', '~\git\PowerShellEditorServices\module', '-Stdio'],
\ }

Now fire up a new instance of neovim and try editing a powershell script. use Ex command :call LanguageClient_textDocument_references() on a symbol to trigger reference finding, which will bring up a fzf prompt if you've got it installed. It's okay if fzf is not around -- the standard quickfix will be used instead.

yatli · 2018-03-01T04:15:53Z

This is only partially working at the moment, from pses log I see some null reference exceptions while trying to do completion, and handling document change events. Still looking into it but I think that's another issue.

…amed pipe on start.

yatli · 2018-03-01T06:08:06Z

Fixed it. Not a lot of changes so I guess we can merge it altogether?
Now it works happily with vim :)

TylerLeonhardt · 2018-03-15T01:18:43Z

@daviwil can you look at this when you get a chance? I remember you saying that you had comments.

TylerLeonhardt · 2018-03-15T01:19:22Z

Oh and FWIW, I was able to get NeoVim working with PSES using this PR. 🎉 🎉

nanoant · 2018-03-16T17:31:19Z

@yatli @tylerl0706 Just wanted to say that this PR makes PSES work like a charm with Sublime Text 3 LSP plugin https://github.com/tomv564/LSP 🎉 😄

I have used following LSP.sublime-settings (just replicated from VIM solution above).

{
  "clients":
  {
    "poshls":
    {
      "command": [
        "powershell",
        "~\\Git\\PowerShellEditorServices\\module\\Start-EditorServices.ps1",
        "-HostName", "nvim",
        "-HostProfileId", "0",
        "-HostVersion", "1.0.0",
        "-EditorServicesVersion", "1.6.0",
        "-LogPath", "~\\Git\\PowerShellEditorServices\\pses.log.txt",
        "-LogLevel", "Normal",
        "-BundledModulesPath", "~\\Git\\PowerShellEditorServices\\module",
        "-Stdio"
      ],
      "scopes": ["source.powershell"],
      "syntaxes": ["Packages/PowerShell/Support/PowershellSyntax.tmLanguage"],
      "languageId": "powershell"
    }
  }
}

SeeminglyScience

This is really awesome. Thank you so much for working on this! This is a big step towards getting a whole lot more PSES implementations :)

Just a couple of nit picks, mostly on the PowerShell side.

SeeminglyScience · 2018-03-19T11:33:39Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

        [int]
        $DebugServicePort,

+        [bool]
+        $Stdio,


Can we make this parameter a [switch] parameter instead of [bool]?

Also, might be better to change name to UseStdio.

@SeeminglyScience I tried to make it a switch as the outer starting script does, but I don't know how to relay the switch in without splitting the command into two, one with the switch and one without. Sorry I'm still learning my way on powershell 😄

Typically it is -InnerSwitch:$OuterSwitch

SeeminglyScience · 2018-03-19T11:41:16Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

+        $LanguageServiceNamedPipe,
+
+        [string]
+        $DebugServiceNamedPipe,


Could the different connection types be separate parameter sets? For example

[Parameter(Mandatory, ParameterSetName='ByTcpPort')] [ValidateNotNull()] [int] $LanguageServicePort, [Parameter(Mandatory, ParameterSetName='ByTcpPort')] [ValidateNotNull()] [int] $DebugServicePort, [Parameter(Mandatory, ParameterSetName='ByNamedPipe')] [ValidateNotNullOrEmpty()] [string] $LanguageServiceNamedPipe, [Parameter(Mandatory, ParameterSetName='ByNamedPipe')] [ValidateNotNullOrEmpty()] [string] $DebugServiceNamePipe, # etc...

SeeminglyScience · 2018-03-19T11:48:28Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

+    if ($DebugServiceNamedPipe) {
+        $debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
+        $debugServiceConfig.Endpoint = "$DebugServiceNamedPipe"
+    }


With parameter sets this would work nicely as a switch statement

switch ($PSCmdlet.ParameterSetName) { ByTcp { $languageServiceConfig.TransportType = $debugServiceConfig.TransportType = Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::Tcp $languageServiceConfig.Endpoint = $LanguageServicePort $debugServiceConfig.Endpoint = $DebugServicePort } ByNamedPipe { $languageServiceConfig.TransportType = $debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe $languageServiceConfig.Endpoint = $LanguageServiceNamedPipe $debugServiceConfig.Endpoint = $DebugServiceNamedPipe } # etc... }

ooo that's nice 😍

Agree with @SeeminglyScience but if you'd prefer we can add the parameter sets after this gets merged.

ah this is totally cool!

I just thought about this for a while.. A few days ago I was debugging a problem when both language service and debug service are talking to stdio and the channel gets too crowded. In fact these two should never sit on the same stdio train. :)

SeeminglyScience · 2018-03-19T11:54:36Z

module/Start-EditorServices.ps1

        -BundledModulesPath $BundledModulesPath `
+        -DebugServiceOnly:$DebugServiceOnly.IsPresent`
        -WaitForDebugger:$WaitForDebugger.IsPresent


Assuming the switch to parameter sets, this would need to be switched to a splat (probably should anyway to fit common community style guides)

$splat = @{ AdditionalModules = @() WaitForDebugger = $WaitForDebugger.IsPresent # etc... } switch ($PSCmdlet.ParameterSetName) { ByTcp { $splat.LanguageServicePort = $languageServicePort # etc... } } $editorServicesHost = Start-EditorServicesHost @splat

SeeminglyScience · 2018-03-19T12:03:53Z

src/PowerShellEditorServices/Workspace/FileChange.cs

+        /// of the content, and all stale content and metadata
+        /// should be discarded.
+        /// </summary>
+        public bool Reloaded { get; set; }


This one is really nit picky, but could we change this member to IsReload

TylerLeonhardt · 2018-03-19T12:24:44Z

Everything looks great! I'll give it a really good look later today but wanted to ask if you could fix this PSModulePath casing as a part of your PR:

https://github.com/PowerShell/PowerShellEditorServices/pull/629/files#diff-12fdf91ec62a52be37451ffb5af7ffb4R138

Totally optional but figured I'd ask since you're already touching the file.

TylerLeonhardt · 2018-03-19T23:23:24Z

@yatli just curious, did you have an example that uses named pipes? 😄

TylerLeonhardt

Agree with all of @SeeminglyScience's comments :) other than those, LGTM!

Super excited to get this in 😄

rkeithhill · 2018-03-19T23:52:49Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

+
+    if ($DebugServiceOnly.IsPresent) {
+        $editorServicesHost.StartDebugService($debugServiceConfig, $profilePaths, $false);
+    } else {


Not a huge thing but the script style for this file doesn't use cuddled else. Again, something we can tweak after merging.

rkeithhill

LGTM. There are some tweaks that could be done but like I mention in my comments, we could do those after merging.

yatli · 2018-03-20T02:02:19Z

@yatli just curious, did you have an example that uses named pipes? 😄

@tylerl0706 nak, I just tested it working with a dummy client, with captured payload from another run :D

yatli · 2018-03-20T02:07:53Z

Everything looks great! I'll give it a really good look later today but wanted to ask if you could fix this PSModulePath casing as a part of your PR:
https://github.com/PowerShell/PowerShellEditorServices/pull/629/files#diff-12fdf91ec62a52be37451ffb5af7ffb4R138
Totally optional but figured I'd ask since you're already touching the file.

Digging gitter chat history, I remember we discussed something about it before

yatli · 2018-03-20T02:13:34Z

ok, got it, $env:PsModulePath should be camel case.

TylerLeonhardt · 2018-04-19T06:36:58Z

that said, we should still fix the test failures before merging.

We also should verify that this PSES still works in VSCode (it should, but just in case), but I can test that for you 🙂

yatli · 2018-04-19T06:41:18Z

understood ^ ^
btw nice term!

yatli · 2018-04-19T06:52:17Z

I think I broke this one:

            JObject result = JObject.Parse(sessionDetailsText);
            if (result["status"].Value<string>() == "started")
            {
                return new Tuple<int, int>(
                    result["languageServicePort"].Value<int>(),
                    result["debugServicePort"].Value<int>());
            }

but now, the details carry my modified info like this:

{"status":"started","debugServiceTransport":"Tcp port 28157","languageServiceTransport":"Tcp port 14630"}

TylerLeonhardt · 2018-04-19T06:58:32Z

Ah I see...

would a structure like this make sense:

{
    "status":"started",
    "debugServiceTransport":"Tcp",
    "languageServiceTransport":"Tcp",
    "languageServicePort":1234,
    "debugServicePort":1234
}

where the ports are "optional" basically

That way it's also back-compat and clients that use TCP don't have to do string manipulation to get the port values.

yatli · 2018-04-19T07:01:31Z

I'm on this. A few seconds...

yatli · 2018-04-19T07:27:09Z

@tylerl0706 they don't go away after testing:

(killing them one by one and rearm! :D)

TylerLeonhardt · 2018-04-19T07:29:26Z

oh... my...

TylerLeonhardt · 2018-04-19T07:35:57Z

so I think this might be something that needs to be handled client side... because it's really the client that's spinning up the instance of PowerShell that runs Start-EditorServices.ps1

in the case of NeoVim, adding something like this on exit?

augroup autocom
    autocmd!
    "executes the command on quit
     autocmd VimLeave *.cpp !your_command
augroup END

which I got from this.

TylerLeonhardt · 2018-04-19T07:37:48Z

I still wish we could make PSES smart enough to realize the client has disconnected and stop itself but I can't think of a good way to do that at the moment.

yatli · 2018-04-19T08:33:24Z

@tylerl0706 it passes the tests locally now. let's wait and see. :)
(sorry that's quite a few seconds! my time got fragmented)

TylerLeonhardt · 2018-04-19T16:06:20Z

No worries, @yatli! That's awesome 😄

Do you have any thoughts on how to address the extra processes?

yatli · 2018-04-20T03:21:29Z

@tylerl0706 I think this is mostly a problem of the language client plugin and it should be addressed over there. :)

isn't there a shutdown command in the LSP?

nanoant · 2018-04-20T08:37:33Z

I think this is mostly a problem of the language client plugin and it should be addressed over there. :)

@yatli sorry to differ, but current behavior is simply far what UNIX pipe programs are. I know I know "posh" comes from Windows world, but imagine you would need to send shutdown to sed otherwise it will be there forever with closed communication channel. This is just unnecessarily cumbersome. Name single stdin (pipe) processing program that lives when the pipe is closed.

When client dies or closes the pipe, or whatever happens with the channel, there is NO way to send shutdown command anymore, since it is single pipe, once closed it is gone forever. It is not a TCP you can reconnect etc. etc. So regardless if this will be addressed in VIM, expect stray processed anyway.

I think this should be addressed either as a part of this patch or soon after as it is strongly related.

yatli · 2018-04-20T08:51:52Z

@nanoant I completely agree with you on the part that stdio is a single-burn thing. My concern is that PSES is by-design multiplexed according to the owners -- it may span across multiple sessions, plus that it has multiple channels. What happens if the debug channel is using stdio, when it's broken, and the lsp channel is actually healthy on a Tcp port?

Also, I'm very curious about how this thing works in VSCode -- because it looks like VSCode is connecting to PSES by two tcp ports, and how is PSES shut down when VSCode decides to do so? -- that's how I come to the conclusion that, vim shall shutdown PSES in the same way that VSCode shall shutdown PSES.

So, like I said earlier in this thread, I do think we need to think about the termination problem -- it's not just about stdio, and all clients should expect a common practice for doing so.

btw in my current setting, my vim lsp client totally ignores PSES debug port. So there'd be a thread waiting for a connection in their house.
How is PSES started? STA or MTA?

TylerLeonhardt · 2018-04-20T21:47:28Z

Ok everyone, I did some research and here's what I've found:

Right now, the client kills the server process.

The proof can be found here in the VSCode client: https://github.com/PowerShell/vscode-powershell/blob/master/src/process.ts#L145-L161

That said, this is not a good thing.

You might be wondering... The LSP has both a shutdown and an exitmessage, don't we implement those?

The answer is yes.......... but it's functionally a no-op 😢

You can see the TODOcomment here:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L148

which is referring to this TaskCompletionSource that is causing the process to stay open:

PowerShellEditorServices/src/PowerShellEditorServices.Host/EditorServicesHost.cs

Line 341 in 4716b7f

this.serverCompletedTask = new TaskCompletionSource<bool>();

So what's the plan?

Since we currently rely on the client to kill the server, we should call that "out of scope" of this PR.

I'll open up a new issue with priority (that I will address ASAP) that actually implements these messages correctly.

P.s. big thanks to @SeeminglyScience for helping me investigate this! 😄

TylerLeonhardt · 2018-04-20T21:48:24Z

@nanoant + @yatli does that seem fair?

TylerLeonhardt · 2018-04-20T22:10:12Z

P.S. I verified that the latest changes work with VSCode 🎉

rjmholt · 2018-04-21T07:47:50Z

I've had a look through everything and this all looks really cool. Haven't had a chance to run the code yet, but will try to this weekend.

On the PSES shutdown thing: is there a way for us to keep track of how many clients we are serving and then shutdown when it goes to zero (like a Windows named pipe)?

TylerLeonhardt · 2018-04-21T15:41:39Z

how many clients

Just for information's sake, this should only be 1 or 0. 2 clients cannot share an instance of PSES because a single runspace is being managed in each PSES session.

shutdown when it goes to zero

I believe that's only possible by using a heartbeat 😟 I'm not a huge fan of that idea, but even if we wanted to do that, I'd still call that out of scope of this PR IMO.

rjmholt · 2018-04-21T16:02:22Z

I'd still call that out of scope of this PR IMO.

Definitely out of scope for this PR, yes.

this should only be 1 or 0. 2 clients cannot share an instance of PSES because a single runspace is being managed in each PSES session.

So does that mean that for a single PSES process there can only be one client? So the problem is that a client connects to PSES over multiple streams and we don't have a good answer for what to do when those streams break? That's how I'm interpreting @yatli's points about multiplexed input.

My question is, do all our proposed streams guarantee connection? I know other protocols have heartbeat messages, but the concept of a connection-oriented transport layer in my mind means we get told by the transport layer below us when a connection is broken or closed. When all connections to PSES are closed, we should just wrap up. Generally the client should give that command, but they can do it with a shutdown message. I'm assuming there's a complicating factor here that I've missed though.

TylerLeonhardt · 2018-04-21T23:54:39Z

I opened this issue to chat more about that:

#655

@nanoant, is this cool with you?

I'm going to accept the PR on Monday if no one has any concerns.

nanoant · 2018-04-23T09:30:14Z

@tylerl0706 Thanks. I am okay to continue the shutdown discussion at #655 and have this merged.

TylerLeonhardt · 2018-04-23T15:51:55Z

It's merged!! 🎉 Thank you so much @yatli!

Also thank you so much to @nanoant for all the awesome feedback. The release of this will come this week and it would be really cool to see a Sublime and NeoVim PSES guide after that 😁

TylerLeonhardt · 2018-04-26T23:09:13Z

PSES 1.7.0 is released with this change!

https://github.com/PowerShell/PowerShellEditorServices/releases/tag/v1.7.0

You should be able to download this zip and get it to work with your LSP client of choice

Yatao Li added 2 commits February 27, 2018 16:58

EditorServiceHost: allow Tcp/NamedPipe/Stdio listeners

318dbb7

update ps scripts

ae39b16

Yatao Li added 2 commits February 27, 2018 23:07

fix script and test errors;

941ab40

update startup script: there can be only one service on the stdio

ed6b722

channel

rkeithhill requested a review from daviwil February 28, 2018 05:09

TylerLeonhardt requested review from rkeithhill and SeeminglyScience February 28, 2018 18:21

1. handle file refresh where range info is absent; 2. listen to the n…

efdf05a

…amed pipe on start.

TylerLeonhardt mentioned this pull request Mar 16, 2018

Stdout/stdin (pipe) protocol support #634

Closed

daviwil approved these changes Mar 16, 2018

View reviewed changes

SeeminglyScience requested changes Mar 19, 2018

View reviewed changes

nanoant mentioned this pull request Mar 19, 2018

Add support for CompletionItem.textEdit (& additionalTextEdits) sublimelsp/LSP#294

Closed

TylerLeonhardt approved these changes Mar 19, 2018

View reviewed changes

rkeithhill reviewed Mar 19, 2018

View reviewed changes

rkeithhill approved these changes Mar 20, 2018

View reviewed changes

FileChange: Reloaded->IsReload

8de969d

update startup script, fix tests

592d29d

TylerLeonhardt mentioned this pull request Apr 20, 2018

:LanguageClientStop or :call LanguageClient#exit() not killing language server process autozimu/LanguageClient-neovim#390

Closed

TylerLeonhardt mentioned this pull request Apr 22, 2018

Change PSES to be buildable as a standalone #647

Merged

TylerLeonhardt mentioned this pull request Apr 23, 2018

Refactor Start-EditorServices.ps1 based on feedback #656

Closed

TylerLeonhardt merged commit 1067b92 into PowerShell:master Apr 23, 2018

Priddles mentioned this pull request May 28, 2019

No user documentation? #968

Open

EditorServiceHost: allow Tcp/NamedPipe/Stdio listeners #629

EditorServiceHost: allow Tcp/NamedPipe/Stdio listeners #629

Conversation

yatli commented Feb 27, 2018

yatli commented Feb 27, 2018

yatli commented Feb 28, 2018

TylerLeonhardt commented Feb 28, 2018

yatli commented Mar 1, 2018 • edited Loading

yatli commented Mar 1, 2018

yatli commented Mar 1, 2018

TylerLeonhardt commented Mar 15, 2018

TylerLeonhardt commented Mar 15, 2018

nanoant commented Mar 16, 2018

SeeminglyScience 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

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 commented Mar 19, 2018

TylerLeonhardt commented Mar 19, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

yatli commented Mar 20, 2018 • edited Loading

yatli commented Mar 20, 2018

yatli commented Mar 20, 2018

TylerLeonhardt commented Apr 19, 2018

yatli commented Apr 19, 2018

yatli commented Apr 19, 2018

TylerLeonhardt commented Apr 19, 2018 • edited Loading

yatli commented Apr 19, 2018

yatli commented Apr 19, 2018 • edited Loading

TylerLeonhardt commented Apr 19, 2018

TylerLeonhardt commented Apr 19, 2018

TylerLeonhardt commented Apr 19, 2018 • edited Loading

yatli commented Apr 19, 2018 • edited Loading

TylerLeonhardt commented Apr 19, 2018

yatli commented Apr 20, 2018 • edited Loading

nanoant commented Apr 20, 2018

yatli commented Apr 20, 2018

TylerLeonhardt commented Apr 20, 2018 • edited Loading

TylerLeonhardt commented Apr 20, 2018

TylerLeonhardt commented Apr 20, 2018

rjmholt commented Apr 21, 2018

TylerLeonhardt commented Apr 21, 2018

rjmholt commented Apr 21, 2018 • edited Loading

TylerLeonhardt commented Apr 21, 2018

nanoant commented Apr 23, 2018

TylerLeonhardt commented Apr 23, 2018

TylerLeonhardt commented Apr 26, 2018

yatli commented Mar 1, 2018 •

edited

Loading

yatli commented Mar 20, 2018 •

edited

Loading

TylerLeonhardt commented Apr 19, 2018 •

edited

Loading

yatli commented Apr 19, 2018 •

edited

Loading

TylerLeonhardt commented Apr 19, 2018 •

edited

Loading

yatli commented Apr 19, 2018 •

edited

Loading

yatli commented Apr 20, 2018 •

edited

Loading

TylerLeonhardt commented Apr 20, 2018 •

edited

Loading

rjmholt commented Apr 21, 2018 •

edited

Loading