Skip to content

WIP Move to runspace synchronizer from completions #980

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.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1453,6 +1453,9 @@ protected Task HandleEvaluateRequestAsync(
executeTask.ContinueWith(
(task) =>
{
// This is the equivalent of hitting ENTER in the Integrated Console
// so we need to activate the RunspaceSynchronizer for completions.
RunspaceSynchronizer.Activate();
// Return an empty result since the result value is irrelevant
// for this request in the LanguageServer
return
Expand Down
33 changes: 31 additions & 2 deletions src/PowerShellEditorServices/Language/AstOperations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,10 @@
using System.Threading;
using System.Threading.Tasks;
using System.Management.Automation.Language;
using System.Management.Automation.Runspaces;

namespace Microsoft.PowerShell.EditorServices
{
using System.Management.Automation;
using System.Management.Automation.Language;

/// <summary>
/// Provides common operations for the syntax tree of a parsed script.
Expand All @@ -32,6 +30,8 @@ internal static class AstOperations

private static readonly SemaphoreSlim s_completionHandle = AsyncUtils.CreateSimpleLockingSemaphore();

private static PowerShell pwsh = PowerShell.Create();

/// <summary>
/// Gets completions for the symbol found in the Ast at
/// the given file offset.
Expand Down Expand Up @@ -69,6 +69,12 @@ static public async Task<CommandCompletion> GetCompletionsAsync(
return null;
}

if (!RunspaceSynchronizer.IsReadyForEvents)
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe this should go somewhere else...

{
pwsh.Runspace.Name = "RunspaceSynchronizerTargetRunspace";
RunspaceSynchronizer.InitializeRunspaces(powerShellContext.CurrentRunspace.Runspace, pwsh.Runspace);
}

try
{
IScriptPosition cursorPosition = (IScriptPosition)s_extentCloneWithNewOffset.Invoke(
Expand All @@ -90,6 +96,29 @@ static public async Task<CommandCompletion> GetCompletionsAsync(

var stopwatch = new Stopwatch();

// Static class members in Windows PowerShell had a thread synchronization issue.
// This issue was fixed in PowerShell 6+ so we only use the new completions if PSReadLine is enabled
// and we're running in .NET Core.
if (powerShellContext.IsPSReadLineEnabled && Utils.IsNetCore)
{
stopwatch.Start();

try
{
return CommandCompletion.CompleteInput(
scriptAst,
currentTokens,
cursorPosition,
options: null,
powershell: pwsh);
}
finally
{
stopwatch.Stop();
logger.Write(LogLevel.Verbose, $"IntelliSense completed in {stopwatch.ElapsedMilliseconds}ms.");
}
}

// If the current runspace is out of process we can use
// CommandCompletion.CompleteInput because PSReadLine won't be taking up the
// main runspace.
Expand Down
282 changes: 282 additions & 0 deletions src/PowerShellEditorServices/Language/RunspaceSynchronizer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
//
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
//

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Management.Automation.Runspaces;
using System.Reflection;
using Microsoft.PowerShell.Commands;

namespace Microsoft.PowerShell.EditorServices
{
using System.Management.Automation;

/// <summary>
/// This class is used to sync the state of one runspace to another.
/// It's done by copying over variables and reimporting modules into the target runspace.
/// It doesn't rely on the pipeline of the source runspace at all, instead leverages Reflection
/// to access internal properties and methods on the Runspace type.
/// Lastly, in order to trigger the synchronizing, you must call the Activate method. This will go
/// in the PSReadLine key handler for ENTER.
/// </summary>
public class RunspaceSynchronizer
{
private static readonly Version versionZero = new Version(0, 0);
// Determines whether the HandleRunspaceStateChange event should attempt to sync the runspaces.
private static bool SourceActionEnabled = false;

// 'moduleCache' keeps track of all modules imported in the source Runspace.
// when there is a `Import-Module -Force`, the new module object would be a
// different instance with different hashcode, so we can tell if there is a
// force loading of an already loaded module.
private static HashSet<PSModuleInfo> moduleCache = new HashSet<PSModuleInfo>();

// 'variableCache' keeps all global scope variable names and their value type.
// As long as the value type doesn't change, we don't need to update the variable
// in the target Runspace, because all tab completion needs is the type information.
private static Dictionary<string, object> variableCache = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);

private static Runspace sourceRunspace;
private static Runspace targetRunspace;
private static EngineIntrinsics sourceEngineIntrinsics;
private static EngineIntrinsics targetEngineIntrinsics;

private readonly static HashSet<string> POWERSHELL_MAGIC_VARIABLES = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
{
"PID",
"PSVersionTable",
"PSEdition",
"PSHOME",
"HOST",
"true",
"false",
"null",
"Error",
"IsMacOS",
"IsLinux",
"IsWindows"
};

/// <summary>
/// Determines if the RunspaceSynchronizer has been initialized.
/// </summary>
public static bool IsReadyForEvents { get; private set; }

#region Public methods

/// <summary>
/// Does a thing
/// </summary>
public static void InitializeRunspaces(Runspace runspaceSource, Runspace runspaceTarget)
{
sourceRunspace = runspaceSource;
sourceEngineIntrinsics = sourceRunspace.GetEngineIntrinsics();
targetRunspace = runspaceTarget;
targetEngineIntrinsics = runspaceTarget.GetEngineIntrinsics();
IsReadyForEvents = true;

sourceEngineIntrinsics.Events.SubscribeEvent(
source: null,
eventName: null,
sourceIdentifier: PSEngineEvent.OnIdle.ToString(),
data: null,
handlerDelegate: HandleRunspaceStateChange,
supportEvent: true,
forwardEvent: false);

Activate();
// Trigger events
HandleRunspaceStateChange(sender: null, args: null);
}

/// <summary>
/// Does a thing
/// </summary>
public static void Activate()
{
SourceActionEnabled = true;
}

#endregion

#region Private Methods

private static void HandleRunspaceStateChange(object sender, PSEventArgs args)
{
if (!SourceActionEnabled || sourceRunspace.Debugger.IsActive)
{
return;
}

SourceActionEnabled = false;

var newOrChangedModules = new List<PSModuleInfo>();
List<PSModuleInfo> modules = sourceRunspace.GetModules();
foreach (PSModuleInfo module in modules)
{
if (moduleCache.Add(module))
{
newOrChangedModules.Add(module);
}
}


var newOrChangedVars = new List<PSVariable>();

var variables = sourceEngineIntrinsics.GetVariables();
foreach (var variable in variables)
{
// If the variable is a magic variable or it's type has not changed, then skip it.
if(POWERSHELL_MAGIC_VARIABLES.Contains(variable.Name) ||
(variableCache.TryGetValue(variable.Name, out object value) && value == variable.Value))
{
continue;
}

// Add the variable to the cache and mark it as a newOrChanged variable.
variableCache[variable.Name] = variable.Value;
newOrChangedVars.Add(variable);
}

if (newOrChangedModules.Count > 0)
{
// Import the modules in the targetRunspace with -Force
using (PowerShell pwsh = PowerShell.Create())
{
pwsh.Runspace = targetRunspace;

foreach (PSModuleInfo moduleInfo in newOrChangedModules)
{
if(moduleInfo.Path != null)
{
string nameParameterValue = moduleInfo.Path;
// If the version is greater than zero, the module info was probably imported by the psd1 or module base.
// If so, we can just import from the module base which is the root of the module folder.
if (moduleInfo.Version > versionZero)
{
nameParameterValue = moduleInfo.ModuleBase;
}

pwsh.AddCommand("Import-Module")
.AddParameter("Name", nameParameterValue)
.AddParameter("Force")
.AddStatement();
Copy link
Collaborator

@SeeminglyScience SeeminglyScience Jun 21, 2019

Choose a reason for hiding this comment

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

Wouldn't this make the psm1 run twice?

Also in Windows PowerShell, static class methods have runspace affinity to the last runspace that processed the type definition. This second import will change the runspace that static class methods run in, even when invoked in the origin runspace.

Copy link
Member

Choose a reason for hiding this comment

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

This affinity issue has been fixed in PowerShell Core. See PowerShell/PowerShell#4209

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I did qualify it, I should have been more clear though. That said, afaik there aren't plans to drop support for it anytime soon right?

Copy link
Member

@daxian-dbw daxian-dbw Jun 24, 2019

Choose a reason for hiding this comment

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

Right, there is no plan to port the fix back to Windows PowerShell.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I'm getting at is PSES still has to support Windows PowerShell. So it seems to me that the choices are:

  1. Break static methods in Windows PowerShell
  2. Maintain both this implementation and the current system
  3. Stop supporting Windows PowerShell

Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Or, 4. Only apply this optimization to PowerShell Core. On Windows PowerShell, keep calling into the integrated console for intellisense operations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify, that's what I meant by item 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm currently now doing 2. If you're using PowerShell Core, you will get this optimization.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will only import the psm1, so it will miss nested/required modules, required assemblies, and any command filtering. We also can't actually determine if the module info object was imported via the psm1 directly or via the psd1, which could yield significantly different results.

Copy link
Member

Choose a reason for hiding this comment

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

This will only import the psm1

Can you elaborate on this? When a module is imported through a manifest, the ModuleInfo.Path points to the psd1 file. For example,

PS:34> $s = gmo Microsoft.PowerShell.Management
PS:35> $s.Path
F:\pscore70\Modules\Microsoft.PowerShell.Management\Microsoft.PowerShell.Management.psd1

Copy link
Member Author

Choose a reason for hiding this comment

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

@daxian-dbw for some reason... for script modules, the path points to the psm1 :( Patrick is right.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. The Microsoft.PowerShell.Management is a bad example. The assembly is listed as a NestedModule, so that's literally a manifest module :) If the RootModule or ModuleToProcess is specified, then the path would point to the specified module file I guess.

A bit more logic is needed here to find out if there is a module manifest file.

Copy link
Member Author

@TylerLeonhardt TylerLeonhardt Jun 24, 2019

Choose a reason for hiding this comment

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

We can do something similar to:

$psmoduleinfo = Get-Module PowerShellGet
Join-Path $psmoduleinfo.ModuleBase "$($psmoduleinfo.Name).psd1"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Problem is you can't tell whether they've imported the psm1 or the psd1.

One could, for instance import a psm1 directly to get intellisense on functions that are typically filtered by the psd1. Conversely if you always assume psm1, you miss out on nested/required modules/assemblies and type/format files.

Copy link
Member

Choose a reason for hiding this comment

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

For the psd1 vs. psm1 case, we can tell which one was used in loading by inspecting the version. If you are importing a psm1 file, the version would be 0.0. For a psd1, it's very unlikely 0.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could override Import-Module in order to get the exact parameters used for Import-Module and then run the real import-module in each runspace.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm now importing the module base if the version is greater than 0.0. overriding Import-Module was more work than it was worth.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work for argument completers that depend on module state. Also not related to this line but argument completers not loaded by a module won't be transferred either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause some modules to throw. iirc a commonly installed version of the VMWare module does not handle being imported into multiple runspaces well. There's a few modules like that.

Copy link
Member

Choose a reason for hiding this comment

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

Also not related to this line but argument completers not loaded by a module won't be transferred either.

Can you elaborate the scenario for this one? The assembly that implements a argument completer not getting loaded by the module?

Copy link
Member

Choose a reason for hiding this comment

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

This will cause some modules to throw. iirc a commonly installed version of the VMWare module does not handle being imported into multiple runspaces well. There's a few modules like that.

This is a valid concern. Normally a module shouldn't be made only work when importing once in a process ... but we need to evaluate how bad it would be (a black list?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also not related to this line but argument completers not loaded by a module won't be transferred either

@rjmholt thought of the idea of overriding Register-ArgumentCompleter which will forward the call to the target runspace. That said, perhaps we should also ask ourselves, could running argument completers for Intellisense be a potential security concern and a valid thing to skip in the editor intellisense at least.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate the scenario for this one? The assembly that implements a argument completer not getting loaded by the module?

Sorry that was confusing in the context of this line, I mean more generally. Like someone just dot sourcing a script or manually running Register-ArgumentCompleter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, perhaps we should also ask ourselves, could running argument completers for Intellisense be a potential security concern and a valid thing to skip in the editor intellisense at least.

Considering they have to run code in order to register an argument completer I don't think it's a security issue. We do automatically import modules during intellisense which could register an argument completer. That might be a security issue, but more because loading a module runs code, not necessarily related to the fact that a completer could be loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may override this cmdlet. It's not as complicated and high profile as Import-Module

}
}

pwsh.Invoke();
}
}

if (newOrChangedVars.Count > 0)
{
// Set or update the variables.
foreach (PSVariable variable in newOrChangedVars)
{
targetEngineIntrinsics.SetVariable(variable);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the debugger is currently stopped in a different session state (like a break point in a module function), this will throw all of the module's state into the global state for the completion runspace. Also stepping through something like Pester will dump a lot of vars into completion for the rest of the session.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Need to try out this in debugging scenario.

Copy link
Member Author

@TylerLeonhardt TylerLeonhardt Jun 24, 2019

Choose a reason for hiding this comment

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

@rjmholt and I have brainstormed about this. For starters, we will skip synchronizing while the initial runspace is debugging. The belief here is that your main focus in debugging is reading, not writing. Unfortunately, this means stuff like $TestName and $TestDrive ala Pester will not be available in completions while debugging... but we think it's not critical enough for this PR.

Eventually, it would be nice to get to a point where after you finish debugging, it triggers a "resync -force" which blows away the target runspace and remakes it with a clean sync so we're back to what we would expect after debugging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The belief here is that your main focus in debugging is reading, not writing.

Writing at a breakpoint, along with $Host.EnterNestedPrompt() in general (which will also have the same problem) are the only ways to get intellisense based on a module's session state. I use this extensively.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've disabled syncing while debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the sadness, like I said, eventually I'd like to do the "trigger resync force" - both for debugging and nested prompts but it's not the highest priority for the majority of customers and I 'm not quite sure how I would approach it just yet.

Any ideas?

}
}
}

#endregion
}

internal static class RunspaceExtensions
{
private static BindingFlags bindingFlags = BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Default;

// Gets the modules loaded in a runspace.
// This exists in runspace.ExecutionContext.Modules.GetModule(string[] patterns, bool all)
internal static List<PSModuleInfo> GetModules(this Runspace runspace)
{
var executionContext = typeof(Runspace)
.GetProperty("ExecutionContext", bindingFlags)
.GetValue(runspace);
var ModuleIntrinsics = executionContext.GetType()
.GetProperty("Modules", bindingFlags)
.GetValue(executionContext);
var modules = ModuleIntrinsics.GetType()
.GetMethod("GetModules", bindingFlags, null, new Type[] { typeof(string[]), typeof(bool) }, null)
.Invoke(ModuleIntrinsics, new object[] { new string[] { "*" }, false }) as List<PSModuleInfo>;
return modules;
}

// Gets the engine intrinsics object on a Runspace.
// This exists in runspace.ExecutionContext.EngineIntrinsics.
internal static EngineIntrinsics GetEngineIntrinsics(this Runspace runspace)
{
var executionContext = typeof(Runspace)
.GetProperty("ExecutionContext", bindingFlags)
.GetValue(runspace);
var engineIntrinsics = executionContext.GetType()
.GetProperty("EngineIntrinsics", bindingFlags)
.GetValue(executionContext) as EngineIntrinsics;
return engineIntrinsics;
}
}

// Extension methods on EngineIntrinsics to streamline some setters and setters.
internal static class EngineIntrinsicsExtensions
{
private const int RETRY_ATTEMPTS = 3;
internal static List<PSVariable> GetVariables(this EngineIntrinsics engineIntrinsics)
{
List<PSVariable> variables = new List<PSVariable>();
foreach (PSObject psobject in engineIntrinsics.GetItems(ItemProviderType.Variable))
{
var variable = (PSVariable) psobject.BaseObject;
variables.Add(variable);
}
return variables;
}

internal static void SetVariable(this EngineIntrinsics engineIntrinsics, PSVariable variable)
{
engineIntrinsics.SetItem(ItemProviderType.Variable, variable.Name, variable.Value);
}

private static Collection<PSObject> GetItems(this EngineIntrinsics engineIntrinsics, ItemProviderType itemType)
{
for (int i = 0; i < RETRY_ATTEMPTS; i++)
{
try
{
return engineIntrinsics.InvokeProvider.Item.Get($@"{itemType.ToString()}:\*");
}
catch(Exception)
{
// InvokeProvider.Item.Get is not threadsafe so let's try a couple times
// to get results from it.
}
}
return new Collection<PSObject>();
}

private static void SetItem(this EngineIntrinsics engineIntrinsics, ItemProviderType itemType, string name, object value)
{
for (int i = 0; i < RETRY_ATTEMPTS; i++)
{
try
{
engineIntrinsics.InvokeProvider.Item.Set($@"{itemType}:\{name}", value);
return;
}
catch (Exception)
{
// InvokeProvider.Item.Set is not threadsafe so let's try a couple times to set.
}
}
}

private enum ItemProviderType
{
Variable,
Function,
Alias
}
}
}
Loading