Dispose Python thread state when native thread is detached

Author: steve-s

graalpython/com.oracle.graal.python.cext/src/pystate.c

@@ -177,21 +177,18 @@ PyGILState_Release(PyGILState_STATE oldstate)
 {
     if (oldstate == PyGILState_UNLOCKED) {
         GraalPyTruffleGILState_Release();
-        /* In the GraalPyTruffleGILState_Release up-call, we cleaned-up the pointer saved in
-         * Java level Python thread state to avoid setting it to NULL in Truffle thread disposal
-         * code, because it is not clear if the native thread is guaranteed to still be around
-         * when the Truffle thread is being disposed.
-         */
-        tstate_current = NULL;
     }
     if (TRUFFLE_CONTEXT) {
         graalpy_gilstate_counter--;
         if (graalpy_gilstate_counter == 0 && graalpy_attached_thread) {
+            GraalPyTruffleBeforeThreadDetach();
             (*TRUFFLE_CONTEXT)->detachCurrentThread(TRUFFLE_CONTEXT);
             graalpy_attached_thread = 0;
             /*
-             * The thread state on the Java-side can get garbage collected after the thread is detached,
-             * so we need to make sure to fetch a fresh pointer the next time we attach.
+             * The thread state on the Java-side is cleared in GraalPyTruffleBeforeThreadDetach.
+             * As part of that the tstate_current pointer should have been set to NULL to make
+             * sure to fetch a fresh pointer the next time we attach. Just to be sure, we clear
+             * it here too:
              */
             tstate_current = NULL;
         }

graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_thread.py

@@ -87,21 +87,20 @@ def test_register_new_thread(self):
             name="TestThread",
             includes="#include <pthread.h>",
             code=r'''
-            void* thread_entrypoint(void* arg) {
+            void test_attach_detach(PyObject *callable) {
                 // This check is important not just to check that the function works without the thread attached,
                 // but also because the thread attaching logic in it can break the following PyGILState_Ensure call
                 if (PyGILState_Check()) {
                     PyErr_SetString(PyExc_RuntimeError, "Thread shouldn't be holding the GIL at this point");
                     PyErr_WriteUnraisable(NULL);
-                    return NULL;
+                    return;
                 }
-                PyObject* callable = (PyObject*)arg;
                 PyGILState_STATE gstate;
                 gstate = PyGILState_Ensure();
                 if (!PyGILState_Check()) {
                     PyErr_SetString(PyExc_RuntimeError, "GIL not acquired");
                     PyErr_WriteUnraisable(NULL);
-                    return NULL;
+                    return;
                 }
                 if (!PyObject_CallNoArgs(callable)) {
                     PyErr_WriteUnraisable(callable);
@@ -113,8 +112,12 @@ def test_register_new_thread(self):
                 if (PyGILState_Check()) {
                     PyErr_SetString(PyExc_RuntimeError, "GIL not released");
                     PyErr_WriteUnraisable(NULL);
-                    return NULL;
                 }
+            }
+            void* thread_entrypoint(void* arg) {
+                test_attach_detach((PyObject*)arg);
+                test_attach_detach((PyObject*)arg);
+                test_attach_detach((PyObject*)arg);
                 return NULL;
             }
             PyObject* run_in_thread(PyObject* self, PyObject* callable) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/PythonLanguage.java

@@ -1028,7 +1028,7 @@ protected void initializeThread(PythonContext context, Thread thread) {
 
     @Override
     protected void disposeThread(PythonContext context, Thread thread) {
-        context.disposeThread(thread);
+        context.disposeThread(thread, false);
     }
 
     public Shape getEmptyShape() {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextPyStateBuiltins.java

@@ -50,7 +50,6 @@
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Void;
 
-import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiBuiltin;
 import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiNullaryBuiltinNode;
 import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiUnaryBuiltinNode;
@@ -103,10 +102,6 @@ abstract static class PyTruffleGILState_Release extends CApiNullaryBuiltinNode {
         static Object restore(
                         @Cached GilNode gil) {
             gil.release(true);
-            // The C API that makes this up-call should reset the thread local variable to NULL
-            // and here, on the Java side, we can clean up the pointers, because there is no need
-            // to reset the native thread local on Truffle thread disposal
-            getContext(gil).getThreadState(PythonLanguage.get(gil)).resetNativeThreadLocalVarPointer();
             return PNone.NO_VALUE;
         }
     }
@@ -119,12 +114,22 @@ Object get(Object tstateCurrentPtr,
                         @CachedLibrary("tstateCurrentPtr") InteropLibrary lib) {
             PythonThreadState pythonThreadState = getContext().getThreadState(getLanguage());
             if (!lib.isNull(tstateCurrentPtr)) {
-                pythonThreadState.setNativeThreadLocalVarPointer(lib, tstateCurrentPtr);
+                pythonThreadState.setNativeThreadLocalVarPointer(tstateCurrentPtr);
             }
             return PThreadState.getOrCreateNativeThreadState(pythonThreadState);
         }
     }
 
+    @CApiBuiltin(ret = Void, args = {}, call = Ignored)
+    abstract static class PyTruffleBeforeThreadDetach extends CApiNullaryBuiltinNode {
+        @Specialization
+        @TruffleBoundary
+        Object doIt() {
+            getContext().disposeThread(Thread.currentThread(), true);
+            return PNone.NO_VALUE;
+        }
+    }
+
     @CApiBuiltin(ret = PyObjectBorrowed, args = {}, call = Direct)
     abstract static class PyThreadState_GetDict extends CApiNullaryBuiltinNode {
 

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

@@ -65,7 +65,6 @@
 import static com.oracle.graal.python.nodes.StringLiterals.T_WARNINGS;
 import static com.oracle.graal.python.nodes.truffle.TruffleStringMigrationHelpers.isJavaString;
 import static com.oracle.graal.python.util.PythonUtils.TS_ENCODING;
-import static com.oracle.graal.python.util.PythonUtils.initUnsafe;
 import static com.oracle.graal.python.util.PythonUtils.toTruffleStringUncached;
 import static com.oracle.graal.python.util.PythonUtils.tsLiteral;
 
@@ -352,12 +351,6 @@ public static final class PythonThreadState {
          */
         Object nativeThreadLocalVarPointer;
 
-        /**
-         * Raw pointer to {@link #nativeThreadLocalVarPointer} if available, so that we can clean up
-         * during thread disposal.
-         */
-        long nativeThreadLocalVarRawPointer;
-
         /* The global tracing function, set by sys.settrace and returned by sys.gettrace. */
         Object traceFun;
 
@@ -537,7 +530,7 @@ public void setContextVarsContext(PContextVarsContext contextVarsContext) {
             this.contextVarsContext = contextVarsContext;
         }
 
-        public void dispose(PythonContext context) {
+        public void dispose(PythonContext context, boolean canRunGuestCode) {
             // This method may be called twice on the same object.
 
             /*
@@ -558,14 +551,11 @@ public void dispose(PythonContext context) {
             }
             /*
              * Write 'NULL' to the native thread-local variable used to store the PyThreadState
-             * struct such that it cannot accidentally be reused. We can invoke LLVM only if we are
-             * not shutting down the thread.
+             * struct such that it cannot accidentally be reused. Since this is done as a
+             * precaution, we just skip this if we cannot run guest code, because it may invoke
+             * LLVM.
              */
-            if (nativeThreadLocalVarRawPointer != 0 && context.env.isNativeAccessAllowed()) {
-                context.getUnsafe().putAddress(nativeThreadLocalVarRawPointer, 0);
-                nativeThreadLocalVarRawPointer = 0;
-                nativeThreadLocalVarPointer = null;
-            } else if (nativeThreadLocalVarPointer != null && !isShuttingDown()) {
+            if (nativeThreadLocalVarPointer != null && canRunGuestCode) {
                 CStructAccess.WritePointerNode.writeUncached(nativeThreadLocalVarPointer, 0, context.getNativeNull());
                 nativeThreadLocalVarPointer = null;
             }
@@ -636,22 +626,10 @@ public void setAsyncgenFirstIter(Object asyncgenFirstIter) {
             this.asyncgenFirstIter = asyncgenFirstIter;
         }
 
-        public void resetNativeThreadLocalVarPointer() {
-            nativeThreadLocalVarRawPointer = 0;
-            nativeThreadLocalVarPointer = null;
-        }
-
-        public void setNativeThreadLocalVarPointer(InteropLibrary interop, Object ptr) {
+        public void setNativeThreadLocalVarPointer(Object ptr) {
             // either unset or same
             assert nativeThreadLocalVarPointer == null || nativeThreadLocalVarPointer == ptr ||
                             InteropLibrary.getUncached().isIdentical(nativeThreadLocalVarPointer, ptr, InteropLibrary.getUncached());
-            if (interop.isPointer(ptr)) {
-                try {
-                    this.nativeThreadLocalVarRawPointer = interop.asPointer(ptr);
-                } catch (UnsupportedMessageException e) {
-                    throw CompilerDirectives.shouldNotReachHere(e);
-                }
-            }
             this.nativeThreadLocalVarPointer = ptr;
         }
     }
@@ -2191,7 +2169,7 @@ public void runShutdownHooks() {
     @TruffleBoundary
     private void disposeThreadStates() {
         for (PythonThreadState ts : threadStateMapping.values()) {
-            ts.dispose(this);
+            ts.dispose(this, true);
         }
         threadStateMapping.clear();
     }
@@ -2270,7 +2248,7 @@ private void joinPythonThreads() {
                     // they are still running some GraalPython code, if they are embedder threads
                     // that are not running GraalPython code anymore, they will just never receive
                     // PythonThreadKillException and continue as if nothing happened.
-                    disposeThread(thread);
+                    disposeThread(thread, true);
                     boolean isOurThread = runViaLauncher || thread.getThreadGroup() == threadGroup;
                     // Do not try so hard when running in embedded mode and the thread may not be
                     // running any GraalPython code anymore
@@ -2364,6 +2342,11 @@ public void killSystemThread(Thread thread) {
         env.submitThreadLocal(new Thread[]{thread}, new ThreadLocalAction(true, false) {
             @Override
             protected void perform(ThreadLocalAction.Access access) {
+                // just in case the thread holds GIL
+                PythonContext ctx = PythonContext.get(null);
+                if (ctx.ownsGil()) {
+                    ctx.releaseGil();
+                }
                 throw new PythonThreadKillException();
             }
         });
@@ -2630,7 +2613,7 @@ public synchronized void attachThread(Thread thread, ContextThreadLocal<PythonTh
         threadStateMapping.put(thread, threadState.get(thread));
     }
 
-    public synchronized void disposeThread(Thread thread) {
+    public synchronized void disposeThread(Thread thread, boolean canRunGuestCode) {
         CompilerAsserts.neverPartOfCompilation();
         // check if there is a live sentinel lock
         PythonThreadState ts = threadStateMapping.get(thread);
@@ -2640,7 +2623,7 @@ public synchronized void disposeThread(Thread thread) {
         }
         ts.shutdown();
         threadStateMapping.remove(thread);
-        ts.dispose(this);
+        ts.dispose(this, canRunGuestCode);
         releaseSentinelLock(ts.sentinelLock);
         getSharedMultiprocessingData().removeChildContextThread(PThread.getThreadId(thread));
     }