From feb4cea02abf5e978d82fe9af1aa374942310dd3 Mon Sep 17 00:00:00 2001
From: "mjk.kirschner" <mjk.kirschner@gmail.com>
Date: Thu, 22 Feb 2024 20:42:01 -0500
Subject: [PATCH] review comments

---
 src/NodeServices/PythonServices.cs | 61 +++++++++++++++++-------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/src/NodeServices/PythonServices.cs b/src/NodeServices/PythonServices.cs
index 10662bc7443..a1ef744bfc9 100644
--- a/src/NodeServices/PythonServices.cs
+++ b/src/NodeServices/PythonServices.cs
@@ -106,6 +106,9 @@ internal static readonly Lazy<PythonEngineManager>
             new Lazy<PythonEngineManager>
             (() => new PythonEngineManager());
 
+        private readonly string[] dotNetRuntimePaths;
+        private readonly IEnumerable<string> dynCorePaths;
+
         #region Public members
         /// <summary>
         /// The actual instance stored in the Singleton class
@@ -131,7 +134,6 @@ internal static readonly Lazy<PythonEngineManager>
         /// IronPython2 Engine name
         /// </summary>
         internal static readonly string IronPython2EngineName = "IronPython2";
-
         internal static string PythonEvaluatorSingletonInstance = "Instance";
 
         internal static string IronPythonEvaluatorClass = "IronPythonEvaluator";
@@ -166,6 +168,8 @@ private PythonEngineManager()
                FirstOrDefault(a => a != null && a.GetName().Name == CPythonAssemblyName));
 
             AppDomain.CurrentDomain.AssemblyLoad += new AssemblyLoadEventHandler((object sender, AssemblyLoadEventArgs args) => LoadDefaultPythonEngine(args.LoadedAssembly));
+            dotNetRuntimePaths = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll");
+            dynCorePaths = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName);
         }
 
         private void LoadDefaultPythonEngine(Assembly a)
@@ -232,7 +236,8 @@ private void LoadPythonEngine(Assembly assembly)
                 {
                     throw new Exception($"Could not get a valid PythonEngine instance by calling the {eType.Name}.{PythonEvaluatorSingletonInstance} method");
                 }
-                VerifyEngineReferences();
+
+                VerifyEngineReferences(assembly,dotNetRuntimePaths.Concat(dynCorePaths));
 
                 if (GetEngine(engine.Name) == null)
                 {
@@ -243,36 +248,38 @@ private void LoadPythonEngine(Assembly assembly)
             {
                 throw new Exception($"Failed to add a Python engine from assembly {assembly.GetName().Name}.dll with error: {ex.Message}");
             }
-            
-
-            void VerifyEngineReferences()
-            {
-                MetadataLoadContext mlc = null;
-                // Retrieve the location of the assembly, net runtime assemblies, dynamo assemblies, and assemblies in the same directory tree as the python assembly.
-                var runtimeAssemblies = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll");
-                var dynCoreAssemblies = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName);
-                var localAssemblies = new FileInfo(assembly.Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName);
-
-                // Create PathAssemblyResolver that can resolve assemblies using the created list.
-                var resolver = new PathAssemblyResolver(runtimeAssemblies.Concat(dynCoreAssemblies).Concat(localAssemblies));
-                mlc = new MetadataLoadContext(resolver);
+        }
+        /// <summary>
+        /// Attempts to verify that the dependencies of the given assembly can be found and loaded into an MetadataLoadContext.
+        /// Will throw exceptions if assemblies cannot be loaded.
+        /// Used to avoid loading python engines that can only be partially loaded and will fail later at a worse time.
+        /// </summary>
+        /// <param name="assembly">assembly to check references</param>
+        /// <param name="standardPaths">standard set of assembly paths that will be used to resolve the assemblt references</param>
+        private static void VerifyEngineReferences(Assembly assembly, IEnumerable<string> standardPaths)
+        {
+            MetadataLoadContext mlc = null;
+            //get assembly paths in the same directory tree as the assembly we're verifying.
+            var localAssemPaths = new FileInfo(assembly.Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName);
 
+            // Create PathAssemblyResolver that can resolve assemblies using the list of paths.
+            var resolver = new PathAssemblyResolver(standardPaths.Concat(localAssemPaths));
+            mlc = new MetadataLoadContext(resolver);
 
-                //at this point we have found an assembly that contains a python engine
-                //and is correctly formed, let's see if we can load its dependencies in the mlc
-                //this will help test that it will not fail to load later outside of a try / catch for example.
-                try
-                {
-                    foreach (var asm in assembly.GetReferencedAssemblies())
-                    {
-                        var mlcasm = mlc.LoadFromAssemblyName(asm);
-                    }
-                }
-                finally
+            //at this point we have found an assembly that contains a python engine
+            //and is correctly formed, let's see if we can load its dependencies in the mlc
+            //this will help test that it will not fail to load later outside of a try / catch for example.
+            try
+            {
+                foreach (var asm in assembly.GetReferencedAssemblies())
                 {
-                    mlc?.Dispose();
+                    var mlcasm = mlc.LoadFromAssemblyName(asm);
                 }
             }
+            finally
+            {
+                mlc?.Dispose();
+            }
         }
     }