diff --git a/src/core/IronPython.StdLib b/src/core/IronPython.StdLib index b4f2595fa..6e08c8264 160000 --- a/src/core/IronPython.StdLib +++ b/src/core/IronPython.StdLib @@ -1 +1 @@ -Subproject commit b4f2595fa163f64227c4a0b94c6e5ce1a4cdc31b +Subproject commit 6e08c8264990fc712d122a7e8e44bd11dd964b5f diff --git a/src/core/IronPython/IronPython.csproj b/src/core/IronPython/IronPython.csproj index 47609f3f9..79cb7650b 100644 --- a/src/core/IronPython/IronPython.csproj +++ b/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}); diff --git a/src/core/IronPython/Modules/_imp.cs b/src/core/IronPython/Modules/_imp.cs index e0db2f9cf..053188424 100644 --- a/src/core/IronPython/Modules/_imp.cs +++ b/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,15 +24,6 @@ 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? @@ -43,23 +31,24 @@ public static PythonList extension_suffixes() { } 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); - } } } diff --git a/src/core/IronPython/Runtime/PythonContext.cs b/src/core/IronPython/Runtime/PythonContext.cs index 434549350..fa62c8ccc 100644 --- a/src/core/IronPython/Runtime/PythonContext.cs +++ b/src/core/IronPython/Runtime/PythonContext.cs @@ -270,7 +270,7 @@ public PythonContext(ScriptDomainManager/*!*/ manager, IDictionary/*!*/ SystemStateModules { get; } = new PythonDictionary(); + /// + /// The global import lock (_imp.acquire_lock), re-entrant and owned by a thread. + /// + 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(params object[] args) { /// /// 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. /// internal Hosting.PythonService GetPythonService(Microsoft.Scripting.Hosting.ScriptEngine engine) { @@ -1722,7 +1727,7 @@ private Dictionary 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> MakeConvertSite(ConversionResultK /// /// 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) /// internal object Operation(PythonOperationKind operation, object self, object other) { @@ -3010,13 +3015,13 @@ internal CultureInfo NumericCulture { /// /// 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. /// @@ -3117,8 +3122,8 @@ public int Compare(object x, object y) { /// /// 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. /// internal IComparer GetComparer(Type type) { diff --git a/tests/suite/test_imp.py b/tests/suite/test_imp.py index 640a73a18..a929bfff2 100644 --- a/tests/suite/test_imp.py +++ b/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)