[GR-60399] Fix GIL hold count assertion in PythonContext.releaseGil.

PullRequest: graalpython/3613

Author: steve-s

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ImpModuleBuiltins.java

@@ -122,6 +122,7 @@
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.RootCallTarget;
+import com.oracle.truffle.api.TruffleSafepoint;
 import com.oracle.truffle.api.dsl.Bind;
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Fallback;
@@ -216,7 +217,7 @@ public abstract static class AcquireLock extends PythonBuiltinNode {
         public Object run(@Cached GilNode gil) {
             gil.release(true);
             try {
-                getContext().getImportLock().lock();
+                TruffleSafepoint.setBlockedThreadInterruptible(this, ReentrantLock::lock, getContext().getImportLock());
             } finally {
                 gil.acquire();
             }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/TimeModuleBuiltins.java

@@ -511,7 +511,6 @@ Object sleep(PythonModule self, long seconds,
                 ModuleState moduleState = self.getModuleState(ModuleState.class);
                 moduleState.timeSlept = nanoTime() - t + moduleState.timeSlept;
             }
-            PythonContext.triggerAsyncActions(this);
             return PNone.NONE;
         }
 
@@ -581,8 +580,8 @@ private static void doSleep(Node node, double seconds, double deadline) {
                     try {
                         Thread.sleep(millis, nanos);
                     } catch (InterruptedException ignored) {
+                        // Truffle would otherwise execute the action again after interrupt
                         Thread.currentThread().interrupt();
-                        return;
                     }
                 }, secs);
                 secs = deadline - timeSeconds();

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeBuiltins.java

