From 48d7af4ba55044acdadb4746396e9ba41b6dc357 Mon Sep 17 00:00:00 2001 From: James Truher Date: Wed, 11 Sep 2019 14:24:18 -0700 Subject: [PATCH 1/8] Change helper to implement IDisposable and clean up the runspace pool The Helper is recreated with every invocation of Invoke-ScriptAnalyzer which essentially resulted in a runspace which was not cleaned up for every invocation. I saw 100s of runspaces (ia get-runspace) in my session. I also gave the CommandInfoCache it's own runspace pool and implemented IDisposable in it. Lastly, I've added a test to catch this in the future. --- Engine/CommandInfoCache.cs | 12 +++++++++--- Engine/Helper.cs | 18 ++++++++++-------- Tests/Engine/Helper.tests.ps1 | 7 +++++++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index 67e35c211..791aaaeac 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -12,7 +12,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer /// /// Provides threadsafe caching around CommandInfo lookups with `Get-Command -Name ...`. /// - internal class CommandInfoCache + internal class CommandInfoCache : IDisposable { private readonly ConcurrentDictionary> _commandInfoCache; private readonly Helper _helperInstance; @@ -21,11 +21,17 @@ internal class CommandInfoCache /// /// Create a fresh command info cache instance. /// - public CommandInfoCache(Helper pssaHelperInstance, RunspacePool runspacePool) + public CommandInfoCache(Helper pssaHelperInstance) { _commandInfoCache = new ConcurrentDictionary>(); _helperInstance = pssaHelperInstance; - _runspacePool = runspacePool; + _runspacePool = RunspaceFactory.CreateRunspacePool(1, 10); + _runspacePool.Open(); + } + + public void Dispose() + { + _runspacePool.Dispose(); } /// diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 330547d02..7b8582ae4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -19,7 +19,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer /// /// This Helper class contains utility/helper functions for classes in ScriptAnalyzer. /// - public class Helper + public class Helper : IDisposable { #region Private members @@ -30,8 +30,8 @@ public class Helper private PSVersionTable psVersionTable; private readonly Lazy _commandInfoCacheLazy; - private readonly RunspacePool _runSpacePool; private readonly object _testModuleManifestLock = new object(); + private readonly RunspacePool _runspacePool; #endregion @@ -116,11 +116,13 @@ internal set /// 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(() => new CommandInfoCache(pssaHelperInstance: this, runspacePool: _runSpacePool)); + _runspacePool = RunspaceFactory.CreateRunspacePool(1,10); + _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this)); + } + + public void Dispose() + { + _runspacePool.Dispose(); } /// @@ -309,7 +311,7 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable Date: Wed, 11 Sep 2019 16:29:55 -0700 Subject: [PATCH 2/8] Remove runspace from helper It no longer needs to be IDisposable Implement dispose in cache along suggested guidelines --- Engine/CommandInfoCache.cs | 20 +++++++++++++++++++- Engine/Helper.cs | 10 +--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index 791aaaeac..61e78d86b 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -17,6 +17,7 @@ internal class CommandInfoCache : IDisposable private readonly ConcurrentDictionary> _commandInfoCache; private readonly Helper _helperInstance; private readonly RunspacePool _runspacePool; + private bool disposed = false; /// /// Create a fresh command info cache instance. @@ -29,9 +30,26 @@ public CommandInfoCache(Helper pssaHelperInstance) _runspacePool.Open(); } + /// Dispose the runspace pool public void Dispose() { - _runspacePool.Dispose(); + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if ( disposed ) + { + return; + } + + if ( disposing ) + { + _runspacePool.Dispose(); + } + + disposed = true; } /// diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 7b8582ae4..71be95b4b 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -19,7 +19,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer /// /// This Helper class contains utility/helper functions for classes in ScriptAnalyzer. /// - public class Helper : IDisposable + public class Helper { #region Private members @@ -31,7 +31,6 @@ public class Helper : IDisposable private readonly Lazy _commandInfoCacheLazy; private readonly object _testModuleManifestLock = new object(); - private readonly RunspacePool _runspacePool; #endregion @@ -116,15 +115,9 @@ internal set /// private Helper() { - _runspacePool = RunspaceFactory.CreateRunspacePool(1,10); _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this)); } - public void Dispose() - { - _runspacePool.Dispose(); - } - /// /// Initializes the Helper class. /// @@ -311,7 +304,6 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable Date: Thu, 12 Sep 2019 11:51:07 -0700 Subject: [PATCH 3/8] Update Tests/Engine/Helper.tests.ps1 Co-Authored-By: Christoph Bergmeister [MVP] --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index db9f55c88..fda58961f 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -28,7 +28,7 @@ Describe "Test Directed Graph" { } Context "Runspaces should be disposed" { - It "Running analyzer 100 times should not result in additional runspaces" { + It "Running analyzer 100 times should not result in additional runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { $null = 1..100 | %{ Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } (Get-Runspace).Count | Should -BeLessThan 10 } From 6ee6dd90ff83febdcc9c37b9ddf56a0b64ee2ea8 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Thu, 12 Sep 2019 11:51:57 -0700 Subject: [PATCH 4/8] Update Tests/Engine/Helper.tests.ps1 Co-Authored-By: Christoph Bergmeister [MVP] --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index fda58961f..b456a15cb 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -29,7 +29,7 @@ Describe "Test Directed Graph" { Context "Runspaces should be disposed" { It "Running analyzer 100 times should not result in additional runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { - $null = 1..100 | %{ Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } + $null = 1..100 | ForEach-Object { Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } (Get-Runspace).Count | Should -BeLessThan 10 } } From ca6fe8391ac66e42db07788744be0917abf74e72 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Thu, 12 Sep 2019 11:55:13 -0700 Subject: [PATCH 5/8] Update Tests/Engine/Helper.tests.ps1 Co-Authored-By: Christoph Bergmeister [MVP] --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index b456a15cb..fc5bd7dc1 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -30,7 +30,7 @@ Describe "Test Directed Graph" { Context "Runspaces should be disposed" { It "Running analyzer 100 times should not result in additional runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { $null = 1..100 | ForEach-Object { Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } - (Get-Runspace).Count | Should -BeLessThan 10 + (Get-Runspace).Count | Should -BeLessOrEqual 10 -Because 'Number of Runspaces should not exceed size of runspace pool cache in CommandInfoCache' } } } From 27d2f77558bffcbb039e71f6072a9aa660527685 Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 12 Sep 2019 12:07:21 -0700 Subject: [PATCH 6/8] update test language --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index fc5bd7dc1..d2ec70bad 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -28,7 +28,7 @@ Describe "Test Directed Graph" { } Context "Runspaces should be disposed" { - It "Running analyzer 100 times should not result in additional runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { + It "Running analyzer 100 times should only create a limited number of runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { $null = 1..100 | ForEach-Object { Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } (Get-Runspace).Count | Should -BeLessOrEqual 10 -Because 'Number of Runspaces should not exceed size of runspace pool cache in CommandInfoCache' } From dba171fa19eedbe6799fc949c3891d0a3cf4ba31 Mon Sep 17 00:00:00 2001 From: "James Truher [MSFT]" Date: Thu, 12 Sep 2019 12:46:13 -0700 Subject: [PATCH 7/8] Update Tests/Engine/Helper.tests.ps1 Co-Authored-By: Christoph Bergmeister [MVP] --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index d2ec70bad..e88afbe78 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -28,7 +28,7 @@ Describe "Test Directed Graph" { } Context "Runspaces should be disposed" { - It "Running analyzer 100 times should only create a limited number of runspaces" -Skip ($PSVersionTable.PSVersion -le '4') { + It "Running analyzer 100 times should only create a limited number of runspaces" -Skip:$($PSVersionTable.PSVersion -le '4') { $null = 1..100 | ForEach-Object { Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } (Get-Runspace).Count | Should -BeLessOrEqual 10 -Because 'Number of Runspaces should not exceed size of runspace pool cache in CommandInfoCache' } From 7287dd010b79d5823fd7e42cf4c4347cad18bad5 Mon Sep 17 00:00:00 2001 From: James Truher Date: Thu, 12 Sep 2019 13:04:20 -0700 Subject: [PATCH 8/8] Fix version comparison to just use major number --- Tests/Engine/Helper.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index e88afbe78..3217e686e 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -28,7 +28,7 @@ Describe "Test Directed Graph" { } Context "Runspaces should be disposed" { - It "Running analyzer 100 times should only create a limited number of runspaces" -Skip:$($PSVersionTable.PSVersion -le '4') { + It "Running analyzer 100 times should only create a limited number of runspaces" -Skip:$($PSVersionTable.PSVersion.Major -le 4) { $null = 1..100 | ForEach-Object { Invoke-ScriptAnalyzer -ScriptDefinition 'gci' } (Get-Runspace).Count | Should -BeLessOrEqual 10 -Because 'Number of Runspaces should not exceed size of runspace pool cache in CommandInfoCache' }