[gjs/wip/3v1n0/toggle-queue-tests: 7/15] toggle: Enforce thread-safety using a per-thread spin-lock

[Philip Chimento <pchimento src gnome org>]

commit 02df4431c90aad7d073b8e5f2fd023539d1b4442
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Thu Apr 29 04:04:19 2021 +0200

    toggle: Enforce thread-safety using a per-thread spin-lock
    
    While the ToggleQueue is expected to be thread-safe, unfortunately the
    way we use isn't always true.
    In fact, while we always perform operations locking the shared data, we
    may act on it depending on results that may have been changed by another
    thread. For example, we check if an object is queued or we cancel a
    queued object and we assume that this is true for the rest of the
    execution, but this may have been already changed meanwhile, thus we
    error later.
    
    This happens repeatedly with applications doing many fast threaded
    operations (an example can be triggered via GFileMonitor when the fs is
    particularly active as bug #297 shows).
    
    To avoid this, we need to ensure that all the access to the queue are
    locked till we're not done with it, but at the same time we must not to
    stop the owner thread to access multiple time to the queue while it owns
    the lock (as it may happen on recursion of we want group various
    operations).
    
    To do this, control public access to the queue only via a structure that
    uses a spin-lock to block non-holder threads and that uses a refcounted
    system to handle the unlocking.
    
    So, any time we get the queue, we have a locked control of it, once the
    ToggleQueue::Locked structure is released, we unlock it only if no other
    instances in the current thread are using it.
    
    This serves us particularly well in the toggle notification callback
    where we had cases in which we were adding duplicate objects to the queue
    even if they were already there or vice versa we were not adding them
    because we the situation changed since the call to is_queued().
    Now instead, we can be sure that each thread will work on the current
    queue state and behave accordingly.
    
    Related-to: #297

 gi/object.cpp | 40 +++++++++++++++++++++++-----------------
 gi/toggle.cpp | 53 +++++++++++++++++++++++++++++++++++++----------------
 gi/toggle.h   | 32 +++++++++++++++++++++++++++-----
 3 files changed, 87 insertions(+), 38 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index ca82c3a0..39f400bd 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1145,7 +1145,7 @@ ObjectInstance::gobj_dispose_notify(void)
     if (m_uses_toggle_ref) {
         g_object_ref(m_ptr.get());
         g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this);
-        ToggleQueue::get_default().cancel(this);
+        ToggleQueue::get_default()->cancel(this);
         wrapped_gobj_toggle_notify(this, m_ptr, TRUE);
         m_uses_toggle_ref = false;
     }
@@ -1336,9 +1336,9 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
      */
     is_main_thread = gjs->is_owner_thread();
 
-    auto& toggle_queue = ToggleQueue::get_default();
+    auto toggle_queue = ToggleQueue::get_default();
     std::tie(toggle_down_queued, toggle_up_queued) =