@@ -303,7 +303,7 @@ static Object doc(Object self, @SuppressWarnings("unused") DescriptorDeleteMarke
 
     @Builtin(name = J___MRO__, minNumOfPositionalArgs = 1, isGetter = true)
     @GenerateNodeFactory
-    abstract static class MroAttrNode extends PythonBuiltinNode {
+    abstract static class MroAttrNode extends PythonUnaryBuiltinNode {
         @Specialization
         static Object doit(Object klass,
                         @Bind("this") Node inliningTarget,

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/PythonContext.java

@@ -175,6 +175,7 @@
 import com.oracle.graal.python.util.PythonUtils;
 import com.oracle.graal.python.util.ShutdownHook;
 import com.oracle.graal.python.util.Supplier;
+import com.oracle.graal.python.util.SuppressFBWarnings;
 import com.oracle.truffle.api.CallTarget;
 import com.oracle.truffle.api.CompilerAsserts;
 import com.oracle.truffle.api.CompilerDirectives;
@@ -2480,9 +2481,26 @@ static final String dumpStackOnAssertionHelper(String msg) {
      * @see GilNode
      */
     @TruffleBoundary
+    // intentional catch of IllegalMonitorStateException, see inline comments
+    @SuppressFBWarnings("IMSE_DONT_CATCH_IMSE")
     void releaseGil() {
-        assert globalInterpreterLock.getHoldCount() == 1 : dumpStackOnAssertionHelper("trying to release the GIL with invalid hold count " + globalInterpreterLock.getHoldCount());
-        globalInterpreterLock.unlock();
+        // We allow hold count == 0 when cancelling, because a thread may have given up the GIL,
+        // then a cancelling (subclass of ThreadDeath) exception is thrown inside the code running
+        // without GIL through thread local action and in such case, we do not try to reacquire the
+        // GIL and let the ThreadDeath bubble up the stack to the top level and along the way we
+        // may have some finally blocks that are trying to release the initially acquired GIL and
+        // reach this method
+        assert globalInterpreterLock.getHoldCount() == 1 || (env.getContext().isCancelling() && globalInterpreterLock.getHoldCount() == 0) : dumpStackOnAssertionHelper(
+                        "trying to release the GIL with invalid hold count " + globalInterpreterLock.getHoldCount());
+        try {
+            globalInterpreterLock.unlock();
+        } catch (IllegalMonitorStateException ex) {
+            // IllegalMonitorStateException is thrown if the lock is not owned by this thread, see
+            // the comment above why we allow it if the context is cancelling
+            if (!env.getContext().isCancelling()) {
+                throw ex;
+            }
+        }
     }
 
     /**

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/exception/PythonThreadKillException.java

@@ -67,11 +67,6 @@ public final class PythonThreadKillException extends RuntimeException {
     private static final long serialVersionUID = 5323687983726237118L;
     public static final PythonThreadKillException INSTANCE = new PythonThreadKillException();
 
-    /**
-     * Creates an exception thrown to model control flow.
-     *
-     * @since 0.8 or earlier
-     */
     public PythonThreadKillException() {
         /*
          * We use the super constructor that initializes the cause to null. Without that, the cause
@@ -84,8 +79,6 @@ public PythonThreadKillException() {
 
     /**
      * For performance reasons, this exception does not record any stack trace information.
-     *
-     * @since 0.8 or earlier
      */
     @SuppressWarnings("sync-override")
     @Override

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/util/PythonSystemThreadTask.java

@@ -56,8 +56,8 @@
  * {@link PythonThreadKillException}. The Java interrupt should interrupt any blocking operation,
  * which should then poll a safepoint. This wrapper takes care of handling
  * {@link PythonThreadKillException} gracefully. Note that Truffle may also shut down this thread
- * via TLA that throws its own internal cancellation exception, which is handled in Truffle internal
- * code that wraps this Runnable.
+ * via TLA that throws its own internal cancellation exception (which inherits {@code ThreadDeath}
+ * exception), which is handled in Truffle internal code that wraps this Runnable.
  */
 public abstract class PythonSystemThreadTask implements Runnable {
     private final String name;

mx.graalpython/mx_graalpython.py

@@ -1280,7 +1280,7 @@ def graalpython_gate_runner(args, tasks):
             override_path = os.path.join(SUITE.get_mx_output_dir(), 'gradle-properties-override')
             original_props_file = "graalpython/com.oracle.graal.python.test/src/tests/standalone/gradle/gradle-test-project/gradle/wrapper/gradle-wrapper.properties"
             mx.copyfile(original_props_file, override_path)
-            mx_graalpython_gradleproject.patch_distributionUrl(override_path, original_props_file)
+            mx_graalpython_gradleproject.patch_distribution_url(override_path, original_props_file)
             env['GRADLE_PROPERTIES_OVERRIDE'] = override_path
 
             env["org.graalvm.maven.downloader.version"] = version
@@ -1315,7 +1315,7 @@ def graalpython_gate_runner(args, tasks):
             override_path = os.path.join(SUITE.get_mx_output_dir(), 'maven-properties-override')
             original_props_file = "graalpython/com.oracle.graal.python.test/src/tests/standalone/mvnw/.mvn/wrapper/maven-wrapper.properties"
             mx.copyfile(original_props_file, override_path)
-            mx_graalpython_gradleproject.patch_distributionUrl(override_path, original_props_file, escape_colon=False)
+            mx_graalpython_gradleproject.patch_distribution_url(override_path, original_props_file, escape_colon=False)
             env['MAVEN_PROPERTIES_OVERRIDE'] = override_path
 
             env["org.graalvm.maven.downloader.version"] = version

mx.graalpython/mx_graalpython_gradleproject.py

@@ -52,7 +52,7 @@ def gradle_wrapper_properties(project_root):
     return os.path.join(project_root, "gradle", "wrapper", "gradle-wrapper.properties")
 
 
-def patch_distributionUrl(properties_file, original_file=None, escape_colon=True):
+def patch_distribution_url(properties_file, original_file=None, escape_colon=True):
     def escape(s):
         return s.replace(':', '\\:') if escape_colon else s
 
@@ -301,8 +301,8 @@ def _create_build_script(self, version: str | None = None):
         """
         # Gradle wrapper
         shutil.copytree(os.path.join(self.subject.gradle_directory, "wrapper"), self.subject.get_output_root(), dirs_exist_ok=True)
-        patch_distributionUrl(gradle_wrapper_properties(self.subject.get_output_root()),
-                             original_file=gradle_wrapper_properties(os.path.join(self.subject.gradle_directory, "wrapper")))
+        patch_distribution_url(gradle_wrapper_properties(self.subject.get_output_root()),
+                               original_file=gradle_wrapper_properties(os.path.join(self.subject.gradle_directory, "wrapper")))
 
         # build.gradle
         deps_decls = []