Isolate PSES dependencies from PowerShell on load + make PSES a pure binary module by rjmholt · Pull Request #1118 · PowerShell/PowerShellEditorServices · GitHub

Fixes PowerShell/vscode-powershell#2292.

Summary

Moves away from the startup script to a binary module that exports a Start-EditorServices command with the same parameters. Ideally we can change the parameters in the future to simplify them and have the Start-EditorServices.ps1 script to provide an adapter for compatibilitiy.
Instead of transcription and Write-Host, we now use logging on startup:
- A threaded file logger that uses a concurrent queue to asynchronously log output to a file, in case we never get to the PSES startup point
- A host logger to print log output to the console. This is disabled when stdio is used. This uses the host writer APIs with the assumption that they can be used concurrently. So far this is true in the console. Just before the LSP/debug servers are started this unsubscribes from logging to not pollute the console. On shutdown, it resubscribes to show information about shutdown.
- Inside PSES, there's an adapter for the 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.
The Start-EditorSevices command 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-EditorServices creates an EditorServicesLoader, 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.
- When it's created, the EditorServicesLoader registers 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.
- In .NET Core we load the PSES DLL in a new assembly load context and expose it in the default load context. The PSES DLL's dependencies are loaded automatically by that new load context and are not exposed in the default load context. We have to do a small amount of work ensuring we take shared dependencies from $PSHOME and looking for PSES dependencies in the right place, but it's a very simple implementation.
- In .NET Framework, when the assembly resolve event is fired, we use Assembly.LoadFrom() to load PSES and its dependencies from the right directory.
- In both frameworks we subscribe to the AppDomain assembly load event so we can log what assemblies are being loaded
- Within the loader we call a static method from the PSES DLL with no implementation which triggers loading. This just exists to put a nice point in the code where we can say "this is where we load PSES"
- From this point we instantiate the EditorServicesRunner and 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.
- The runner assumes PSES has already loaded and it can do what it likes. It takes the configuration and turns it into the now consolidate 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
- The runner then asynchronously starts up LSP and debug servers, sets up the debug server restart event, and waits for the LSP server to shut down
The session file writer is now all in C#, but uses the hosting PowerShell to run the JSON conversion so we don't need to explicitly load Newtonsoft.Json.dll, meaning PSSA is now the constraint on using a different version of Newtonsoft.Json in Windows PowerShell. In later work, we might be able to pull that into isolation so we no longer force a Newtonsoft.Json version match
The build is changed so we no longer cherry pick DLLs to include in the module. Since we're changing the layout of the module for dependency isolation, I also made the changes to ensure we work as a netstandard2.0 library on net461, net462 and net47, where we include netstandard.dll and all the shim DLLs. Rather than writing each one in, we just copy all the DLLs produced by the net461 or netcoreapp2.1 target. This means that future dependency additions to csproj files should generally "just work".
Since we now target concrete frameworks in the hosting assembly, I was able to get rid of the reflection for named pipe construction and we now do it all in easy-to-check code. Because we now create streams in the host (which is not as clean as I'd like, but I chose to get rid of reflection rather than layer quite as nicely), we just pass the streams into server constructors. This meant I was able to get rid of LSP server subclasses that specialise for the transport.
The only serious #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 usage
To keep dependencies isolated nicely, EditorServicesHost is essentially replaced with EditorServicesFactory, 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.
The profile path class we used to have tried to be a bit clever. I've changed it so the host is responsible for all the profile paths and we just pass them in. This might be the wrong idea, but for now it's not very exposed.

Very exciting changes, glad to see this finally happening 🙂

Still going over it, but here's some initial feedback.

Consider:

Suggested change

      
                public class EditorServicesConfig
          
                public sealed class EditorServicesConfig

Consider:

Suggested change

      
                public class ProfilePathConfig
          
                public sealed class ProfilePathConfig

I typically like to have aliases in a separate group separated by line. Might just be me though, totally optional.

What's this do when targeting specific frameworks?

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.

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 🙂).

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

…nner.cs Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

Here is an overview of what got changed by this pull request:

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

Issue found: Implement 'IEquatable' in value type 'ProfilePathConfig'.

Issue found: Make the enclosing instance method 'static' or remove this set on the 'static' field.

Issue found: Make 'GetPSOutputEncoding' a static method.

Issue found: Change return type to 'void'; not a single caller uses the returned value.

Issue found: Remove the type specification; it is redundant.

Issue found: Remove this initialization to 'DebugServiceTransport', the compiler will do that for you.

Issue found: Remove this initialization to 'LanguageServiceTransport', the compiler will do that for you.

Issue found: Remove this initialization to 'SessionFileEntries', the compiler will do that for you.

Issue found: Remove the type specification; it is redundant.

Issue found: Add a 'default' clause to this 'switch' statement.

Issue found: Remove this default value assigned to parameter 'sendOutputToHost'.

Issue found: Remove this empty statement.

Issue found: Move this 'Dispose' call into this class' own 'Dispose' method.

Issue found: Remove this initialization to 'AdditionalModules', the compiler will do that for you.

Issue found: Make 'GetOSArchitecture' a static method.

Issue found: Either remove or fill this block of code.

Issue found: Make 'LoadEditorServices' a static method.

Issue found: Remove this initialization to 's_hasRunPsrlStaticCtor', the compiler will do that for you.

Issue found: Remove this initialization to 'FeatureFlags', the compiler will do that for you.

Patrick Meinecke (@SeeminglyScience) do you wanna rereview?

LGTM

Rob Holt (rjmholt) requested review from Patrick Meinecke (SeeminglyScience), TylerLeonhardt and Dongbo Wang (daxian-dbw) November 27, 2019 21:30

Patrick Meinecke (SeeminglyScience) reviewed Dec 2, 2019

