PR review feedback

Author: Mohamed Koubaa

netfx_loader/DomainData.cs

@@ -10,24 +10,26 @@ namespace ClrLoader
     public static class DomainSetup
     {
         public delegate int EntryPoint(IntPtr buffer, int size);
+
         public static void StoreFunctorFromDomainData()
         {
             var domain = AppDomain.CurrentDomain;
             var assemblyPath = (string)domain.GetData("_assemblyPath");
             var typeName = (string)domain.GetData("_typeName");
             var function = (string)domain.GetData("_function");
-            var functor = GetFunctor(domain, assemblyPath, typeName, function);
+            var deleg = GetDelegate(domain, assemblyPath, typeName, function);
+            var functor = Marshal.GetFunctionPointerForDelegate(deleg);
+            domain.SetData("_thisDelegate", deleg);
             domain.SetData("_thisFunctor", functor);
         }
 
-        private static IntPtr GetFunctor(AppDomain domain, string assemblyPath, string typeName, string function)
+        private static Delegate GetDelegate(AppDomain domain, string assemblyPath, string typeName, string function)
         {
-            var assemblyName = AssemblyName.GetAssemblyName(assemblyPath).Name;
-            var assembly = domain.Load(AssemblyName.GetAssemblyName(assemblyPath));
+            var assemblyName = AssemblyName.GetAssemblyName(assemblyPath);
+            var assembly = domain.Load(assemblyName);
             var type = assembly.GetType(typeName, throwOnError: true);
             var deleg = Delegate.CreateDelegate(typeof(EntryPoint), type, function);
-            IntPtr result = Marshal.GetFunctionPointerForDelegate(deleg);
-            return result;
+            return deleg;
         }
     }
 
@@ -61,27 +63,46 @@ private void installResolver(string assemblyPath)
             };
         }
 
+        private static readonly object _lockObj = new object();
+
         public IntPtr GetFunctor(string assemblyPath, string typeName, string function)
         {
             if (_disposed)
                 throw new InvalidOperationException("Domain is already disposed");
 
-            installResolver(assemblyPath);
-
-            var key = (assemblyPath, typeName, function);
-
-            IntPtr result;
-            if (!_functors.TryGetValue(key, out result))
+            // neither the domain data nor the _functors dictionary is threadsafe
+            lock (_lockObj)
             {
-                Domain.SetData("_assemblyPath", assemblyPath);
-                Domain.SetData("_typeName", typeName);
-                Domain.SetData("_function", function);
-
-                Domain.DoCallBack(new CrossAppDomainDelegate(DomainSetup.StoreFunctorFromDomainData));
-                result = (IntPtr)Domain.GetData("_thisFunctor");
-                _functors[key] = result;
+                installResolver(assemblyPath);
+                var assemblyName = AssemblyName.GetAssemblyName(assemblyPath).Name;
+
+                var key = (assemblyName, typeName, function);
+
+                IntPtr result;
+                if (!_functors.TryGetValue(key, out result))
+                {
+                    Domain.SetData("_assemblyPath", assemblyPath);
+                    Domain.SetData("_typeName", typeName);
+                    Domain.SetData("_function", function);
+
+                    Domain.DoCallBack(new CrossAppDomainDelegate(DomainSetup.StoreFunctorFromDomainData));
+                    result = (IntPtr)Domain.GetData("_thisFunctor");
+                    if (result == IntPtr.Zero)
+                        throw new Exception($"Unable to get functor for {assemblyName}, {typeName}, {function}");
+
+                    // set inputs to StoreFunctorFromDomainData to null.
+                    // (There's no method to explicitly clear domain data)
+                    Domain.SetData("_assemblyPath", null);
+                    Domain.SetData("_typeName", null);
+                    Domain.SetData("_function", null);
+
+                    // the result has to remain in the domain data because we don't know when the
+                    // client of pyclr_get_function will actually invoke the functor, and if we
+                    // remove it from the domain data after returning it may get collected too early.
+                    _functors[key] = result;
+                }
+                return result;
             }
-            return result;
         }
 
         public void Dispose()