-        toggle_queue.is_queued(self);
+        toggle_queue->is_queued(self);
 
     if (is_last_ref) {
         /* We've transitions from 2 -> 1 references,
@@ -1352,10 +1352,9 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
                     "toggle down",
                     gobj, G_OBJECT_TYPE_NAME(gobj));
             }
-
             self->toggle_down();
         } else if (!toggle_down_queued) {
-            toggle_queue.enqueue(self, ToggleQueue::DOWN, toggle_handler);
+            toggle_queue->enqueue(self, ToggleQueue::DOWN, toggle_handler);
         }
     } else {
         /* We've transitioned from 1 -> 2 references.
@@ -1372,7 +1371,7 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
             }
             self->toggle_up();
         } else if (!toggle_up_queued) {
-            toggle_queue.enqueue(self, ToggleQueue::UP, toggle_handler);
+            toggle_queue->enqueue(self, ToggleQueue::UP, toggle_handler);
         }
     }
 }
@@ -1411,14 +1410,13 @@ ObjectInstance::release_native_object(void)
 void
 gjs_object_clear_toggles(void)
 {
-    ToggleQueue::get_default().handle_all_toggles(toggle_handler);
+    ToggleQueue::get_default()->handle_all_toggles(toggle_handler);
 }
 
 void
 gjs_object_shutdown_toggle_queue(void)
 {
-    auto& toggle_queue = ToggleQueue::get_default();
-    toggle_queue.shutdown();
+    ToggleQueue::get_default()->shutdown();
 }
 
 /*
@@ -1472,6 +1470,10 @@ void ObjectInstance::update_heap_wrapper_weak_pointers(JSContext*,
                         "%zu wrapped GObject(s) to examine",
                         ObjectInstance::num_wrapped_gobjects());
 
+    // Take a lock on the queue till we're done with it, so that we don't
+    // risk that another thread will queue something else while sweeping
+    auto locked_queue = ToggleQueue::get_default();
+
     ObjectInstance::remove_wrapped_gobjects_if(
         std::mem_fn(&ObjectInstance::weak_pointer_was_finalized),
         std::mem_fn(&ObjectInstance::disassociate_js_gobject));
@@ -1483,9 +1485,9 @@ ObjectInstance::weak_pointer_was_finalized(void)
     if (has_wrapper() && !wrapper_is_rooted()) {
         bool toggle_down_queued, toggle_up_queued;
 
-        auto& toggle_queue = ToggleQueue::get_default();
+        auto toggle_queue = ToggleQueue::get_default();
         std::tie(toggle_down_queued, toggle_up_queued) =
-            toggle_queue.is_queued(this);
+            toggle_queue->is_queued(this);
 
         if (!toggle_down_queued && toggle_up_queued)
             return false;
@@ -1494,7 +1496,7 @@ ObjectInstance::weak_pointer_was_finalized(void)
             return false;
 
         if (toggle_down_queued)
-            toggle_queue.cancel(this);
+            toggle_queue->cancel(this);
 
         /* Ouch, the JS object is dead already. Disassociate the
          * GObject and hope the GObject dies too. (Remove it from
@@ -1602,8 +1604,8 @@ ObjectInstance::disassociate_js_gobject(void)
 {
     bool had_toggle_down, had_toggle_up;
 
-    auto& toggle_queue = ToggleQueue::get_default();
-    std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(this);
+    auto locked_queue = ToggleQueue::get_default();
+    std::tie(had_toggle_down, had_toggle_up) = locked_queue->cancel(this);
     if (had_toggle_up && !had_toggle_down) {
         g_error(
             "JS object wrapper for GObject %p (%s) is being released while "
@@ -1777,10 +1779,14 @@ ObjectInstance::~ObjectInstance() {
 
     invalidate_closure_list(&m_closures);
 
+    // Do not keep the queue locked here, as we may want to leave the other
+    // threads to queue toggle events till we're owning the GObject so that
+    // eventually (once the toggle reference is finally removed) we can be
+    // sure that no other toggle event will target this (soon dead) wrapper.
     bool had_toggle_up;
     bool had_toggle_down;
-    auto& toggle_queue = ToggleQueue::get_default();
-    std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(this);
+    std::tie(had_toggle_down, had_toggle_up) =
+        ToggleQueue::get_default()->cancel(this);
 
     /* GObject is not already freed */
     if (m_ptr) {
@@ -1803,7 +1809,7 @@ ObjectInstance::~ObjectInstance() {
         if (was_using_toggle_refs) {
             // We need to cancel again, to be sure that no other thread added
             // another toggle reference before we were removing the last one.
-            toggle_queue.cancel(this);
+            ToggleQueue::get_default()->cancel(this);
         }
     }
 
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index 74c4ea9c..701afbec 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -7,8 +7,8 @@
 // SPDX-FileContributor: Marco Trevisan <marco trevisan canonical com>
 
 #include <algorithm>  // for find_if
+#include <atomic>
 #include <deque>
-#include <mutex>
 #include <utility>  // for pair
 
 #include "gi/object.h"
@@ -23,6 +23,28 @@ inline void debug(const char* did GJS_USED_VERBOSE_LIFECYCLE,
                         object ? object->ptr() : nullptr);
 }
 
+void ToggleQueue::lock() {
+    auto holding_thread = std::thread::id();
+    auto current_thread = std::this_thread::get_id();
+
+    while (!m_holder.compare_exchange_weak(holding_thread, current_thread,
+                                           std::memory_order_acquire)) {
+        // In case the current thread is holding the lock, we can just try
+        // again, checking if this is still true and in case continue
+        if (holding_thread != current_thread)
+            holding_thread = std::thread::id();
+    }
+
+    m_holder_ref_count++;
+}
+
+void ToggleQueue::maybe_unlock() {
+    g_assert(owns_lock() && "Nothing to unlock here");
+
+    if (!(--m_holder_ref_count))
+        m_holder.store(std::thread::id(), std::memory_order_release);
+}
+
 std::deque<ToggleQueue::Item>::iterator ToggleQueue::find_operation_locked(
     const ObjectInstance* obj, ToggleQueue::Direction direction) {
     return std::find_if(
@@ -50,6 +72,7 @@ bool ToggleQueue::find_and_erase_operation_locked(
 }
 
 void ToggleQueue::handle_all_toggles(Handler handler) {
+    g_assert(owns_lock() && "Unsafe access to queue");
     while (handle_toggle(handler))
         ;
 }
@@ -57,7 +80,7 @@ void ToggleQueue::handle_all_toggles(Handler handler) {
 gboolean
 ToggleQueue::idle_handle_toggle(void *data)
 {
-    auto self = static_cast<ToggleQueue *>(data);
+    auto self = Locked(static_cast<ToggleQueue*>(data));
     self->handle_all_toggles(self->m_toggle_handler);
 
     return G_SOURCE_REMOVE;
@@ -66,14 +89,13 @@ ToggleQueue::idle_handle_toggle(void *data)
 void
 ToggleQueue::idle_destroy_notify(void *data)
 {
-    auto self = static_cast<ToggleQueue *>(data);
-    std::lock_guard<std::mutex> hold(self->lock);
+    auto self = Locked(static_cast<ToggleQueue*>(data));
     self->m_idle_id = 0;
     self->m_toggle_handler = nullptr;
 }
 
 std::pair<bool, bool> ToggleQueue::is_queued(ObjectInstance* obj) const {
-    std::lock_guard<std::mutex> hold(lock);
+    g_assert(owns_lock() && "Unsafe access to queue");
     bool has_toggle_down = find_operation_locked(obj, DOWN) != q.end();
     bool has_toggle_up = find_operation_locked(obj, UP) != q.end();
     return {has_toggle_down, has_toggle_up};
@@ -81,7 +103,7 @@ std::pair<bool, bool> ToggleQueue::is_queued(ObjectInstance* obj) const {
 
 std::pair<bool, bool> ToggleQueue::cancel(ObjectInstance* obj) {
     debug("cancel", obj);
-    std::lock_guard<std::mutex> hold(lock);
+    g_assert(owns_lock() && "Unsafe access to queue");
     bool had_toggle_down = find_and_erase_operation_locked(obj, DOWN);
     bool had_toggle_up = find_and_erase_operation_locked(obj, UP);
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p was %s", obj->ptr(),
@@ -94,21 +116,19 @@ std::pair<bool, bool> ToggleQueue::cancel(ObjectInstance* obj) {
 }
 
 bool ToggleQueue::handle_toggle(Handler handler) {
-    Item item;
-    {
-        std::lock_guard<std::mutex> hold(lock);
-        if (q.empty())
-            return false;
-
-        item = q.front();
-        q.pop_front();
-    }
+    g_assert(owns_lock() && "Unsafe access to queue");
 
+    if (q.empty())
+        return false;
+
+    auto const& item = q.front();
     if (item.direction == UP)
         debug("handle UP", item.object);
     else
         debug("handle DOWN", item.object);
+
     handler(item.object, item.direction);
+    q.pop_front();
 
     return true;
 }
@@ -124,6 +144,8 @@ ToggleQueue::shutdown(void)
 
 void ToggleQueue::enqueue(ObjectInstance* obj, ToggleQueue::Direction direction,
                           ToggleQueue::Handler handler) {
+    g_assert(owns_lock() && "Unsafe access to queue");
+
     if (G_UNLIKELY (m_shutdown)) {
         gjs_debug(GJS_DEBUG_GOBJECT,
                   "Enqueuing GObject %p to toggle %s after "
@@ -132,7 +154,6 @@ void ToggleQueue::enqueue(ObjectInstance* obj, ToggleQueue::Direction direction,
         return;
     }
 
-    std::lock_guard<std::mutex> hold(lock);
     /* Only keep an unowned reference on the object here, as if we're here, the
      * JSObject wrapper has already a reference and we don't want to cause
      * any weak notify in case it has lost one already in the main thread.
diff --git a/gi/toggle.h b/gi/toggle.h
index c470df67..df38f551 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -1,8 +1,10 @@
 /* -*- mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
 // SPDX-License-Identifier: MIT OR LGPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017 Endless Mobile, Inc.
+// SPDX-FileCopyrightText: 2021 Canonical Ltd.
 // SPDX-FileContributor: Authored by: Philip Chimento <philip endlessm com>
 // SPDX-FileContributor: Philip Chimento <philip chimento gmail com>
+// SPDX-FileContributor: Marco Trevisan <marco trevisan canonical com>
 
 #ifndef GI_TOGGLE_H_
 #define GI_TOGGLE_H_
@@ -11,7 +13,7 @@
 
 #include <atomic>
 #include <deque>
-#include <mutex>
+#include <thread>
 #include <utility>  // for pair
 
 class ObjectInstance;
@@ -36,12 +38,28 @@ public:
         ToggleQueue::Direction direction;
     };
 
-    mutable std::mutex lock;
+    struct Locked {
+        explicit Locked(ToggleQueue* queue) { queue->lock(); }
+        ~Locked() { get_default_unlocked().maybe_unlock(); }
+        ToggleQueue* operator->() { return &get_default_unlocked(); }
+    };
+
     std::deque<Item> q;
     std::atomic_bool m_shutdown = ATOMIC_VAR_INIT(false);
 
     unsigned m_idle_id = 0;
     Handler m_toggle_handler = nullptr;
+    std::atomic<std::thread::id> m_holder = std::thread::id();
+    unsigned m_holder_ref_count = 0;
+
+    void lock();
+    void maybe_unlock();
+    [[nodiscard]] bool is_locked() const {
+        return m_holder != std::thread::id();
+    }
+    [[nodiscard]] bool owns_lock() const {
+        return m_holder == std::this_thread::get_id();
+    }
 
     [[nodiscard]] std::deque<Item>::iterator find_operation_locked(
         const ObjectInstance* obj, Direction direction);
@@ -55,6 +73,11 @@ public:
     static gboolean idle_handle_toggle(void *data);
     static void idle_destroy_notify(void *data);
 
+    [[nodiscard]] static ToggleQueue& get_default_unlocked() {
+        static ToggleQueue the_singleton;
+        return the_singleton;
+    }
+
  public:
     /* These two functions return a pair DOWN, UP signifying whether toggles
      * are / were queued. is_queued() just checks and does not modify. */
@@ -76,9 +99,8 @@ public:
     /* Queues a toggle to be processed in idle time. */
     void enqueue(ObjectInstance* obj, Direction direction, Handler handler);
 
-    [[nodiscard]] static ToggleQueue& get_default() {
-        static ToggleQueue the_singleton;
-        return the_singleton;
+    [[nodiscard]] static Locked get_default() {
+        return Locked(&get_default_unlocked());
     }
 };