Skip to content
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

Change CommandInfoCache to implement IDisposable and clean up the runspace pool #1335

Merged
merged 8 commits into from
Sep 12, 2019
30 changes: 27 additions & 3 deletions Engine/CommandInfoCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,44 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
/// <summary>
/// Provides threadsafe caching around CommandInfo lookups with `Get-Command -Name ...`.
/// </summary>
internal class CommandInfoCache
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
internal class CommandInfoCache : IDisposable
{
private readonly ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>> _commandInfoCache;
private readonly Helper _helperInstance;
private readonly RunspacePool _runspacePool;
private bool disposed = false;

/// <summary>
/// Create a fresh command info cache instance.
/// </summary>
public CommandInfoCache(Helper pssaHelperInstance, RunspacePool runspacePool)
public CommandInfoCache(Helper pssaHelperInstance)
{
_commandInfoCache = new ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>>();
_helperInstance = pssaHelperInstance;
_runspacePool = runspacePool;
_runspacePool = RunspaceFactory.CreateRunspacePool(1, 10);
_runspacePool.Open();
}

/// <summary>Dispose the runspace pool</summary>
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
if ( disposed )
{
return;
}

if ( disposing )
{
_runspacePool.Dispose();
}

disposed = true;
}

/// <summary>
Expand Down
8 changes: 1 addition & 7 deletions Engine/Helper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public class Helper
private PSVersionTable psVersionTable;

private readonly Lazy<CommandInfoCache> _commandInfoCacheLazy;
private readonly RunspacePool _runSpacePool;
private readonly object _testModuleManifestLock = new object();

#endregion
Expand Down Expand Up @@ -116,11 +115,7 @@ internal set
/// </summary>
private Helper()
{
// There are 5 rules that use the CommandInfo cache but one rule (AvoidAlias) makes parallel queries.
// Therefore 10 runspaces was a heuristic measure where no more speed improvement was seen.
_runSpacePool = RunspaceFactory.CreateRunspacePool(1, 10);
_runSpacePool.Open();
_commandInfoCacheLazy = new Lazy<CommandInfoCache>(() => new CommandInfoCache(pssaHelperInstance: this, runspacePool: _runSpacePool));
_commandInfoCacheLazy = new Lazy<CommandInfoCache>(() => new CommandInfoCache(pssaHelperInstance: this));
}

/// <summary>
Expand Down Expand Up @@ -309,7 +304,6 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable<ErrorReco
{
using (var ps = System.Management.Automation.PowerShell.Create())
{
ps.RunspacePool = _runSpacePool;
ps.AddCommand("Test-ModuleManifest")
.AddParameter("Path", filePath)
.AddParameter("WarningAction", ActionPreference.SilentlyContinue);
Expand Down
7 changes: 7 additions & 0 deletions Tests/Engine/Helper.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,11 @@ Describe "Test Directed Graph" {
$digraph.IsConnected('v1', 'v4') | Should -BeTrue
}
}

Context "Runspaces should be disposed" {
It "Running analyzer 100 times should not result in additional runspaces" {
JamesWTruher marked this conversation as resolved.
Show resolved Hide resolved
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
$null = 1..100 | %{ Invoke-ScriptAnalyzer -ScriptDefinition 'gci' }
JamesWTruher marked this conversation as resolved.
Show resolved Hide resolved
(Get-Runspace).Count | Should -BeLessThan 10
JamesWTruher marked this conversation as resolved.
Show resolved Hide resolved
}
}
}