diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index da43d0ee1..dad365f99 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -23,7 +23,17 @@ internal class CommandInfoCache : IDisposable private const int MaxLookupAttempts = 3; private readonly ConcurrentDictionary> _commandInfoCache; - private readonly RunspacePool _runspacePool; + + /// + /// Guards all access to so that only one thread at a time drives the + /// PowerShell engine. The engine is not thread safe, so concurrent lookups can fail transiently, + /// see https://github.com/PowerShell/PowerShell/issues/4003. + /// A monitor is used rather than a semaphore because it is re-entrant, which avoids a deadlock + /// should a lookup ever end up calling back into the cache on the same thread. + /// + private readonly object _runspaceLock = new object(); + + private readonly Runspace _runspace; private bool disposed = false; /// @@ -32,11 +42,13 @@ internal class CommandInfoCache : IDisposable public CommandInfoCache() { _commandInfoCache = new ConcurrentDictionary>(); - _runspacePool = RunspaceFactory.CreateRunspacePool(1, 10); - _runspacePool.Open(); + // A single runspace rather than a pool: all lookups are serialized on it, so that the + // PowerShell engine is never driven concurrently. + _runspace = RunspaceFactory.CreateRunspace(); + _runspace.Open(); } - /// Dispose the runspace pool + /// Dispose the runspace public void Dispose() { Dispose(true); @@ -45,17 +57,23 @@ public void Dispose() protected virtual void Dispose(bool disposing) { - if ( disposed ) + // Always take the lock, also on the finalizer path, so that 'disposed' is never + // published without the runspace being disposed along with it and so that the runspace + // cannot be disposed while a lookup is in flight. + lock (_runspaceLock) { - return; - } + if ( disposed ) + { + return; + } - if ( disposing ) - { - _runspacePool.Dispose(); - } + disposed = true; - disposed = true; + if ( disposing ) + { + _runspace.Dispose(); + } + } } /// @@ -123,41 +141,51 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command for (int attempt = 1; ; attempt++) { - using (var ps = System.Management.Automation.PowerShell.Create()) + // Serialize all use of the PowerShell engine. Only cache misses reach this point; + // lookups that are already cached are served without taking the lock. + lock (_runspaceLock) { - ps.RunspacePool = _runspacePool; - - ps.AddCommand("Get-Command") - .AddParameter("Name", actualCmdName) - .AddParameter("ErrorAction", "SilentlyContinue"); - - if (commandType != null) + if (disposed) { - ps.AddParameter("CommandType", commandType); + return null; } - if (!string.IsNullOrEmpty(moduleName)) + using (var ps = System.Management.Automation.PowerShell.Create()) { - ps.AddParameter("Module", moduleName); - } + ps.Runspace = _runspace; - try - { - return ps.Invoke() - .FirstOrDefault(); - } - // 'Get-Command' is invoked with 'SilentlyContinue', so a CommandNotFoundException can only - // mean that the engine failed to resolve 'Get-Command' itself in the pooled runspace. - // That happens intermittently because the PowerShell engine is not thread safe, see - // https://github.com/PowerShell/PowerShell/issues/4003 and - // https://github.com/PowerShell/PSScriptAnalyzer/issues/2205 - // Retrying usually succeeds, but rather than failing the whole analysis when it does not, - // treat the command as unresolvable. - catch (CommandNotFoundException) - { - if (attempt >= MaxLookupAttempts) + ps.AddCommand("Get-Command") + .AddParameter("Name", actualCmdName) + .AddParameter("ErrorAction", "SilentlyContinue"); + + if (commandType != null) + { + ps.AddParameter("CommandType", commandType); + } + + if (!string.IsNullOrEmpty(moduleName)) + { + ps.AddParameter("Module", moduleName); + } + + try + { + return ps.Invoke() + .FirstOrDefault(); + } + // 'Get-Command' is invoked with 'SilentlyContinue', so a CommandNotFoundException can only + // mean that the engine failed to resolve 'Get-Command' itself in the runspace. + // That happened intermittently when lookups ran concurrently because the PowerShell engine + // is not thread safe, see https://github.com/PowerShell/PowerShell/issues/4003 and + // https://github.com/PowerShell/PSScriptAnalyzer/issues/2205 + // Lookups are serialized now, so this should no longer occur, but the retry is kept as a + // safety net for hosts that drive the engine from other threads at the same time. + catch (CommandNotFoundException) { - return null; + if (attempt >= MaxLookupAttempts) + { + return null; + } } } } diff --git a/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 b/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 new file mode 100644 index 000000000..10f9c1047 --- /dev/null +++ b/Tests/Engine/CommandInfoCacheConcurrency.tests.ps1 @@ -0,0 +1,64 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +Describe "Concurrent command lookups" { + BeforeAll { + # Run the analyzer once so that the singleton Helper is created by the cmdlet. Touching + # Helper.Instance before that would install a helper without a command invocation context, + # which breaks every later analysis in this process. + $null = Invoke-ScriptAnalyzer -ScriptDefinition 'Get-Item -Path .' + + # The concurrency driver is written in C# so that the lookups really do run on separate + # threads. Invoking a PowerShell script block on a thread pool thread would introduce + # runspace affinity problems of its own and would not test the command info cache. + $analyzerAssembly = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Helper].Assembly.Location + Add-Type -IgnoreWarnings -WarningAction SilentlyContinue -ReferencedAssemblies $analyzerAssembly, ([System.Management.Automation.PSObject].Assembly.Location) -TypeDefinition @' +using System.Threading.Tasks; +using Microsoft.Windows.PowerShell.ScriptAnalyzer; + +public static class ConcurrentCommandLookup +{ + public static string[] Lookup(string[] commandNames) + { + var helper = Helper.Instance; + var tasks = new Task[commandNames.Length]; + for (int i = 0; i < commandNames.Length; i++) + { + string name = commandNames[i]; + tasks[i] = Task.Run(() => + { + var commandInfo = helper.GetCommandInfo(name); + return commandInfo == null ? null : commandInfo.Name; + }); + } + + Task.WaitAll(tasks); + + var results = new string[tasks.Length]; + for (int i = 0; i < tasks.Length; i++) + { + results[i] = tasks[i].Result; + } + + return results; + } +} +'@ + } + + It "resolves commands from several threads without failing" { + $commandNames = @( + 'Get-ChildItem', 'Where-Object', 'ForEach-Object', 'Get-Content', 'Write-Output', + 'Test-Path', 'Get-Command', 'Select-Object', 'Sort-Object', 'Measure-Object' + ) * 4 + + # A lookup that hits the thread safety problem throws, which fails the test. + $results = [ConcurrentCommandLookup]::Lookup($commandNames) + + $results.Count | Should -Be $commandNames.Count + # A failed lookup returns null, so every entry must name the command that was requested. + for ($i = 0; $i -lt $commandNames.Count; $i++) { + $results[$i] | Should -BeExactly $commandNames[$i] + } + } +}