Skip to content
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

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

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 5 commits
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
58 changes: 45 additions & 13 deletions module/PowerShellEditorServices/PowerShellEditorServices.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,37 @@ function Start-EditorServicesHost {
[string]
$HostVersion,

[Parameter(ParameterSetName="Stdio",Mandatory=$true)]
[switch]
$Stdio,

[Parameter(ParameterSetName="NamedPipe",Mandatory=$true)]
[ValidateNotNullOrEmpty()]
[string]
$LanguageServiceNamedPipe,

[Parameter(ParameterSetName="NamedPipe")]
[string]
$DebugServiceNamedPipe,

[Parameter(ParameterSetName="NamedPipeHalfDuplex",Mandatory=$true)]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

[ValidateNotNullOrEmpty()]
[string]
$LanguageServiceInNamedPipe,

[Parameter(ParameterSetName="NamedPipeHalfDuplex",Mandatory=$true)]
[ValidateNotNullOrEmpty()]
[string]
$LanguageServiceOutNamedPipe,

[Parameter(ParameterSetName="NamedPipeHalfDuplex")]
[string]
$DebugServiceInNamedPipe,

[Parameter(ParameterSetName="NamedPipeHalfDuplex")]
[string]
$DebugServiceOutNamedPipe,

[ValidateNotNullOrEmpty()]
[string]
$BundledModulesPath,
Expand Down Expand Up @@ -99,19 +121,29 @@ function Start-EditorServicesHost {
$debugServiceConfig =
Microsoft.PowerShell.Utility\New-Object Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportConfig

if ($Stdio.IsPresent) {
$languageServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::Stdio
$debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::Stdio
}

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

if ($DebugServiceNamedPipe) {
$debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
$debugServiceConfig.Endpoint = "$DebugServiceNamedPipe"
switch ($PSCmdlet.ParameterSetName) {
"Stdio" {
$languageServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::Stdio
$debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::Stdio
}
rkeithhill marked this conversation as resolved.
Show resolved Hide resolved
"NamedPipe" {
$languageServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
$languageServiceConfig.InOutPipeName = "$LanguageServiceNamedPipe"
if ($DebugServiceNamedPipe) {
$debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
$debugServiceConfig.InOutPipeName = "$DebugServiceNamedPipe"
}
}
"NamedPipeHalfDuplex" {
$languageServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
$languageServiceConfig.InPipeName = $LanguageServiceInNamedPipe
$languageServiceConfig.OutPipeName = $LanguageServiceOutNamedPipe
if ($DebugServiceInNamedPipe -and $DebugServiceOutNamedPipe) {
$debugServiceConfig.TransportType = [Microsoft.PowerShell.EditorServices.Host.EditorServiceTransportType]::NamedPipe
$debugServiceConfig.InPipeName = $DebugServiceInNamedPipe
$debugServiceConfig.OutPipeName = $DebugServiceOutNamedPipe
}
}
}

if ($DebugServiceOnly.IsPresent) {
Expand Down
192 changes: 124 additions & 68 deletions module/PowerShellEditorServices/Start-EditorServices.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# Services GitHub repository:
#
# https://github.com/PowerShell/PowerShellEditorServices/blob/master/module/PowerShellEditorServices/Start-EditorServices.ps1

[Cmdletbinding(DefaultParameterSetName="NamedPipe")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

param(
[Parameter(Mandatory=$true)]
[ValidateNotNullOrEmpty()]
Expand Down Expand Up @@ -65,14 +65,39 @@ param(
[switch]
$ConfirmInstall,

[Parameter(ParameterSetName="Stdio",Mandatory=$true)]
[switch]
$Stdio,

[Parameter(ParameterSetName="NamedPipe")]
[string]
$LanguageServicePipeName = $null,

[Parameter(ParameterSetName="NamedPipe")]
[string]
$DebugServicePipeName = $null,

[Parameter(ParameterSetName="NamedPipeHalfDuplex",Mandatory=$true)]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

[switch]
$SplitInOutPipes,

[Parameter(ParameterSetName="NamedPipeHalfDuplexParam",Mandatory=$true)]
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

[ValidateNotNullOrEmpty()]
[string]
$DebugServicePipeName = $null
$LanguageServiceInPipeName,

[Parameter(ParameterSetName="NamedPipeHalfDuplexParam",Mandatory=$true)]
[ValidateNotNullOrEmpty()]
[string]
$LanguageServiceOutPipeName,

[Parameter(ParameterSetName="NamedPipeHalfDuplexParam")]
[string]
$DebugServiceInPipeName = $null,

[Parameter(ParameterSetName="NamedPipeHalfDuplexParam")]
[string]
$DebugServiceOutPipeName = $null
)

$DEFAULT_USER_MODE = "600"
Expand Down Expand Up @@ -173,8 +198,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())"
TylerLeonhardt marked this conversation as resolved.
Show resolved Hide resolved

if ((Test-NamedPipeName -PipeName $PipeName)) {
return $PipeName
Expand Down Expand Up @@ -243,6 +267,29 @@ function Set-NamedPipeMode {
}
}

function Test-NamedPipeName-OrCreate-IfNull {
param(
[string]
$PipeName
)
if (-not $PipeName) {
$PipeName = New-NamedPipeName
}
else {
if (-not (Test-NamedPipeName -PipeName $PipeName)) {
Copy link
Member

@TylerLeonhardt TylerLeonhardt Nov 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

ExitWithError "Pipe name supplied is already taken: $PipeName"
}
}
return $PipeName
}

function SetPipeFileResult($ResultTable, [string]$PipeNameKey, [string]$PipeNameValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$ResultTable[$PipeNameKey] = Get-NamedPipePath -PipeName $PipeNameValue
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $ResultTable[$PipeNameKey]
}
}

# Add BundledModulesPath to $env:PSModulePath
if ($BundledModulesPath) {
$env:PSModulePath = $env:PSModulePath.TrimEnd([System.IO.Path]::PathSeparator) + [System.IO.Path]::PathSeparator + $BundledModulesPath
Expand All @@ -263,81 +310,90 @@ try {

Microsoft.PowerShell.Core\Import-Module PowerShellEditorServices -ErrorAction Stop

# Locate available port numbers for services
# There could be only one service on Stdio channel

$languageServiceTransport = $null
$debugServiceTransport = $null

if ($Stdio.IsPresent) {
$languageServiceTransport = "Stdio"
$debugServiceTransport = "Stdio"
}
else {
$languageServiceTransport = "NamedPipe"
$debugServiceTransport = "NamedPipe"
if (-not $LanguageServicePipeName) {
$LanguageServicePipeName = New-NamedPipeName
}
else {
if (-not (Test-NamedPipeName -PipeName $LanguageServicePipeName)) {
ExitWithError "Pipe name supplied is already taken: $LanguageServicePipeName"
}
}
if (-not $DebugServicePipeName) {
$DebugServicePipeName = New-NamedPipeName
}
else {
if (-not (Test-NamedPipeName -PipeName $DebugServicePipeName)) {
ExitWithError "Pipe name supplied is already taken: $DebugServicePipeName"
}
}
}

if ($EnableConsoleRepl) {
Write-Host "PowerShell Integrated Console`n"
}

# Create the Editor Services host
Log "Invoking Start-EditorServicesHost"
$editorServicesHost =
Start-EditorServicesHost `
-HostName $HostName `
-HostProfileId $HostProfileId `
-HostVersion $HostVersion `
-LogPath $LogPath `
-LogLevel $LogLevel `
-AdditionalModules $AdditionalModules `
-LanguageServiceNamedPipe $LanguageServicePipeName `
-DebugServiceNamedPipe $DebugServicePipeName `
-Stdio:$Stdio.IsPresent`
-BundledModulesPath $BundledModulesPath `
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent `
-DebugServiceOnly:$DebugServiceOnly.IsPresent `
-WaitForDebugger:$WaitForDebugger.IsPresent

# TODO: Verify that the service is started
Log "Start-EditorServicesHost returned $editorServicesHost"

$resultDetails = @{
"status" = "started";
"languageServiceTransport" = $languageServiceTransport;
"debugServiceTransport" = $debugServiceTransport;
"status" = "not started";
"languageServiceTransport" = $PSCmdlet.ParameterSetName.Replace("Param","");
"debugServiceTransport" = $PSCmdlet.ParameterSetName.Replace("Param","");
};

if ($LanguageServicePipeName) {
$resultDetails["languageServicePipeName"] = Get-NamedPipePath -PipeName $LanguageServicePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["languageServicePipeName"]
# Create the Editor Services host
Log "Invoking Start-EditorServicesHost"
# There could be only one service on Stdio channel
# Locate available port numbers for services
switch -Wildcard ($PSCmdlet.ParameterSetName) {
"Stdio" {
$editorServicesHost = Start-EditorServicesHost `
-HostName $HostName `
-HostProfileId $HostProfileId `
-HostVersion $HostVersion `
-LogPath $LogPath `
-LogLevel $LogLevel `
-AdditionalModules $AdditionalModules `
-Stdio `
-BundledModulesPath $BundledModulesPath `
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent `
-DebugServiceOnly:$DebugServiceOnly.IsPresent `
-WaitForDebugger:$WaitForDebugger.IsPresent
}
}
if ($DebugServicePipeName) {
$resultDetails["debugServicePipeName"] = Get-NamedPipePath -PipeName $DebugServicePipeName
if ($IsLinux -or $IsMacOS) {
Set-NamedPipeMode -PipeFile $resultDetails["debugServicePipeName"]
"NamedPipeHalfDuplex*" {
$LanguageServiceInPipeName = Test-NamedPipeName-OrCreate-IfNull $LanguageServiceInPipeName
$LanguageServiceOutPipeName = Test-NamedPipeName-OrCreate-IfNull $LanguageServiceOutPipeName
$DebugServiceInPipeName = Test-NamedPipeName-OrCreate-IfNull $DebugServiceInPipeName
$DebugServiceOutPipeName = Test-NamedPipeName-OrCreate-IfNull $DebugServiceOutPipeName

$editorServicesHost = Start-EditorServicesHost `
-HostName $HostName `
-HostProfileId $HostProfileId `
-HostVersion $HostVersion `
-LogPath $LogPath `
-LogLevel $LogLevel `
-AdditionalModules $AdditionalModules `
-LanguageServiceInNamedPipe $LanguageServiceInPipeName `
-LanguageServiceOutNamedPipe $LanguageServiceOutPipeName `
-DebugServiceInNamedPipe $DebugServiceInPipeName `
-DebugServiceOutNamedPipe $DebugServiceOutPipeName `
-BundledModulesPath $BundledModulesPath `
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent `
-DebugServiceOnly:$DebugServiceOnly.IsPresent `
-WaitForDebugger:$WaitForDebugger.IsPresent

SetPipeFileResult $resultDetails "languageServiceReadPipeName" $LanguageServiceInPipeName
SetPipeFileResult $resultDetails "languageServiceWritePipeName" $LanguageServiceOutPipeName
SetPipeFileResult $resultDetails "debugServiceReadPipeName" $DebugServiceInPipeName
SetPipeFileResult $resultDetails "debugServiceWritePipeName" $DebugServiceOutPipeName
}
Default {
$LanguageServicePipeName = Test-NamedPipeName-OrCreate-IfNull $LanguageServicePipeName
$DebugServicePipeName = Test-NamedPipeName-OrCreate-IfNull $DebugServicePipeName

$editorServicesHost = Start-EditorServicesHost `
-HostName $HostName `
-HostProfileId $HostProfileId `
-HostVersion $HostVersion `
-LogPath $LogPath `
-LogLevel $LogLevel `
-AdditionalModules $AdditionalModules `
-LanguageServiceNamedPipe $LanguageServicePipeName `
-DebugServiceNamedPipe $DebugServicePipeName `
-BundledModulesPath $BundledModulesPath `
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent `
-DebugServiceOnly:$DebugServiceOnly.IsPresent `
-WaitForDebugger:$WaitForDebugger.IsPresent

SetPipeFileResult $resultDetails "languageServicePipeName" $LanguageServicePipeName
SetPipeFileResult $resultDetails "debugServicePipeName" $DebugServicePipeName
}
}

# TODO: Verify that the service is started
Log "Start-EditorServicesHost returned $editorServicesHost"

$resultDetails["status"] = "started"

# Notify the client that the services have started
WriteSessionFile $resultDetails

Expand Down
15 changes: 13 additions & 2 deletions src/PowerShellEditorServices.Host/EditorServicesHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ public class EditorServiceTransportConfig
/// For Stdio it's ignored.
/// For NamedPipe it's the pipe name.
/// </summary>
public string Endpoint { get; set; }
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}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}";

}

/// <summary>
Expand Down Expand Up @@ -463,7 +466,15 @@ private IServerListener CreateServiceListener(MessageProtocolType protocol, Edit

case EditorServiceTransportType.NamedPipe:
{
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 + "'");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}'");

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'

return new NamedPipeServerListener(protocol, config.InPipeName, config.OutPipeName, this.logger);
}
else
{
return new NamedPipeServerListener(protocol, config.InOutPipeName, this.logger);
}
}

default:
Expand Down
Loading