Isolate PSES dependencies from PowerShell on load + make PSES a pure binary module #1118
Conversation
rjmholt
requested review from
TylerLeonhardt,
SeeminglyScience and
daxian-dbw
| /// <summary> | ||
| /// Configuration for editor services startup. | ||
| /// </summary> | ||
| public class EditorServicesConfig |
There was a problem hiding this comment.
Consider:
| public class EditorServicesConfig | |
| public sealed class EditorServicesConfig |
| /// <summary> | ||
| /// Configuration for Editor Services' PowerShell profile paths. | ||
| /// </summary> | ||
| public class ProfilePathConfig |
There was a problem hiding this comment.
Consider:
| public class ProfilePathConfig | |
| public sealed class ProfilePathConfig |
| using System.Reflection; | ||
| using System.Threading.Tasks; | ||
| using System.Collections.Generic; | ||
| using SMA = System.Management.Automation; |
There was a problem hiding this comment.
I typically like to have aliases in a separate group separated by line. Might just be me though, totally optional.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="NETStandard.Library" Version="2.0.3" /> |
There was a problem hiding this comment.
What's this do when targeting specific frameworks?
There was a problem hiding this comment.
So this should mean we get netstandard.dll for net461 (and nothing in .NET Core). Basically it's the binding trampoline from netstandard to the concrete runtime.
One thing I've noticed though is that in Core it will keep trying to find netstandard.dll. That hasn't lead to any crashes, but I wonder if it signifies that we're doing something wrong. As far as I know, .NET Core 2.0+ has netstandard.dll baked in, so there's no physical DLL to find.
src/PowerShellEditorServices.Hosting/StartEditorServicesCommand.cs
Outdated
Show resolved
Hide resolved
| protected override void BeginProcessing() | ||
| { | ||
| #if DEBUG | ||
| if (WaitForDebugger) |
There was a problem hiding this comment.
Oh? Neat! I had no idea C# would account for implicit bool conversions in an if statement. Makes me wonder what the point of op_True is (or rather helps me realize why no one uses it 🙂).
src/PowerShellEditorServices.Hosting/StartEditorServicesCommand.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: Tyler James Leonhardt <[email protected]>
…nner.cs Co-Authored-By: Tyler James Leonhardt <[email protected]>
|
Issues
======
+ Solved 6
- Added 19
Complexity increasing per file
==============================
- src/PowerShellEditorServices.Hosting/Internal/NamedPipeUtils.cs 7
- src/PowerShellEditorServices.Hosting/EditorServicesLoader.cs 7
- src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs 2
- src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs 6
- src/PowerShellEditorServices/Hosting/HostStartupInfo.cs 6
- src/PowerShellEditorServices/Server/PsesDebugServer.cs 1
- src/PowerShellEditorServices/Logging/HostLoggerAdapter.cs 1
- src/PowerShellEditorServices.Hosting/Configuration/HostLogger.cs 6
- src/PowerShellEditorServices.Hosting/Configuration/EditorServicesConfig.cs 1
- src/PowerShellEditorServices.Hosting/Configuration/HostInfo.cs 1
- src/PowerShellEditorServices.Hosting/Configuration/TransportConfig.cs 2
- src/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs 7
- src/PowerShellEditorServices/Hosting/EditorServicesLoading.cs 1
- src/PowerShellEditorServices.Hosting/Configuration/SessionFileWriter.cs 7
- src/PowerShellEditorServices.Hosting/Internal/PsesLoadContext.cs 3
See the complete overview on Codacy |
| /// <summary> | ||
| /// Configuration for Editor Services' PowerShell profile paths. | ||
| /// </summary> | ||
| public struct ProfilePathConfig |
There was a problem hiding this comment.
| .Wait(); | ||
| if (!s_hasRunPsrlStaticCtor && _usePSReadLine) | ||
| { | ||
| s_hasRunPsrlStaticCtor = true; |
There was a problem hiding this comment.
| LogOperatingSystemDetails(); | ||
| } | ||
|
|
||
| private string GetPSOutputEncoding() |
There was a problem hiding this comment.
| debugServer.StartAsync(); | ||
| } | ||
|
|
||
| private Task RestartDebugServerAsync(PsesDebugServer debugServer, bool usePSReadLine) |
There was a problem hiding this comment.
| } | ||
|
|
||
| // Unlike in .NET Core, we need to be look for all dependencies in .NET Framework, not just PSES.dll | ||
| AppDomain.CurrentDomain.AssemblyResolve += (object sender, ResolveEventArgs args) => |
There was a problem hiding this comment.
| /// <summary> | ||
| /// Configuration for the debug adapter protocol transport to use. | ||
| /// </summary> | ||
| public ITransportConfig DebugServiceTransport { get; set; } = null; |
There was a problem hiding this comment.
| /// <summary> | ||
| /// Configuration for the language server protocol transport to use. | ||
| /// </summary> | ||
| public ITransportConfig LanguageServiceTransport { get; set; } = null; |
There was a problem hiding this comment.
|
|
||
| public string SessionFileTransportName => "Stdio"; | ||
|
|
||
| public IReadOnlyDictionary<string, object> SessionFileEntries { get; } = null; |
There was a problem hiding this comment.
|
|
||
| if (hostConfig.LogLevel == PsesLogLevel.Diagnostic) | ||
| { | ||
| AppDomain.CurrentDomain.AssemblyLoad += (object sender, AssemblyLoadEventArgs args) => |
There was a problem hiding this comment.
| public void OnNext((PsesLogLevel logLevel, string message) value) | ||
| { | ||
| string message = null; | ||
| switch (value.logLevel) |
There was a problem hiding this comment.
|
|
||
| return this.ExecuteCommandAsync<PSObject>(importCommand, false, false); | ||
| return this.ExecuteCommandAsync<PSObject>(importCommand, sendOutputToHost: false, sendErrorToHost: false); |
There was a problem hiding this comment.
| case PsesLogLevel.Error: | ||
| message = $"[ERR]: {value.message}"; | ||
| break; | ||
| }; |
There was a problem hiding this comment.
| _unsubscriber.Dispose(); | ||
| _fileWriter.Flush(); | ||
| _fileWriter.Close(); | ||
| _fileWriter.Dispose(); |
There was a problem hiding this comment.
| /// <summary> | ||
| /// Names of or paths to any additional modules to load on startup. | ||
| /// </summary> | ||
| public IReadOnlyList<string> AdditionalModules { get; set; } = null; |
There was a problem hiding this comment.
| "); | ||
| } | ||
|
|
||
| private string GetOSArchitecture() |
There was a problem hiding this comment.
| } | ||
| } | ||
| catch (TaskCanceledException) | ||
| { |
There was a problem hiding this comment.
| // This is not high priority, since the PSES process shouldn't be reused | ||
| } | ||
|
|
||
| private void LoadEditorServices() |
There was a problem hiding this comment.
| { | ||
| protected readonly ILoggerFactory _loggerFactory; | ||
| private static bool s_hasRunPsrlStaticCtor = false; |
There was a problem hiding this comment.
| /// <summary> | ||
| /// Flags of features to enable on startup. | ||
| /// </summary> | ||
| public IReadOnlyList<string> FeatureFlags { get; set; } = null; |
There was a problem hiding this comment.
|
@SeeminglyScience do you wanna rereview? |
Fixes PowerShell/vscode-powershell#2292.
Summary
Start-EditorServicescommand with the same parameters. Ideally we can change the parameters in the future to simplify them and have theStart-EditorServices.ps1script to provide an adapter for compatibilitiy.Write-Host, we now use logging on startup:ILogger. We can't pull this type into the host assembly because it's one of the dependencies we're trying to keep out of the default assembly load context, but we do a handover where the ILogger gets a dump of all the logs so far from the hosting assembly and subscribes for further logs. I've also moved the PSES logging sink from the file one to the async file one.Start-EditorSevicescommand tries to do as little as possible in its direct implementation. Instead I've broken the logic up into various classes and it looks like this:Start-EditorServicescreates anEditorServicesLoader, which is designed to be hostable if that's ever wanted. This has a simplified configuration API designed to be simpler and harder to misconfigure than the startup script.EditorServicesLoaderregisters assembly resolve events so that when dependencies aren't found (since they've been moved into a different location), we intercept the failure and load them from the right place, transparently so no reflection is required.Assembly.LoadFrom()to load PSES and its dependencies from the right directory.EditorServicesRunnerand use it to run PSES. This class exists so that we only have one type from the PSES DLL expressed in the loader so we don't accidentally break the load semantics by moving statements around.HostStartupInfo, which is basically a big object for passing all the startup information needed for PSES through the eye of the needle and into PSES. Consolidating more things into this class has simplified a number of our APIs where we were just passing lots of things all the way down to PowerShellContextService#ifs in the code are in the loader for assembly resolve event subscription and in the named pipe construction logic -- I tried to minimise their usageEditorServicesHostis essentially replaced withEditorServicesFactory, which keeps concepts like logging within the PSES DLL. This means we have to duplicate log level types, but otherwise I think is fairly clean.