Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 22, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9db4d7e
Adds ability to use separate pipes for reading and writing
Oct 24, 2018
ea65162
Use Path.DirectorySeparatorChar as read/write pipe name separator for…
Oct 27, 2018
8d20b53
WritePipeServer: pipe direction should be "Out"
Oct 27, 2018
c47e80b
Organize startup script parameters by parameter sets
Oct 27, 2018
4e747a1
EditorServiceTransportType: Introduce separate properties for read/wr…
Oct 27, 2018
0189966
Merge remote-tracking branch 'upstream/master'
Nov 10, 2018
569c223
[ PowerShell/PowerShellEditorServices#785 PR ] Cleanup: refactorings,…
Nov 10, 2018
93a391f
remove e in excpe
TylerLeonhardt Nov 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions module/PowerShellEditorServices/PowerShellEditorServices.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,15 @@ function Start-EditorServicesHost {
[string]
$LanguageServiceNamedPipe,

[string]
$LanguageServiceWriteNamedPipe,

[string]
$DebugServiceNamedPipe,

[string]
$DebugServiceWriteNamedPipe,

[ValidateNotNullOrEmpty()]
[string]
$BundledModulesPath,
Expand Down Expand Up @@ -106,12 +112,24 @@ function Start-EditorServicesHost {

if ($LanguageServiceNamedPipe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something to re-visit for v2?

}
else
{
$languageServiceConfig.Endpoint = "$LanguageServiceNamedPipe"
}
}

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

if ($DebugServiceOnly.IsPresent) {
Expand Down
48 changes: 40 additions & 8 deletions module/PowerShellEditorServices/Start-EditorServices.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ param(
[switch]
$Stdio,

[switch]
$SplitInOutPipes,

[string]
$LanguageServicePipeName = $null,

Expand Down Expand Up @@ -173,8 +176,7 @@ function New-NamedPipeName {

# We try 10 times to find a valid pipe name
for ($i = 0; $i -lt 10; $i++) {
# add a guid to make the pipe unique
$PipeName = "PSES_$([guid]::NewGuid())"
$PipeName = "PSES_$([System.IO.Path]::GetRandomFileName())"

if ((Test-NamedPipeName -PipeName $PipeName)) {
return $PipeName
Expand Down Expand Up @@ -269,6 +271,9 @@ try {
$languageServiceTransport = $null
$debugServiceTransport = $null

$LanguageServiceWritePipeName = $null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for $DebugServiceWritePipeName below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$DebugServiceWritePipeName = $null

if ($Stdio.IsPresent) {
$languageServiceTransport = "Stdio"
$debugServiceTransport = "Stdio"
Expand All @@ -292,6 +297,11 @@ try {
ExitWithError "Pipe name supplied is already taken: $DebugServicePipeName"
}
}
# If present create second pipe for writing
if ($SplitInOutPipes.IsPresent) {
$LanguageServiceWritePipeName = New-NamedPipeName
$DebugServiceWritePipeName = New-NamedPipeName
}
}

if ($EnableConsoleRepl) {
Expand All @@ -309,7 +319,9 @@ try {
-LogLevel $LogLevel `
-AdditionalModules $AdditionalModules `
-LanguageServiceNamedPipe $LanguageServicePipeName `
-LanguageServiceWriteNamedPipe $LanguageServiceWritePipeName `
-DebugServiceNamedPipe $DebugServicePipeName `
-DebugServiceWriteNamedPipe $DebugServiceWritePipeName `
-Stdio:$Stdio.IsPresent`
-BundledModulesPath $BundledModulesPath `
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent `
Expand All @@ -326,15 +338,35 @@ try {
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$resultDetails["languageServiceWritePipeName"] = Get-NamedPipePath -PipeName $LanguageServiceWritePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["languageServiceReadPipeName"]
Set-NamedPipeMode -PipeFile $resultDetails["languageServiceWritePipeName"]
}
}
else {
$resultDetails["languageServicePipeName"] = Get-NamedPipePath -PipeName $LanguageServicePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["languageServicePipeName"]
}
}
}
if ($DebugServicePipeName) {
$resultDetails["debugServicePipeName"] = Get-NamedPipePath -PipeName $DebugServicePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["debugServicePipeName"]
if ($DebugServiceWritePipeName) {
$resultDetails["debugServiceReadPipeName"] = Get-NamedPipePath -PipeName $DebugServicePipeName
$resultDetails["debugServiceWritePipeName"] = Get-NamedPipePath -PipeName $DebugServiceWritePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["debugServiceReadPipeName"]
Set-NamedPipeMode -PipeFile $resultDetails["debugServiceWritePipeName"]
}
}
else {
$resultDetails["debugServicePipeName"] = Get-NamedPipePath -PipeName $DebugServicePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["debugServicePipeName"]
}
}
}

Expand Down
14 changes: 13 additions & 1 deletion src/PowerShellEditorServices.Host/EditorServicesHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(';');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (splitIndex > 0)
{
string readPipeName = endpoint.Substring(0, splitIndex);
string writePipeName = endpoint.Substring(splitIndex + 1);
this.logger.Write(LogLevel.Verbose, $"Creating NamedPipeServerListener for ${protocol} protocol with two pipes: Read: '" + readPipeName + "'. Write: '" + writePipeName + "'");
return new NamedPipeServerListener(protocol, readPipeName, writePipeName, this.logger);
}
else
{
return new NamedPipeServerListener(protocol, endpoint, this.logger);
}
}

default:
Expand Down
14 changes: 13 additions & 1 deletion src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerChannel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,23 @@ public class NamedPipeServerChannel : ChannelBase
{
private ILogger logger;
private NamedPipeServerStream pipeServer;
private NamedPipeServerStream writePipeServer;

public NamedPipeServerChannel(
NamedPipeServerStream pipeServer,
ILogger logger)
{
this.pipeServer = pipeServer;
this.writePipeServer = null;
this.logger = logger;
}
public NamedPipeServerChannel(
NamedPipeServerStream readPipeServer,
NamedPipeServerStream writePipeServer,
ILogger logger)
{
this.pipeServer = readPipeServer;
this.writePipeServer = writePipeServer;
this.logger = logger;
}

Expand All @@ -31,7 +42,7 @@ protected override void Initialize(IMessageSerializer messageSerializer)

this.MessageWriter =
new MessageWriter(
this.pipeServer,
this.writePipeServer ?? this.pipeServer,
messageSerializer,
this.logger);
}
Expand All @@ -40,6 +51,7 @@ protected override void Shutdown()
{
// The server listener will take care of the pipe server
this.pipeServer = null;
this.writePipeServer = null;
}
}
}
Expand Down
90 changes: 65 additions & 25 deletions src/PowerShellEditorServices.Protocol/MessageProtocol/Channel/NamedPipeServerListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.PowerShell.EditorServices.Utility;
using Microsoft.Win32.SafeHandles;
using System;
using System.Collections.Generic;
using System.IO;
using System.IO.Pipes;
using System.Runtime.InteropServices;
Expand All @@ -19,7 +20,9 @@ public class NamedPipeServerListener : ServerListenerBase<NamedPipeServerChannel
{
private ILogger logger;
private string pipeName;
private readonly string writePipeName;
private NamedPipeServerStream pipeServer;
private NamedPipeServerStream writePipeServer;

public NamedPipeServerListener(
MessageProtocolType messageProtocolType,
Expand All @@ -29,6 +32,19 @@ public NamedPipeServerListener(
{
this.logger = logger;
this.pipeName = pipeName;
this.writePipeName = null;
}

public NamedPipeServerListener(
MessageProtocolType messageProtocolType,
string readPipeName,
string writePipeName,
ILogger logger)
: base(messageProtocolType)
{
this.logger = logger;
this.pipeName = readPipeName;
this.writePipeName = writePipeName;
}

public override void Start()
Expand Down Expand Up @@ -63,6 +79,10 @@ public override void Start()
// 99% of this code was borrowed from PowerShell here:
// https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs#L124-L256
this.pipeServer = NamedPipeNative.CreateNamedPipe(pipeName, pipeSecurity);
if (this.writePipeName != null)
{
this.writePipeServer = NamedPipeNative.CreateNamedPipe(writePipeName, pipeSecurity);
}
}
else
{
Expand All @@ -74,6 +94,15 @@ public override void Start()
maxNumberOfServerInstances: 1,
transmissionMode: PipeTransmissionMode.Byte,
options: PipeOptions.Asynchronous);
if (this.writePipeName != null)
{
this.writePipeServer = new NamedPipeServerStream(
pipeName: writePipeName,
direction: PipeDirection.InOut,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the direction here be just PipeDirection.In?

maxNumberOfServerInstances: 1,
transmissionMode: PipeTransmissionMode.Byte,
options: PipeOptions.None);
}
}
ListenForConnection();
}
Expand All @@ -97,39 +126,50 @@ public override void Stop()

this.logger.Write(LogLevel.Verbose, "Named pipe server has been disposed.");
}
if (this.writePipeServer != null)
{
this.logger.Write(LogLevel.Verbose, $"Named write pipe server {writePipeServer} shutting down...");

this.writePipeServer.Dispose();

this.logger.Write(LogLevel.Verbose, $"Named write pipe server {writePipeServer} has been disposed.");
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ant-druha ant-druha Oct 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (this.writePipeServer != null)
{
connectionTasks.Add(WaitForConnectionAsync(this.writePipeServer));
}

Task.Run(async () =>
{
try
{
try
{
await Task.WhenAll(connectionTasks);
this.OnClientConnect(new NamedPipeServerChannel(this.pipeServer, this.writePipeServer, this.logger));
}
catch (Exception e)
{
this.logger.WriteException(
"An unhandled exception occurred while listening for a named pipe client connection",
e);

throw e;
Copy link
Contributor

@rkeithhill rkeithhill Nov 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

}
});
}

private static async Task WaitForConnectionAsync(NamedPipeServerStream pipeServerStream)
{
#if CoreCLR
await this.pipeServer.WaitForConnectionAsync();
await pipeServerStream.WaitForConnectionAsync();
#else
await Task.Factory.FromAsync(
this.pipeServer.BeginWaitForConnection,
this.pipeServer.EndWaitForConnection, null);
await Task.Factory.FromAsync(pipeServerStream.BeginWaitForConnection, pipeServerStream.EndWaitForConnection, null);
#endif

await this.pipeServer.FlushAsync();

this.OnClientConnect(
new NamedPipeServerChannel(
this.pipeServer,
this.logger));
}
catch (Exception e)
{
this.logger.WriteException(
"An unhandled exception occurred while listening for a named pipe client connection",
e);

throw e;
}
});
await pipeServerStream.FlushAsync();
}
}

Expand Down