Respect the GIL.

Author: ericsnowcurrently

Include/cpython/pystate.h

@@ -112,6 +112,10 @@ struct _py_trashcan {
     PyObject *delete_later;
 };
 
+// We can't include pythread.h yet (due to include cycles)
+// so we duplicate PyThread_type_lock here.
+typedef void *PyThread_type_lock;
+
 struct _ts {
     /* See Python/ceval.c for comments explaining most fields */
 
@@ -188,30 +192,49 @@ struct _ts {
 
     struct _py_trashcan trash;
 
-    /* Called when a thread state is deleted normally, but not when it
-     * is destroyed after fork().
-     * Pain:  to prevent rare but fatal shutdown errors (issue 18808),
-     * Thread.join() must wait for the join'ed thread's tstate to be unlinked
-     * from the tstate chain.  That happens at the end of a thread's life,
-     * in pystate.c.
-     * The obvious way doesn't quite work:  create a lock which the tstate
-     * unlinking code releases, and have Thread.join() wait to acquire that
-     * lock.  The problem is that we _are_ at the end of the thread's life:
-     * if the thread holds the last reference to the lock, decref'ing the
-     * lock will delete the lock, and that may trigger arbitrary Python code
-     * if there's a weakref, with a callback, to the lock.  But by this time
-     * _PyRuntime.gilstate.tstate_current is already NULL, so only the simplest
-     * of C code can be allowed to run (in particular it must not be possible to
-     * release the GIL).
-     * So instead of holding the lock directly, the tstate holds a weakref to
-     * the lock:  that's the value of on_delete_data below.  Decref'ing a
-     * weakref is harmless.
-     * on_delete points to _threadmodule.c's static release_sentinel() function.
-     * After the tstate is unlinked, release_sentinel is called with the
-     * weakref-to-lock (on_delete_data) argument, and release_sentinel releases
-     * the indirectly held lock.
-     */
-    int (*on_delete)(void *);
+    struct {
+        /* Called when a thread state is deleted normally, but not when
+         * it is destroyed after fork().
+         *
+         * Pain:  to prevent rare but fatal shutdown errors (issue 18808),
+         * Thread.join() must wait for the join'ed thread's tstate to be
+         * unlinked from the tstate chain.  That happens at the end of a
+         * thread's life, in pystate.c.
+         *
+         * The obvious way doesn't quite work:  create a lock which the
+         * tstate unlinking code releases, and have Thread.join() wait
+         * to acquire that lock.  The problem is that we _are_ at the end
+         * of the thread's life:  if the thread holds the last reference
+         * to the lock, decref'ing the lock will delete the lock, and
+         * that may trigger arbitrary Python code if there's a weakref,
+         * with a callback, to the lock.  But by this time the current
+         * tstate is already NULL, so only the simplest of C code can be
+         * allowed to run (in particular it must not be possible to
+         * release the GIL).
+         *
+         * So instead of holding the lock directly, the tstate holds
+         * a weakref to the lock:  that's the value of .lock_weakref
+         * below.  Decref'ing a weakref is harmless.
+         *
+         * .pre_delete points to the thread_prepare_delete static function
+         * in _threadmodule.c.  It gets called right _before_ the tstate
+         * is deleted, with the GIL held, and returns the lock to release.
+         * It also adds a pending call to delete the lock once the lock
+         * has been released.  This works because the pending call won't
+         * run until another thread of the interpreter takes the GIL.
+         *
+         * It is important that the lock be released _after_ the GIL
+         * is released to avoid a race with threading._shutdown().
+         */
+        // XXX Does all of the above explanation still hold?
+        PyThread_type_lock (*pre_delete)(PyThreadState *);
+        PyObject *lock_weakref;
+    } _threading_thread;
+    /* These weren't ever meant to be used except internally,
+     * but we're keeping them around just in case.  Internally,
+     * we use the _threading_thread field. */
+    /* XXX Drop them! */
+    void (*on_delete)(void *);
     void *on_delete_data;
 
     int coroutine_origin_tracking_depth;

Modules/_threadmodule.c

@@ -1307,57 +1307,136 @@ yet finished.\n\
 This function is meant for internal and specialized purposes only.\n\
 In most applications `threading.enumerate()` should be used instead.");
 
+
+/* Here we're essentinally cleaning up after a thread that finished
+   and has aleady been deallocated (both the threading.Thread and
+   the tstate).  Thus it will run in a different thread with the
+   same interpreter (via a "pending call"). */
 static int
-release_sentinel(void *wr_raw)
+clean_up_sentinel(void *data)
 {
-    PyObject *wr = _PyObject_CAST(wr_raw);
-    /* Tricky: this function is called when the current thread state
-       is being deleted.  Therefore, only simple C code can safely
-       execute here. */
-    PyObject *obj = PyWeakref_GET_OBJECT(wr);
-    lockobject *lock;
-    if (obj != Py_None) {
-        lock = (lockobject *) obj;
-        if (lock->locked) {
-            PyThread_release_lock(lock->lock_lock);
-            lock->locked = 0;
+    PyObject *lockobj = (PyObject *)data;
+    assert(lockobj != NULL);
+    PyThread_type_lock lock = ((lockobject *)lockobj)->lock_lock;
+
+    /* Wait here until we know for sure that he thread running
+       _PyThreadState_DeleteCurrent() has released the lock. */
+    if (acquire_timed(lock, 0) == PY_LOCK_ACQUIRED) {
+        /* _PyThreadState_DeleteCurrent() finished, so we can proceed. */
+        PyThread_release_lock(lock);
+        ((lockobject *)lockobj)->locked = 0;
+    }
+    else if (((lockobject *)lockobj)->locked == 2) {
+        /* _PyThreadState_DeleteCurrent() is still holding
+           the lock, so we will wait here until it is released.
+           We don't need to hold the GIL while we wait though. */
+        ((lockobject *)lockobj)->locked = 1;
+
+        PyLockStatus r = acquire_timed(lock, -1);
+        // XXX Why do we have to loop?
+        while (r == PY_LOCK_FAILURE) {
+            r = acquire_timed(lock, -1);
         }
+        PyThread_release_lock(lock);
+        ((lockobject *)lockobj)->locked = 0;
     }
+    /* Otherwise the current thread acquired the lock right before
+       its eval loop was interrupted to run this pending call.
+       We can simply let it proceed. */
+
+    /* In all cases, at this point we are done with the lock. */
+    Py_DECREF(lockobj);
+    return 0;
+}
+
+static PyThread_type_lock
+thread_prepare_delete(PyThreadState *tstate)
+{
+    assert(tstate->_threading_thread.pre_delete != NULL);
+    PyThread_type_lock lock = NULL;
+
+    /* Tricky: this function is called when the current thread state
+       is being deleted.  Therefore, only simple C code can safely
+       execute here.  The GIL is still held. */
+
+    PyObject *wr = tstate->_threading_thread.lock_weakref;
+    assert(wr != NULL);
+    PyObject *lockobj = PyWeakref_GET_OBJECT(wr);
+    if (lockobj == Py_None) {
+        /* The thread has already been destroyed, so we can clean up now. */
+        goto done;
+    }
+    if (_PyThreadState_GET() != tstate) {
+        assert(PyThread_get_thread_ident() != tstate->thread_id);
+        /* It must be a daemon thread that was killed during
+         * interp/runtime finalization, so there's nothing to do. */
+        goto done;
+    }
+    assert(((lockobject *)lockobj)->locked == 1);
+    assert(acquire_timed(((lockobject *)lockobj)->lock_lock, 0) == PY_LOCK_FAILURE);
+    /* We cheat a little here to allow clean_up_sentinel() to know
+       that this thread is still holding the lock.  The value will be
+       reset to the normal 0 or 1 as soon as any other thread
+       uses the lock. */
+    ((lockobject *)lockobj)->locked = 2;
+
+    /* We need to prevent the underlying PyThread_type_lock from getting
+       destroyed before we release it in _PyThreadState_DeleteCurrent(),
+       However, we don't need the weakref any more. */
+    Py_INCREF(lockobj);
+
+    /* The pending call will be run the next time the GIL is taken
+       by one of this interpreter's threads. */
+    void *data = (void *) lockobj;
+    if (Py_AddPendingCall(clean_up_sentinel, data) < 0) {
+        Py_DECREF(lockobj);
+        /* We otherwise ignore the error.  A non-zero value means
+           there were too many pending calls already queued up.
+           This case is unlikely, and, at worst,
+           we'll just leak the lock.
+           */
+        goto done;
+    }
+
+    lock = ((lockobject *)lockobj)->lock_lock;
+
+done:
     /* Deallocating a weakref with a NULL callback only calls
        PyObject_GC_Del(), which can't call any Python code. */
     Py_DECREF(wr);
-    return 0;
+    tstate->_threading_thread.pre_delete = NULL;
+    tstate->_threading_thread.lock_weakref = NULL;
+    return lock;
 }
 
 static PyObject *
 thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored))
 {
-    PyObject *wr;
     PyThreadState *tstate = _PyThreadState_GET();
-    lockobject *lock;
 
-    if (tstate->on_delete_data != NULL) {
+    if (tstate->_threading_thread.lock_weakref != NULL) {
         /* We must support the re-creation of the lock from a
            fork()ed child. */
-        assert(tstate->on_delete == &release_sentinel);
-        wr = (PyObject *) tstate->on_delete_data;
-        tstate->on_delete = NULL;
-        tstate->on_delete_data = NULL;
-        Py_DECREF(wr);
-    }
-    lock = newlockobject(module);
-    if (lock == NULL)
+        assert(tstate->_threading_thread.pre_delete == &thread_prepare_delete);
+        tstate->_threading_thread.pre_delete = NULL;
+        tstate->_threading_thread.lock_weakref = NULL;
+        Py_DECREF(tstate->_threading_thread.lock_weakref);
+    }
+
+    PyObject *lockobj = (PyObject *) newlockobject(module);
+    if (lockobj == NULL) {
         return NULL;
+    }
     /* The lock is owned by whoever called _set_sentinel(), but the weakref
        hangs to the thread state. */
-    wr = PyWeakref_NewRef((PyObject *) lock, NULL);
+    PyObject *wr = PyWeakref_NewRef(lockobj, NULL);
     if (wr == NULL) {
-        Py_DECREF(lock);
+        Py_DECREF(lockobj);
         return NULL;
     }
-    tstate->on_delete_data = (void *) wr;
-    tstate->on_delete = &release_sentinel;
-    return (PyObject *) lock;
+    tstate->_threading_thread.pre_delete = &thread_prepare_delete;
+    tstate->_threading_thread.lock_weakref = wr;
+    return lockobj;
 }
 
 PyDoc_STRVAR(_set_sentinel_doc,

Python/pystate.c

@@ -1490,6 +1490,22 @@ PyThreadState_Clear(PyThreadState *tstate)
 
     Py_CLEAR(tstate->context);
 
+    if (tstate != current_fast_get(&_PyRuntime)) {
+        /* The "current" case is handled in _PyThreadState_DeleteCurrent(). */
+        if (tstate->_threading_thread.pre_delete != NULL) {
+#ifdef NDEBUG
+            (void) tstate->_threading_thread.pre_delete(tstate);
+#else
+            PyThread_type_lock lock;
+            lock = tstate->_threading_thread.pre_delete(tstate);
+            assert(lock == NULL);
+#endif
+        }
+    }
+    if (tstate->on_delete != NULL) {
+        tstate->on_delete(tstate->on_delete_data);
+    }
+
     tstate->_status.cleared = 1;
 
     // XXX Call _PyThreadStateSwap(runtime, NULL) here if "current".
@@ -1501,6 +1517,7 @@ static void
 tstate_delete_common(PyThreadState *tstate)
 {
     assert(tstate->_status.cleared && !tstate->_status.finalized);
+    // XXX assert(tstate->_threading_thread.pre_delete == NULL);
 
     PyInterpreterState *interp = tstate->interp;
     if (interp == NULL) {
@@ -1561,18 +1578,33 @@ void
 _PyThreadState_DeleteCurrent(PyThreadState *tstate)
 {
     _Py_EnsureTstateNotNULL(tstate);
-    /* We ignore the return value.  A non-zero value means there were
-       too many pending calls already queued up.  This case is unlikely,
-       and, at worst, we'll leak on_delete_data.
-       Also, note that the pending call will be run the next time the
-       GIL is taken by one of this interpreter's threads.  So it won't
-       happen until after the _PyEval_ReleaseLock() call below. */
-    (void)_PyEval_AddPendingCall(tstate->interp,
-                                 tstate->on_delete, tstate->on_delete_data);
+
+    PyThread_type_lock lock = NULL;
+    if (tstate->_threading_thread.pre_delete != NULL) {
+        /* This may queue up a pending call that will run in a
+           different thread in the same interpreter.  It will only
+           run _after_ we've released the GIL below.
+
+           lock will be NULL if the threading.Thread has already been
+           destroyed, if there are too many pending calls already queued
+           up, or if _PyThreadState_DeleteCurrent() wasn't called by
+           thread_run() (in _threadmodule.c).
+           */
+        lock = tstate->_threading_thread.pre_delete(tstate);
+    }
 
     tstate_delete_common(tstate);
     current_fast_clear(tstate->interp->runtime);
     _PyEval_ReleaseLock(tstate);
+
+    if (lock != NULL) {
+        /* Notify threading._shutdown() that this thread has been finalized.
+           This must happen *after* the GIL is released,
+           to avoid a race with threading._shutdown()
+           (via wait_for_thread_shutdown() in pylifecycle.c).. */
+        PyThread_release_lock(lock);
+    }
+
     free_threadstate(tstate);
 }
 

Tools/c-analyzer/c_analyzer/analyze.py

@@ -174,7 +174,14 @@ def find_typedecl(decl, typespec, typespecs):
             # If the decl is in a source file then we expect the
             # type to be in the same file or in a header file.
             continue
-        candidates.append(typedecl)
+        for c in candidates:
+            if c.name == typedecl.name and c.data == typedecl.data:
+                # The type was duplicated in another file.
+                assert c.parent == typedecl.parent
+                break
+        else:
+            candidates.append(typedecl)
+
     if not candidates:
         return None
     elif len(candidates) == 1: