Fix locking in _imp

src/core/IronPython/IronPython.csproj

@@ -21,6 +21,7 @@
       M:System.String.Contains(System.Char);
       M:System.String.EndsWith(System.Char);
       M:System.String.StartsWith(System.Char);
+      T:System.Threading.Lock;
       M:System.Text.Encoding.GetString(System.ReadOnlySpan{System.Byte});
     </MeziantouPolyfill_IncludedPolyfills>
   </PropertyGroup>

src/core/IronPython/Modules/_imp.cs

@@ -4,13 +4,10 @@
 
 using System;
 using System.Diagnostics;
-using System.Runtime.CompilerServices;
+using System.Threading;
 
 using IronPython.Runtime;
 using IronPython.Runtime.Operations;
-using IronPython.Runtime.Types;
-
-using Microsoft.Scripting.Utils;
 
 [assembly: PythonModule("_imp", typeof(IronPython.Modules.PythonImport))]
 namespace IronPython.Modules {
@@ -27,39 +24,31 @@ public static class PythonImport {
         internal const int PythonCodeResource = 8;
         internal const int SearchError = 0;
         internal const int ImporterHook = 9;
-        private static readonly object _lockCountKey = new object();
-
-        [SpecialName]
-        public static void PerformModuleReload(PythonContext/*!*/ context, PythonDictionary/*!*/ dict) {
-            // set the lock count to zero on the 1st load, don't reset the lock count on reloads
-            if (!context.HasModuleState(_lockCountKey)) {
-                context.SetModuleState(_lockCountKey, 0L);
-            }
-        }
 
         public static PythonList extension_suffixes() {
             // TODO: support extensions?
             return new PythonList();
         }
 
         public static bool lock_held(CodeContext/*!*/ context) {
-            return GetLockCount(context) != 0;
+            // held by any thread
+            Lock importLock = context.LanguageContext.ImportLock;
+            if (importLock.IsHeldByCurrentThread) return true; // the current thread already holds the lock
+            if (!importLock.TryEnter()) return true; // try an exclusive lock, another thread might already hold the lock
+            importLock.Exit();
+            return false;
         }
 
         public static void acquire_lock(CodeContext/*!*/ context) {
-            lock (_lockCountKey) {
-                SetLockCount(context, GetLockCount(context) + 1);
-            }
+            context.LanguageContext.ImportLock.Enter();
         }
 
         public static void release_lock(CodeContext/*!*/ context) {
-            lock (_lockCountKey) {
-                long lockCount = GetLockCount(context);
-                if (lockCount == 0) {
-                    throw PythonOps.RuntimeError("not holding the import lock");
-                }
-                SetLockCount(context, lockCount - 1);
-            }            
+            Lock importLock = context.LanguageContext.ImportLock;
+            if (!importLock.IsHeldByCurrentThread) {
+                throw PythonOps.RuntimeError("not holding the import lock");
+            }
+            importLock.Exit();
         }
 
         public static object init_builtin(CodeContext/*!*/ context, [NotNone] string/*!*/ name) {
@@ -107,7 +96,7 @@ public static int is_builtin(CodeContext/*!*/ context, string/*!*/ name) {
                     return -1;
                 }
 
-                
+
                 return 1;
             }
             return 0;
@@ -124,13 +113,5 @@ public static bool is_frozen_package(string name) {
         public static void _fix_co_filename() {
             throw new NotImplementedException();
         }
-
-        private static long GetLockCount(CodeContext/*!*/ context) {
-            return (long)context.LanguageContext.GetModuleState(_lockCountKey);
-        }
-
-        private static void SetLockCount(CodeContext/*!*/ context, long lockCount) {
-            context.LanguageContext.SetModuleState(_lockCountKey, lockCount);
-        }
     }
 }

src/core/IronPython/Runtime/PythonContext.cs

@@ -270,7 +270,7 @@ public PythonContext(ScriptDomainManager/*!*/ manager, IDictionary<string, objec
                 try {
                     HookAssemblyResolve();
                 } catch (System.Security.SecurityException) {
-                    // We may not have SecurityPermissionFlag.ControlAppDomain. 
+                    // We may not have SecurityPermissionFlag.ControlAppDomain.
                     // If so, we will not look up sys.path for module loads
                 }
             }
@@ -666,6 +666,11 @@ internal bool TryGetSystemPath(out PythonList path) {
 
         internal IDictionary<object, object>/*!*/ SystemStateModules { get; } = new PythonDictionary();
 
+        /// <summary>
+        /// The global import lock (_imp.acquire_lock), re-entrant and owned by a thread.
+        /// </summary>
+        internal Lock ImportLock { get; } = new Lock();
+
         internal PythonModule GetModuleByName(string/*!*/ name) {
             Assert.NotNull(name);
             if (SystemStateModules.TryGetValue(name, out object scopeObj) && scopeObj is PythonModule module) {
@@ -1270,7 +1275,7 @@ private void UnhookAssemblyResolve() {
             try {
                 AppDomain.CurrentDomain.AssemblyResolve -= _resolveHolder.AssemblyResolveEvent;
             } catch (System.Security.SecurityException) {
-                // We may not have SecurityPermissionFlag.ControlAppDomain. 
+                // We may not have SecurityPermissionFlag.ControlAppDomain.
                 // If so, we will not look up sys.path for module loads
             }
         }
@@ -1614,7 +1619,7 @@ public override TService GetService<TService>(params object[] args) {
 
         /// <summary>
         /// Returns (and creates if necessary) the PythonService that is associated with this PythonContext.
-        /// 
+        ///
         /// The PythonService is used for providing remoted convenience helpers for the DLR hosting APIs.
         /// </summary>
         internal Hosting.PythonService GetPythonService(Microsoft.Scripting.Hosting.ScriptEngine engine) {
@@ -1722,7 +1727,7 @@ private Dictionary<string, Type> CreateBuiltinTable() {
 
                 if (Environment.OSVersion.Platform == PlatformID.Unix) {
                     // we make our nt package show up as a posix package
-                    // on unix platforms.  Because we build on top of the 
+                    // on unix platforms.  Because we build on top of the
                     // CLI for all file operations we should be good from
                     // there, but modules that check for the presence of
                     // names (e.g. os) will do the right thing.
@@ -2552,8 +2557,8 @@ private CallSite<Func<CallSite, object, T>> MakeConvertSite<T>(ConversionResultK
 
         /// <summary>
         /// Invokes the specified operation on the provided arguments and returns the new resulting value.
-        /// 
-        /// operation is usually a value from StandardOperators (standard CLR/DLR operator) or 
+        ///
+        /// operation is usually a value from StandardOperators (standard CLR/DLR operator) or
         /// OperatorStrings (a Python specific operator)
         /// </summary>
         internal object Operation(PythonOperationKind operation, object self, object other) {
@@ -3010,13 +3015,13 @@ internal CultureInfo NumericCulture {
         /// <summary>
         /// Sets the current command dispatcher for the Python command line.  The previous dispatcher
         /// is returned.  Null can be passed to remove the current command dispatcher.
-        /// 
+        ///
         /// The command dispatcher will be called with a delegate to be executed.  The command dispatcher
         /// should invoke the target delegate in the desired context.
-        /// 
+        ///
         /// A common use for this is to enable running all REPL commands on the UI thread while the REPL
         /// continues to run on a non-UI thread.
-        /// 
+        ///
         /// The ipy.exe REPL will call into PythonContext.DispatchCommand to dispatch each execution to
         /// the correct thread.  Other REPLs can do the same to support this functionality as well.
         /// </summary>
@@ -3117,8 +3122,8 @@ public int Compare(object x, object y) {
         /// <summary>
         /// Gets a function which can be used for comparing two values using the normal
         /// Python semantics.
-        /// 
-        /// If type is null then a generic comparison function is returned.  If type is 
+        ///
+        /// If type is null then a generic comparison function is returned.  If type is
         /// not null a comparison function is returned that's used for just that type.
         /// </summary>
         internal IComparer GetComparer(Type type) {

tests/suite/test_imp.py

@@ -2,9 +2,11 @@
 # The .NET Foundation licenses this file to you under the Apache 2.0 License.
 # See the LICENSE file in the project root for more information.
 
+import gc
 import imp
 import os
 import sys
+import threading
 import unittest
 
 from iptest import IronPythonTestCase, is_cli, is_posix, path_modifier, run_test, skipUnlessIronPython
@@ -257,16 +259,39 @@ def test_module_dict(self):
         self.assertEqual(type({}), type(currentModule.__dict__))
         self.assertEqual(isinstance(currentModule.__dict__, dict), True)
 
+    @unittest.skipUnless(is_cli or sys.version_info >= (3, 6), "CPython < 3.6 leaks the import lock in test_load_package")
     def test_lock(self):
-        i=0
-        while i<5:
-            i+=1
-            if not imp.lock_held():
-                self.assertRaises(RuntimeError,imp.release_lock)
-                imp.acquire_lock()
-            else:
+        self.assertFalse(imp.lock_held())
+        self.assertRaises(RuntimeError, imp.release_lock)
+        imp.acquire_lock()
+        try:
+            self.assertTrue(imp.lock_held())
+            # re-entrant
+            imp.acquire_lock()
+            imp.release_lock()
+            self.assertTrue(imp.lock_held())
+        finally:
+            imp.release_lock()
+        self.assertFalse(imp.lock_held())
+
+    @unittest.skipUnless(is_cli or sys.version_info >= (3, 6), "CPython < 3.6 leaks the import lock in test_load_package")
+    def test_lock_gc_collect(self):
+        # weakref callbacks run on the finalizer thread; if one took the import lock (like importlib's
+        # module lock cleanup used to), gc.collect() would wait on it forever while the lock is held
+        def run():
+            imp.acquire_lock()
+            try:
+                sys.modules.pop('colorsys', None)
+                import colorsys # creates a module lock which dies once the import completes
+                gc.collect()
+            finally:
                 imp.release_lock()
 
+        t = threading.Thread(target=run, daemon=True)
+        t.start()
+        t.join(30)
+        self.assertFalse(t.is_alive(), "deadlock between the import lock and gc.collect()")
+
     def test_is_frozen(self):
         for name in self.temp_name:
             f = imp.is_frozen(name)