View reviewed changes

Rob Holt and others added 26 commits December 2, 2019 15:28

First work

55485e7

More progress

57ce346

Enable compilation

ce7f50f

Build increments

9814824

Add initial load context

a2ab488

Fix loading issues

1be54f2

Write session file

23abd87

Fix startup connection issue

e50e95c

Add logging

884a742

Add doc comments + some cleanup

3ab36d5

Plumb host logging into PSES

dbd3081

Fix compile issues

8fb83f9

First part of host logger handover

7f04198

Complete host logging deregistration

0580aea

Add more logging on startup

b073e53

Migrate startup script

b2eebcc

Fix Windows PowerShell startup

fd6d25e

Add event handler logging

14fc131

Add warning for low .NET versions

accfa52

Fix VSCode and Command module imports

1cebbb6

Add constrained language mode check

640f97b

Add copyright headers

9bd8789

Remove extraneous host files

c18b5e7

Warn user about read key on crash

72da273

Add configuration validation

97bbae7

Add more logging

d744818

TylerLeonhardt reviewed Dec 9, 2019

View reviewed changes

Comment thread PowerShellEditorServices.build.ps1

Comment thread PowerShellEditorServices.build.ps1

Comment thread src/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs Outdated

Rob Holt (rjmholt) and others added 4 commits December 9, 2019 13:46

Update PowerShellEditorServices.build.ps1

69b9583

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

Update src/PowerShellEditorServices.Hosting/Internal/EditorServicesRu…

7951fb3

…nner.cs Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

Skip adding SMA asms to module

dde7ea7

Fix codacy bits

ddb97af

TylerLeonhardt reviewed Dec 9, 2019

View reviewed changes

TylerLeonhardt approved these changes Dec 9, 2019

View reviewed changes

Rob Holt (rjmholt) merged commit 65a7a79 into PowerShell:master Dec 10, 2019

Rob Holt (rjmholt) deleted the dependency-isolation branch December 11, 2019 21:41

Andy Jordan (andyleejordan) added the Area-Startup label May 12, 2021

+                  /// <summary>
+                  /// Configuration for editor services startup.
+                  /// </summary>
+                  public class EditorServicesConfig

+                  /// <summary>
+                  /// Configuration for Editor Services' PowerShell profile paths.
+                  /// </summary>
+                  public class ProfilePathConfig

+              using System.Reflection;
+              using System.Threading.Tasks;
+              using System.Collections.Generic;
+              using SMA = System.Management.Automation;

+                  /// <summary>
+                  /// Configuration for Editor Services' PowerShell profile paths.
+                  /// </summary>
+                  public struct ProfilePathConfig

+                      /// <summary>
+                      /// Configuration for the debug adapter protocol transport to use.
+                      /// </summary>
+                      public ITransportConfig DebugServiceTransport { get; set; } = null;

	public class EditorServicesConfig
	public sealed class EditorServicesConfig

	public class ProfilePathConfig
	public sealed class ProfilePathConfig

+                </PropertyGroup>
+                <ItemGroup>
+                  <PackageReference Include="NETStandard.Library" Version="2.0.3" />

+                      protected override void BeginProcessing()
+                      {
+              #if DEBUG
+                          if (WaitForDebugger)

-                                  .Wait();
+                              if (!s_hasRunPsrlStaticCtor && _usePSReadLine)
+                              {
+                                  s_hasRunPsrlStaticCtor = true;

+                          LogOperatingSystemDetails();
+                      }
+                      private string GetPSOutputEncoding()

+                          debugServer.StartAsync();
+                      }
+                      private Task RestartDebugServerAsync(PsesDebugServer debugServer, bool usePSReadLine)

+                          }
+                          // 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) =>


		public string SessionFileTransportName => "Stdio";

		public IReadOnlyDictionary<string, object> SessionFileEntries { get; } = null;

+                          if (hostConfig.LogLevel == PsesLogLevel.Diagnostic)
+                          {
+                              AppDomain.CurrentDomain.AssemblyLoad += (object sender, AssemblyLoadEventArgs args) =>

+                      public void OnNext((PsesLogLevel logLevel, string message) value)
+                      {
+                          string message = null;
+                          switch (value.logLevel)

+                              .AddArgument(s_commandsModulePath);
-                          return this.ExecuteCommandAsync<PSObject>(importCommand, false, false);
+                          return this.ExecuteCommandAsync<PSObject>(importCommand, sendOutputToHost: false, sendErrorToHost: false);

+                              case PsesLogLevel.Error:
+                                  message = $"[ERR]: {value.message}";
+                                  break;
+                          };

+                          _unsubscriber.Dispose();
+                          _fileWriter.Flush();
+                          _fileWriter.Close();
+                          _fileWriter.Dispose();

+              ");
+                      }
+                      private string GetOSArchitecture()

+                              }
+                          }
+                          catch (TaskCanceledException)
+                          {

+                          //       This is not high priority, since the PSES process shouldn't be reused
+                      }
+                      private void LoadEditorServices()

+                  internal class PsesDebugServer : IDisposable
                   {
-                      protected readonly ILoggerFactory _loggerFactory;
+                      private static bool s_hasRunPsrlStaticCtor = false;

Isolate PSES dependencies from PowerShell on load + make PSES a pure binary module by rjmholt · Pull Request #1118 · PowerShell/PowerShellEditorServices · GitHub

Uh oh!

Isolate PSES dependencies from PowerShell on load + make PSES a pure binary module - #1118

Conversation

Rob Holt (rjmholt) commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Patrick Meinecke (SeeminglyScience) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TylerLeonhardt commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Dec 9, 2019

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Rob Holt (rjmholt) commented Nov 27, 2019 •
edited
Loading