Use IDisposable pattern for ModuleDependencyHandler

Author: Kapil Borle

Engine/Generic/ModuleDependencyHandler.cs

@@ -238,30 +238,20 @@ private void RestorePSModulePath()
         /// <param name="tempPath">Path to the user scoped temporary directory</param>
         /// <param name="localAppDataPath">Path to the local app data directory</param>
         public ModuleDependencyHandler(
-            Runspace runspace = null,
+            Runspace runspace,
             string moduleRepository = null,
             string tempPath = null,
             string localAppDataPath = null)
         {
-            if (runspace == null)
-            {
-                Runspace = RunspaceFactory.CreateRunspace();
-            }
-            else
-            {
-                Runspace = runspace;
-            }
-            
-            if (Runspace.RunspaceStateInfo.State == RunspaceState.BeforeOpen)
-            {
-                Runspace.Open();
-            }
-            else if (Runspace.RunspaceStateInfo.State != RunspaceState.Opened)
+
+            ThrowIfNull(runspace, "runspace");
+            if (runspace.RunspaceStateInfo.State != RunspaceState.Opened)
             {
                 throw new ArgumentException(string.Format(
-                        "Runspace state cannot be {0}",
+                        "Runspace state cannot be in {0} state. It must be in Opened state",
                         runspace.RunspaceStateInfo.State.ToString()));
             }
+            Runspace = runspace;
 
             // TODO should set PSSA environment variables outside this class
             // Should be set in ScriptAnalyzer class
@@ -416,7 +406,6 @@ public static string GetModuleNameFromErrorExtent(ParseError error, ScriptBlockA
         /// </summary>
         public void Dispose()
         {
-            runspace.Dispose();
             RestorePSModulePath();
         }
 

Engine/ScriptAnalyzer.cs

@@ -45,9 +45,6 @@ public sealed class ScriptAnalyzer
         List<Regex> includeRegexList;
         List<Regex> excludeRegexList;
         bool suppressedOnly;
-        Runspace runspace;
-        ModuleDependencyHandler moduleHandler;
-
         #endregion
 
         #region Singleton
@@ -170,10 +167,6 @@ public void CleanUp()
             includeRegexList = null;
             excludeRegexList = null;
             suppressedOnly = false;
-            if (moduleHandler != null)
-            {
-                moduleHandler.Dispose();
-            }
         }
 
         internal bool ParseProfile(object profileObject, PathIntrinsics path, IOutputWriter writer)
@@ -486,10 +479,6 @@ private void Initialize(
 
             this.outputWriter = outputWriter;
 
-            // TODO Create a runspace pool
-            runspace = RunspaceFactory.CreateRunspace();
-            moduleHandler = resolveDSCResourceDependency ? new ModuleDependencyHandler(runspace) : null;
-
             #region Verifies rule extensions and loggers path
 
             List<string> paths = this.GetValidCustomRulePaths(customizedRulePath, path);
@@ -1176,16 +1165,22 @@ public IEnumerable<DiagnosticRecord> AnalyzePath(string path, bool searchRecursi
             // is an optimization over doing the whole operation at once
             // and calling .Concat on IEnumerables to join results.
             this.BuildScriptPathList(path, searchRecursively, scriptFilePaths);
-
-            foreach (string scriptFilePath in scriptFilePaths)
+            using (var rsp = RunspaceFactory.CreateRunspace())
             {
-                // Yield each record in the result so that the 
-                // caller can pull them one at a time
-                foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath))
+                rsp.Open();
+                using (var moduleHandler = new ModuleDependencyHandler(rsp))
                 {
-                    yield return diagnosticRecord;
+                    foreach (string scriptFilePath in scriptFilePaths)
+                    {
+                        // Yield each record in the result so that the
+                        // caller can pull them one at a time
+                        foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath, moduleHandler))
+                        {
+                            yield return diagnosticRecord;
+                        }
+                    }
                 }
-            }
+            } // disposing the runspace also closes it if it not already closed
         }
 
         /// <summary>
@@ -1287,7 +1282,9 @@ private void BuildScriptPathList(
         }        
 
 
-        private IEnumerable<DiagnosticRecord> AnalyzeFile(string filePath)
+        private IEnumerable<DiagnosticRecord> AnalyzeFile(
+            string filePath,
+            ModuleDependencyHandler moduleHandler = null)
         {
             ScriptBlockAst scriptAst = null;
             Token[] scriptTokens = null;

Tests/Engine/ModuleDependencyHandler.tests.ps1

@@ -26,10 +26,13 @@ Describe "Resolve DSC Resource Dependency" {
     }
 
     Context "Module handler class" {
+        $moduleHandlerType = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.ModuleDependencyHandler]
         $oldEnvVars = Get-Item Env:\* | Sort-Object -Property Key
         $oldPSModulePath = $env:PSModulePath
         It "Sets defaults correctly" {
-            $depHandler = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.ModuleDependencyHandler]::new()
+            $rsp = [runspacefactory]::CreateRunspace()
+            $rsp.Open()
+            $depHandler = $moduleHandlerType::new($rsp)
 
             $expectedPath = [System.IO.Path]::GetTempPath()
             $depHandler.TempPath | Should Be $expectedPath
@@ -47,11 +50,22 @@ Describe "Resolve DSC Resource Dependency" {
             $env:PSModulePath | Should Be $expectedPSModulePath
 
             $depHandler.Dispose()
+            $rsp.Dispose()
         }
 
         It "Keeps the environment variables unchanged" {
             Test-EnvironmentVariables($oldEnvVars)
         }
+
+        It "Throws if runspace is null" {
+            {$moduleHandlerType::new($null)} | Should Throw
+        }
+
+        It "Throws if runspace is not opened" {
+            $rsp = [runspacefactory]::CreateRunspace()
+            {$moduleHandlerType::new($rsp)} | Should Throw
+            $rsp.Dispose()
+        }
     }
 
     Context "Invoke-ScriptAnalyzer without switch" {
@@ -82,10 +96,13 @@ Describe "Resolve DSC Resource Dependency" {
         New-Item -Type Directory -Path $newTempPath
 
         # create and dispose module dependency handler object
-        # to setup the temporary module location
-        $depHandler = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.ModuleDependencyHandler]::new()
+        # to setup the temporary module
+        $rsp = [runspacefactory]::CreateRunspace()
+        $rsp.Open()
+        $depHandler = [Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.ModuleDependencyHandler]::new($rsp)
         $pssaAppDataPath = $depHandler.PSSAAppDataPath
         $tempModulePath = $depHandler.TempModulePath
+        $rsp.Dispose()
         $depHandler.Dispose()
 
         # copy myresource module to the temporary location