Switch to named pipes #1327
Switch to named pipes #1327
Conversation
src/session.ts
Outdated
|
|
||
| if (this.sessionSettings.developer.editorServicesWaitForDebugger) { | ||
| this.editorServicesArgs += "-WaitForDebugger "; | ||
| this.editorServicesArgs += `-WaitForDebugger `; |
There was a problem hiding this comment.
No need for string interpolation here.
src/session.ts
Outdated
| } | ||
| if (this.sessionSettings.developer.editorServicesLogLevel) { | ||
| this.editorServicesArgs += "-LogLevel '" + this.sessionSettings.developer.editorServicesLogLevel + "' "; | ||
| this.editorServicesArgs += `-LogLevel '${this.sessionSettings.developer.editorServicesLogLevel}' `; |
There was a problem hiding this comment.
why don't we need it here?
There was a problem hiding this comment.
If your question is about the string interpolation, then I'm for its use here, where there is a variable to interpolate inside the string. :-)
|
Connecting logic all seems right but there's a crash that occurs when a script finishes debugging (staring and walking through breakpoints and such all work fine):
I'll have to dig in to how the debug adapter handles ending of debugging. |
|
Ok that crash is fixed in PowerShell/PowerShellEditorServices#668 I'm removing the WIP. I've tested named pipes on:
|
| "-AdditionalModules @('PowerShellEditorServices.VSCode') " + | ||
| "-BundledModulesPath '" + this.bundledModulesPath + "' " + | ||
| "-EnableConsoleRepl "; | ||
| `-HostName 'Visual Studio Code Host' ` + |
There was a problem hiding this comment.
Thoughts on using ordinary strings where no interpolation is needed vs consistency? Not opinionated either way.
There was a problem hiding this comment.
for here, I just kept it consistent with the rest of the string concat
| ".dotnet", "corefx", "pipe", | ||
| pipeName); | ||
| // Windows uses NamedPipes where non-Windows platforms use Unix Domain Sockets. | ||
| // This requires connecting to the pipe file in different locations on Windows vs non-Windows. |
There was a problem hiding this comment.
Technically the name difference is just .NET Core being strange, rather than the use of domain sockets, no?
There was a problem hiding this comment.
nah the location is different too. Named Pipes are in:
\\.\pipe\foo
domain sockets are in:
/tmp/CoreFxPipe_foo
Even the Named Pipes on Windows using .NET Standard instead are placed here:
\\.\pipe\foo
PR Summary
switch to using named pipes on Windows (and Unix domain sockets) instead of TCP.
It's shockingly great how simple this change was :)
marking WIP because I want to test on a few platforms and want a CI build.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready