Adds ability to use separate pipes for reading and writing #785

ant-druha · 2018-10-24T07:34:36Z

Some clients have blocking File I/O API, thus it is not possible for them to use single named pipe for reading and writing asynchronously.
E.g. this is needed for my PowerShell plugin for IntelliJ IDEs.
For communication protocol I'm using lsp4j library which has no issues with tcp protocol but it does not support named pipes. So the proposed solution was to use two separate pipes.

This request adds the ability to use separate pipes for reading and writing when the client specifies the "-SplitInOutPipes" switch for the startup script.

Tested that two pipes approach with my plugin (on OS X and Win7) works fine. Also tested with VS Code on OS X and Win7 also seems to work fine.

Some clients (e.g. Java on Windows) have blocking File I/O API, thus it is not possible for them to use single named pipe for reading and writing asynchronously. It will add the ability to use separate pipes for reading and writing when the client specifies the "-SplitInOutPipes" switch for the startup script.

msftclas · 2018-10-24T07:34:47Z

All CLA requirements met.

module/PowerShellEditorServices/Start-EditorServices.ps1

TylerLeonhardt

This looks really good! Just a couple questions.

I'm really excited to get this in and have another editor that uses the latest version of PSES 😄

TylerLeonhardt · 2018-10-24T15:54:37Z

src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerListener.cs

