From 34ecec957c606a97b3aff805619952776437c5da Mon Sep 17 00:00:00 2001
From: Sam Gross <colesbury@gmail.com>
Date: Tue, 3 Dec 2019 16:41:13 -0500
Subject: [PATCH] Atomically delete object queue only when empty

Once the threads object queue is empty, other threads may explicitly
merge reference counts immediately. It's no longer safe for the original
thread to do RC operations because of races with accesses to ob_ref_local.

This ensures that the queue is empty when it's removed from the hashtable
mapping thread-id to queue. There are still other RC operations that can
happen during thread destruction, so the call to _Py_queue_destroy will
probably need to be moved.

See #50
---
 Python/pyrefcnt.c | 51 +++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/Python/pyrefcnt.c b/Python/pyrefcnt.c
index fc60e098be7..4963155dcc4 100644
--- a/Python/pyrefcnt.c
+++ b/Python/pyrefcnt.c
@@ -22,23 +22,6 @@ _Py_RefcntQueue_Push(PyThreadState *tstate, PyObject *ob)
     }
 }
 
-// NOTE: ABA problem
-static PyObject *
-_Py_RefcntQueue_Pop(PyThreadState *tstate)
-{
-    PyObject **object_queue = &tstate->object_queue;
-    for (;;) {
-        PyObject *head = _Py_atomic_load_ptr_relaxed(object_queue);
-        if (!head) {
-            return NULL;
-        }
-        PyObject *next = _Py_atomic_load_ptr_relaxed(&head->ob_tid);
-        if (_Py_atomic_compare_exchange_ptr(object_queue, head, next)) {
-            return head;
-        }
-    }
-}
-
 void
 _Py_queue_object(PyObject *ob)
 {
@@ -68,26 +51,25 @@ _Py_queue_object(PyObject *ob)
         return;
     }
 
-    PyThreadState *target_tstate = NULL;
-
     int err;
     if ((err = pthread_rwlock_rdlock(&interp->object_queues_lk)) != 0) {
         Py_FatalError("_Py_queue_object: unable to lock");
         return;
     }
 
+    PyThreadState *target_tstate = NULL;
     _Py_hashtable_entry_t *entry = _Py_HASHTABLE_GET_ENTRY(ht, ob_tid);
     if (entry) {
         _Py_HASHTABLE_ENTRY_READ_DATA(ht, entry, target_tstate);
     }
 
-    pthread_rwlock_unlock(&interp->object_queues_lk);
-
     if (target_tstate) {
         _Py_RefcntQueue_Push(target_tstate, ob);
     }
-    else {
-        // printf("NO queue for %ld\n", ob_tid);
+
+    pthread_rwlock_unlock(&interp->object_queues_lk);
+
+    if (!target_tstate) {
         _Py_ExplicitMergeRefcount(ob);
     }
 
@@ -98,9 +80,11 @@ _Py_queue_process(PyThreadState *tstate)
 {
     assert(tstate);
 
-    PyObject *ob;
-    while ((ob = _Py_RefcntQueue_Pop(tstate)) != NULL)  {
-        _Py_ExplicitMergeRefcount(ob);
+    PyObject *head = _Py_atomic_exchange_ptr(&tstate->object_queue, NULL);
+    while (head) {
+        PyObject *next = (PyObject *)head->ob_tid;
+        _Py_ExplicitMergeRefcount(head);
+        head = next;
     }
 }
 
@@ -131,14 +115,21 @@ _Py_queue_destroy(PyThreadState *tstate)
     PyInterpreterState *interp = tstate->interp;
     assert(interp);
 
+    _Py_hashtable_t *ht = interp->object_queues;
+    uint64_t tid = tstate->fast_thread_id;
+
+retry:
+    _Py_queue_process(tstate);
+
     if (pthread_rwlock_wrlock(&interp->object_queues_lk) != 0) {
         Py_FatalError("_Py_queue_destroy: unable to lock");
         return;
     }
 
-    _Py_hashtable_t *ht = interp->object_queues;
-    uint64_t tid = tstate->fast_thread_id;
-    // printf("destroying queue for %ld\n", tid);
+    if (tstate->object_queue) {
+        pthread_rwlock_unlock(&interp->object_queues_lk);
+        goto retry;
+    }
 
     PyThreadState *value = NULL;
     if (!_Py_HASHTABLE_POP(ht, tid, value)) {
@@ -147,6 +138,4 @@ _Py_queue_destroy(PyThreadState *tstate)
     assert(value == tstate);
 
     pthread_rwlock_unlock(&interp->object_queues_lk);
-
-    _Py_queue_process(tstate);
 }