+                    {
+                        this.writePipeServer = new NamedPipeServerStream(
+                            pipeName: writePipeName,
+                            direction: PipeDirection.InOut,


Should the direction here be just PipeDirection.In?

module/PowerShellEditorServices/PowerShellEditorServices.psm1

module/PowerShellEditorServices/Start-EditorServices.ps1

TylerLeonhardt · 2018-10-24T16:11:04Z

src/PowerShellEditorServices.Host/EditorServicesHost.cs

@@ -463,7 +463,19 @@ private IServerListener CreateServiceListener(MessageProtocolType protocol, Edit

                case EditorServiceTransportType.NamedPipe:
                {
-                    return new NamedPipeServerListener(protocol, config.Endpoint, this.logger);
+                    string endpoint = config.Endpoint;
+                    int splitIndex = endpoint.IndexOf(';');


I'm more inclined to say that the character that separates the two file names should be the DirectorySeparator (System.IO.Path.DirectorySeparatorChar) since ; could be a part of a valid file name. I don't think GetRandomFileName would ever spit out a ; but at least if we use the DirectorySeparatorChar we can be sure.

I spoke to @rjmholt about the use of Endpoint here. Historically, this was used to support TCP and NamedPipes but since we don't have TCP anymore, we could change this EditorServiceTransportConfig to have a Pipe and WritePipe property (but named better) that way you don't have to do this string splitting.

I think I'm ok if you don't want to do this in this PR - but it's pretty important so if you want to take it on I'd really appreciate it.

rjmholt

Thanks for this work @ant-druha.

I've left a couple of comments, but they aren't really blocking.

The big thing is that our start script and configuration are starting to get pretty complex and I think we (tagging @tylerl0706, @SeeminglyScience, @rkeithhill) should discuss a cleaner way to pass configuration in and start EditorServices, specifically what it should look like and what breaking changes it would involve. Just because it strikes me that the combinatorics of all the configuration we're trying to juggle now are starting to get spaghettified, and with v2 on the horizon and more clients wanting to depend on us, we have an opportunity to simplify it all.

But all good for this PR.

rjmholt · 2018-10-24T17:40:42Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

@@ -106,12 +112,24 @@ function Start-EditorServicesHost {

    if ($LanguageServiceNamedPipe) {
        $languageServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
-        $languageServiceConfig.Endpoint = "$LanguageServiceNamedPipe"
+        if ($LanguageServiceWriteNamedPipe) {
+            $languageServiceConfig.Endpoint = "$LanguageServiceNamedPipe;$LanguageServiceWriteNamedPipe"


This way of specifying two pipe names in a single string here works, but I'm not a huge fan of it.

Getting around it without a breaking change doesn't seem all that simple, but I think we should revisit this soon. Otherwise all our config options are going to end up named something abstract and needing a bunch of preprocessing to unspool, even though we have a typed, in-memory .NET API.

Not suggesting this be changed in any way other than @tylerl0706's suggestion, but I don't think this should be the long term solution.

Perhaps something to re-visit for v2?

rjmholt · 2018-10-24T17:43:28Z

module/PowerShellEditorServices/Start-EditorServices.ps1

@@ -269,6 +271,9 @@ try {
    $languageServiceTransport = $null
    $debugServiceTransport = $null

+    $LanguageServiceWritePipeName = $null


Are these lines here needed? Or would leaving them out have the same effect? Since they are PascalCased, I'm assuming they are script parameters, so the nulling could be done in the parameter definition if required.

Not a script parameter. This variable is a local so it should be camelCased - $languageServiceWritePipeName

Ditto for $DebugServiceWritePipeName below.

Agree, missed this. Will make them as script parameters and move to a separate parameter set.

rjmholt · 2018-10-24T17:45:23Z

module/PowerShellEditorServices/Start-EditorServices.ps1

-        if ($IsLinux -or $IsMacOS) {
-            Set-NamedPipeMode -PipeFile $resultDetails["languageServicePipeName"]
+        if ($LanguageServiceWritePipeName) {
+            $resultDetails["languageServiceReadPipeName"] = Get-NamedPipePath -PipeName $LanguageServicePipeName


It looks like this pattern is repeated 3 times. Would it be possible to make it a function?

rjmholt · 2018-10-24T17:49:13Z

src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerListener.cs

        }

        private void ListenForConnection()
        {
-            Task.Factory.StartNew(
-                async () =>
+            List<Task> connectionTasks = new List<Task> {WaitForConnectionAsync(this.pipeServer)};


Can use var here on the LHS instead of List<Task>.

I used var at first :) but having a look at other code here thought it is a coding style to use explicit types (including local vars). Can make it var if you'd like.

Ah, yeah the coding style in this project isn't great at the moment -- we've been trying to move it over gradually to being less redundant and verbose.

rkeithhill · 2018-10-25T02:35:58Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

@@ -106,12 +112,24 @@ function Start-EditorServicesHost {

    if ($LanguageServiceNamedPipe) {


If we used parameter sets then here we'd check if ($PSCmdlet.ParameterSetName -eq 'NamedPipeDuplex') and perhaps $LanguageServicePipe would be a mandatory parameter? Not sure about $DebugLanguageServicePipe, I suppose you could create one but not the other?

… transport config endpoint

…ite pipes

ant-druha · 2018-10-27T10:34:59Z

So, I have organised startup script parameters into parameter sets (had to move code around a bit for this, sorry).
Probably it is better to introduce an enumeration parameter for transport type, with names like e.g. "Stdio", "NamedPipe", "NamedPipeHalfDuplex" and optional parameters for all the pipes, getting rid of StdIo and SplitPipes switches. Then it would be simpler to work with it in script.

Also introduced separate properties for pipes in EditorServiceTransportType.

I hope it is suffice for this PR to make it more or less better.

Looking forward to a new release :)

rjmholt · 2018-10-30T20:28:52Z

module/PowerShellEditorServices/PowerShellEditorServices.psm1

        [string]
        $DebugServiceNamedPipe,

+        [Parameter(ParameterSetName="NamedPipeHalfDuplex",Mandatory=$true)]


Technically, I think this may be "simplex" rather than "half duplex" since each pipe can only ever be used in one direction (rather than both directions, one at a time), but it's not a major concern

module/PowerShellEditorServices/PowerShellEditorServices.psm1

rjmholt · 2018-10-30T20:44:13Z

module/PowerShellEditorServices/Start-EditorServices.ps1

+    [string]
+    $DebugServicePipeName = $null,
+
+    [Parameter(ParameterSetName="NamedPipeHalfDuplex",Mandatory=$true)]


Setting Mandatory=$true for a switch parameter may be unneeded, particularly for one in its own parameter set.

rjmholt · 2018-10-30T20:46:03Z

module/PowerShellEditorServices/Start-EditorServices.ps1

+    [switch]
+    $SplitInOutPipes,
+
+    [Parameter(ParameterSetName="NamedPipeHalfDuplexParam",Mandatory=$true)]


What's the reason for the "Param" parameter set? Is it so that you can either set a switch to split the pipe or nominate a pipename yourself?

I feel like we probably don't need so many different parameter sets -- what is the envisioned usage of each current set?

What's the reason for the "Param" parameter set? Is it so that you can either set a switch to split the pipe or nominate a pipename yourself?

I feel like we probably don't need so many different parameter sets -- what is the envisioned usage of each current set?

Right. I wanted to enforce the contract that one could either specify SplitInOutPipes Switch or be enforced to specify read/write pipe names directory. Ok, simplified the parameter sets to just one additional set for simplex named pipes.

rjmholt · 2018-10-30T20:49:21Z

module/PowerShellEditorServices/Start-EditorServices.ps1

+    return $PipeName
+}
+
+function SetPipeFileResult($ResultTable, [string]$PipeNameKey, [string]$PipeNameValue) {


This function should separate the verb Set with a dash in the name, and also use a param block inside the body rather than take parameters after the function name.

If result table is expected to be a hashtable, it might be worth adding that to the param signature as well.

SeeminglyScience

LGTM! Thanks @ant-druha!

Just a few optional nit picks and suggestions.

SeeminglyScience · 2018-11-01T11:39:50Z

module/PowerShellEditorServices/Start-EditorServices.ps1

@@ -15,7 +15,7 @@
 #       Services GitHub repository:
 #
 #       https://github.com/PowerShell/PowerShellEditorServices/blob/master/module/PowerShellEditorServices/Start-EditorServices.ps1
-
+[Cmdletbinding(DefaultParameterSetName="NamedPipe")]


Suggested change

[Cmdletbinding(DefaultParameterSetName="NamedPipe")]

[CmdletBinding(DefaultParameterSetName="NamedPipe")]

SeeminglyScience · 2018-11-01T11:44:27Z

src/PowerShellEditorServices.Host/EditorServicesHost.cs

+        public string InOutPipeName { get; set; }
+        public string OutPipeName { get; set; }
+        public string InPipeName { get; set; }
+        internal string Endpoint => OutPipeName != null && InPipeName!=null ? $"In pipe: {InPipeName} Out pipe: {OutPipeName}" : $" InOut pipe: {InOutPipeName}";


Suggested change

internal string Endpoint => OutPipeName != null && InPipeName!=null ? $"In pipe: {InPipeName} Out pipe: {OutPipeName}" : $" InOut pipe: {InOutPipeName}";

internal string Endpoint => OutPipeName != null && InPipeName != null ? $"In pipe: {InPipeName} Out pipe: {OutPipeName}" : $" InOut pipe: {InOutPipeName}";

SeeminglyScience · 2018-11-01T11:45:34Z

src/PowerShellEditorServices.Host/EditorServicesHost.cs

-                    return new NamedPipeServerListener(protocol, config.Endpoint, this.logger);
+                    if (config.OutPipeName !=null && config.InPipeName !=null)
+                    {
+                        this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: In: '" + config.InPipeName + "'. Out: '" + config.OutPipeName + "'");


Suggested change

this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: In: '" + config.InPipeName + "'. Out: '" + config.OutPipeName + "'");

this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: In: '{config.InPipeName}'. Out: '{config.OutPipeName}'");

Is the $ supposed to be there before {protocol}?

There could be $DebugAdapter or $LanguageServer protocols. Example log:

Creating NamedPipeServerListener for $LanguageServer protocol with two pipes: In: 'PSES_saem21o3.ere'. Out: 'PSES_dmxrnado.rh5' ... Creating NamedPipeServerListener for $DebugAdapter protocol with two pipes: In: 'PSES_jzzah5uk.4ek'. Out: 'PSES_w3ablqxx.uyb'

SeeminglyScience · 2018-11-01T11:46:58Z

src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerChannel.cs

            ILogger logger)
        {
-            this.pipeServer = pipeServer;
+            this.inOutPipeServer = inOutPipeServer;
+            this.outPipeServer = null;


nit: This line can be removed, the field will already be initialized with null by default.

SeeminglyScience · 2018-11-01T11:47:48Z

src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerListener.cs

+        {
+            this.logger = logger;
+            this.inOutPipeName = inOutPipeName;
+            this.outPipeName = null;


nit: re: field initialization not needed

TylerLeonhardt · 2018-11-07T22:29:42Z

@ant-druha do you think you'll be able to address this feedback? I just want to make sure this PR doesn't get lost because it's super important!

ant-druha · 2018-11-08T11:29:45Z

@ant-druha do you think you'll be able to address this feedback? I just want to make sure this PR doesn't get lost because it's super important!

Hi @TylerLeonhardt, sorry for the silence! Haven't had time to look at it, yet. Yes I will address the feedback this week or weekend, I hope it is okay.

# Conflicts: # module/PowerShellEditorServices/Start-EditorServices.ps1

TylerLeonhardt · 2018-11-15T03:52:47Z

module/PowerShellEditorServices/Start-EditorServices.ps1

+        $PipeName = New-NamedPipeName
+    }
+    else {
+        if (-not (Test-NamedPipeName -PipeName $PipeName)) {


So what happens when Test-NamedPipeName equals true? Will it return null? Should that ever happen?

I feel as if the Test & Create part should be two separate functions. Separation of concerns

The function will fail and abort with the error if the supplied by the user pipe name is not a valid name. If a valid name was supplied it just returns this pipe name. If no pipe name was provided it creates it.

rkeithhill · 2018-11-16T02:15:05Z

src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerListener.cs

+                        "An unhandled exception occurred while listening for a named pipe client connection",
+                        e);
+
+                    throw e;


Rethrow should never specify the exception - you'll lose stack info. Use throw; by itself.

Ideally our logger methods would return false so you could use C# exception filters e.g.:

catch (Exception e) when (this.logger.WriteException("...", e)) { }

But we don't do that - yet. @rjmholt Any chance we could modify the log methods to return false for this reason?

Yeah, we could definitely modify the methods.

Or else write wrapper methods that explain the intended caller context:

catch (Exception e) when (_logger.WriteExceptionWithoutUnwinding("...", e)) { }

Bit of a mouthful -- just some name that says "I return false for use with exception filters".

ant-druha · 2018-11-21T05:29:39Z

Hi all. Can this PR be merged? Are there any blockers?

TylerLeonhardt

LGTM, I removed the 'e' per @rkeithhill's recommendation

TylerLeonhardt · 2018-11-21T19:48:34Z

Once @rkeithhill is good with this, we can merge it in

TylerLeonhardt · 2018-11-22T00:30:00Z

@ant-druha all merged! Let me know when your extension is updated with this. I'd love to spread the word!

ant-druha requested review from rjmholt, SeeminglyScience and TylerLeonhardt as code owners October 24, 2018 07:34

ant-druha commented Oct 24, 2018

View reviewed changes

module/PowerShellEditorServices/Start-EditorServices.ps1 Show resolved Hide resolved

ant-druha mentioned this pull request Oct 24, 2018

Support Named Pipes as Language Server transport and update PSES version ant-druha/intellij-powershell#6

Closed

TylerLeonhardt requested a review from rkeithhill October 24, 2018 15:48

TylerLeonhardt reviewed Oct 24, 2018

View reviewed changes

rjmholt approved these changes Oct 24, 2018

View reviewed changes

rkeithhill reviewed Oct 25, 2018

View reviewed changes

Andrey Dernov added 4 commits October 27, 2018 09:34

Use Path.DirectorySeparatorChar as read/write pipe name separator for…

ea65162

… transport config endpoint

WritePipeServer: pipe direction should be "Out"

8d20b53

Organize startup script parameters by parameter sets

c47e80b

EditorServiceTransportType: Introduce separate properties for read/wr…

4e747a1

…ite pipes

rjmholt reviewed Oct 30, 2018

View reviewed changes

SeeminglyScience approved these changes Nov 1, 2018

View reviewed changes

Andrey Dernov added 2 commits November 10, 2018 13:10

Merge remote-tracking branch 'upstream/master'

0189966

# Conflicts: # module/PowerShellEditorServices/Start-EditorServices.ps1

[ PowerShell#785 PR ] Cleanup: refactorings, code style and reformat

569c223

TylerLeonhardt reviewed Nov 15, 2018

View reviewed changes

rkeithhill reviewed Nov 16, 2018

View reviewed changes

remove e in excpe

93a391f

TylerLeonhardt approved these changes Nov 21, 2018

View reviewed changes

rkeithhill approved these changes Nov 21, 2018

View reviewed changes

TylerLeonhardt merged commit 487ad1d into PowerShell:master Nov 22, 2018

		@@ -106,12 +112,24 @@ function Start-EditorServicesHost {

		if ($LanguageServiceNamedPipe) {

	[Cmdletbinding(DefaultParameterSetName="NamedPipe")]
	[CmdletBinding(DefaultParameterSetName="NamedPipe")]

	internal string Endpoint => OutPipeName != null && InPipeName!=null ? $"In pipe: {InPipeName} Out pipe: {OutPipeName}" : $" InOut pipe: {InOutPipeName}";
	internal string Endpoint => OutPipeName != null && InPipeName != null ? $"In pipe: {InPipeName} Out pipe: {OutPipeName}" : $" InOut pipe: {InOutPipeName}";

	this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: In: '" + config.InPipeName + "'. Out: '" + config.OutPipeName + "'");
	this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: In: '{config.InPipeName}'. Out: '{config.OutPipeName}'");

Adds ability to use separate pipes for reading and writing #785

Adds ability to use separate pipes for reading and writing #785

Conversation

ant-druha commented Oct 24, 2018 • edited Loading

msftclas commented Oct 24, 2018 • 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

rjmholt 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

ant-druha Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ant-druha commented Oct 27, 2018

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

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

TylerLeonhardt commented Nov 7, 2018

ant-druha commented Nov 8, 2018

TylerLeonhardt Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ant-druha commented Nov 21, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Nov 21, 2018

TylerLeonhardt commented Nov 22, 2018

ant-druha commented Oct 24, 2018 •

edited

Loading

msftclas commented Oct 24, 2018 •

edited

Loading

ant-druha Oct 25, 2018 •

edited

Loading

TylerLeonhardt Nov 15, 2018 •

edited

Loading

rkeithhill Nov 16, 2018 •

edited

Loading