From 5d023b4c6075743dd5a18707d37a0123195f393a Mon Sep 17 00:00:00 2001 From: Paul Davis Date: Fri, 21 Apr 2023 12:02:22 -0600 Subject: [PATCH] libpbd: fix an important thinko for cross-thread signal architecture The old code assumed that the thread that created a request buffer for a given signal-emitting thread would be the latter thread, and thus a thread-local pointer to the request buffer could be used. This turns out not to be true: the GUI thread tends to be responsible for constructing the request buffers for pre-registered threads. That mechanism has been replaced by using a RWLock protected map using pthread_t as the key and the request buffer as the value. This allows any thread to create and register the request buffers used between any other pair of threads (because the lookup always uses a pthread_t). The symptoms of this problem were a signal emitted in an audioengine thread that was propagated to the target thread, but when the target thread scans its request buffers for requests, it finds nothing (because it didn't know about the request buffer). In a sense, the signal was successfully delivered to the target thread, but no meaningful work (i.e the signal handler) is performed. --- libs/pbd/event_loop.cc | 2 +- libs/pbd/pbd/abstract_ui.cc | 73 +++++++++++++++++--------------- libs/pbd/pbd/abstract_ui.h | 8 ++-- libs/pbd/pbd/event_loop.h | 2 +- luasession/luasession.cc | 4 +- session_utils/common.cc | 4 +- tools/signal-test/signal-test.cc | 4 +- 7 files changed, 52 insertions(+), 45 deletions(-) diff --git a/libs/pbd/event_loop.cc b/libs/pbd/event_loop.cc index ff9fac4fa1..6bad8f1d97 100644 --- a/libs/pbd/event_loop.cc +++ b/libs/pbd/event_loop.cc @@ -101,7 +101,7 @@ EventLoop::invalidate_request (void* data) if (ir->event_loop) { DEBUG_TRACE (PBD::DEBUG::EventLoop, string_compose ("%1: invalidating request from %2 (%3) @ %4\n", pthread_name(), ir->event_loop, ir->event_loop->event_loop_name(), ir)); - Glib::Threads::Mutex::Lock lm (ir->event_loop->slot_invalidation_mutex()); + Glib::Threads::RWLock::WriterLock lm (ir->event_loop->slot_invalidation_rwlock()); ir->invalidate (); ir->event_loop->trash.push_back(ir); } diff --git a/libs/pbd/pbd/abstract_ui.cc b/libs/pbd/pbd/abstract_ui.cc index 4650fafcbd..a89089fae4 100644 --- a/libs/pbd/pbd/abstract_ui.cc +++ b/libs/pbd/pbd/abstract_ui.cc @@ -68,9 +68,6 @@ cleanup_request_buffer (void* ptr) rb->dead = true; } -template -Glib::Threads::Private::RequestBuffer> AbstractUI::per_thread_request_buffer (cleanup_request_buffer::RequestBuffer>); - template AbstractUI::AbstractUI (const string& name) : BaseUI (name) @@ -88,7 +85,8 @@ AbstractUI::AbstractUI (const string& name) vector tbm = EventLoop::get_request_buffers_for_target_thread (event_loop_name()); { - Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock); + Glib::Threads::RWLock::WriterLock rbml (request_buffer_map_lock); + for (vector::iterator t = tbm.begin(); t != tbm.end(); ++t) { request_buffers[t->emitting_thread] = static_cast (t->request_buffer); } @@ -126,46 +124,54 @@ AbstractUI::register_thread (pthread_t thread_id, string thread_n event loop will catch. */ - RequestBuffer* b = per_thread_request_buffer.get(); - - if (!b) { - - /* create a new request queue/ringbuffer */ - - DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("create new request buffer for %1 in %2\n", thread_name, event_loop_name())); - - b = new RequestBuffer (num_requests); // XXX leaks - /* set this thread's per_thread_request_buffer to this new - queue/ringbuffer. remember that only this thread will - get this queue when it calls per_thread_request_buffer.get() - - the second argument is a function that will be called - when the thread exits, and ensures that the buffer is marked - dead. it will then be deleted during a call to handle_ui_requests() - */ - - per_thread_request_buffer.set (b); - } else { - DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 : %2 is already registered\n", event_loop_name(), thread_name)); - } + RequestBuffer* b = 0; + bool store = false; { + Glib::Threads::RWLock::ReaderLock lm (request_buffer_map_lock); + typename RequestBufferMap::const_iterator ib = request_buffers.find (pthread_self()); + + if (ib == request_buffers.end()) { + /* create a new request queue/ringbuffer */ + DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("create new request buffer for %1 in %2 from %3/%4\n", thread_name, event_loop_name(), pthread_name(), thread_id)); + b = new RequestBuffer (num_requests); // XXX leaks + store = true; + } + } + + if (store) { + /* add the new request queue (ringbuffer) to our map so that we can iterate over it when the time is right. This step is not RT-safe, but is assumed to be called only at thread initialization time, not repeatedly, and so this is of little consequence. */ - Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock); - request_buffers[thread_id] = b; - } + Glib::Threads::RWLock::WriterLock rbml (request_buffer_map_lock); + request_buffers[thread_id] = b; + DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2/%3 registered request buffer-B @ %4 for %5\n", event_loop_name(), pthread_name(), pthread_self(), b, thread_id)); + + } else { + DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 : %2 is already registered\n", event_loop_name(), thread_name)); + } +} + +template typename AbstractUI::RequestBuffer* +AbstractUI::get_per_thread_request_buffer () +{ + Glib::Threads::RWLock::ReaderLock lm (request_buffer_map_lock); + typename RequestBufferMap::iterator ib = request_buffers.find (pthread_self()); + if (ib != request_buffers.end()) { + return ib->second; + } + return 0; } template RequestObject* AbstractUI::get_request (RequestType rt) { - RequestBuffer* rbuf = per_thread_request_buffer.get (); + RequestBuffer* rbuf = get_per_thread_request_buffer (); RequestBufferVector vec; /* see comments in ::register_thread() above for an explanation of @@ -212,7 +218,7 @@ AbstractUI::handle_ui_requests () RequestBufferVector vec; /* check all registered per-thread buffers first */ - Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock); + Glib::Threads::RWLock::ReaderLock rbml (request_buffer_map_lock); /* clean up any dead invalidation records (object was deleted) */ trash.sort(); @@ -409,7 +415,7 @@ AbstractUI::send_request (RequestObject *req) * reader. */ - RequestBuffer* rbuf = per_thread_request_buffer.get (); + RequestBuffer* rbuf = get_per_thread_request_buffer (); if (rbuf != 0) { DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send per-thread request type %3 using ringbuffer @ %4 IR: %5\n", event_loop_name(), pthread_name(), req->type, rbuf, req->invalidation)); @@ -419,7 +425,7 @@ AbstractUI::send_request (RequestObject *req) * single-reader/single-writer semantics */ DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send heap request type %3 IR %4\n", event_loop_name(), pthread_name(), req->type, req->invalidation)); - Glib::Threads::Mutex::Lock lm (request_buffer_map_lock); + Glib::Threads::RWLock::WriterLock lm (request_buffer_map_lock); request_list.push_back (req); } @@ -495,6 +501,5 @@ template void* AbstractUI::request_buffer_factory (uint32_t num_requests) { RequestBuffer* mcr = new RequestBuffer (num_requests); // XXX leaks - per_thread_request_buffer.set (mcr); return mcr; } diff --git a/libs/pbd/pbd/abstract_ui.h b/libs/pbd/pbd/abstract_ui.h index 7c0083459a..c877c3a83c 100644 --- a/libs/pbd/pbd/abstract_ui.h +++ b/libs/pbd/pbd/abstract_ui.h @@ -61,9 +61,9 @@ public: void register_thread (pthread_t, std::string, uint32_t num_requests); bool call_slot (EventLoop::InvalidationRecord*, const boost::function&); - Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; } + Glib::Threads::RWLock& slot_invalidation_rwlock() { return request_buffer_map_lock; } - Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::RWLock request_buffer_map_lock; static void* request_buffer_factory (uint32_t num_requests); @@ -92,7 +92,6 @@ protected: #endif RequestBufferMap request_buffers; - static Glib::Threads::Private per_thread_request_buffer; std::list request_list; @@ -102,6 +101,9 @@ protected: virtual void do_request (RequestObject *) = 0; PBD::ScopedConnection new_thread_connection; + + RequestBuffer* get_per_thread_request_buffer (); + }; #endif /* __pbd_abstract_ui_h__ */ diff --git a/libs/pbd/pbd/event_loop.h b/libs/pbd/pbd/event_loop.h index 58f3e547aa..35e7aeb9e7 100644 --- a/libs/pbd/pbd/event_loop.h +++ b/libs/pbd/pbd/event_loop.h @@ -90,7 +90,7 @@ public: }; virtual bool call_slot (InvalidationRecord*, const boost::function&) = 0; - virtual Glib::Threads::Mutex& slot_invalidation_mutex() = 0; + virtual Glib::Threads::RWLock& slot_invalidation_rwlock() = 0; std::string event_loop_name() const { return _name; } diff --git a/luasession/luasession.cc b/luasession/luasession.cc index 15fb361a7b..cceb2fbc25 100644 --- a/luasession/luasession.cc +++ b/luasession/luasession.cc @@ -127,14 +127,14 @@ public: ; // TODO process Events, if any } - Glib::Threads::Mutex& slot_invalidation_mutex () + Glib::Threads::RWLock& slot_invalidation_rwlock () { return request_buffer_map_lock; } private: Glib::Threads::Thread* run_loop_thread; - Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::RWLock request_buffer_map_lock; }; static MyEventLoop* event_loop = NULL; diff --git a/session_utils/common.cc b/session_utils/common.cc index 6662406352..c481cd4e4f 100644 --- a/session_utils/common.cc +++ b/session_utils/common.cc @@ -103,11 +103,11 @@ class MyEventLoop : public sigc::trackable, public EventLoop return true; } - Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; } + Glib::Threads::RWLock& slot_invalidation_rwlock() { return request_buffer_map_lock; } private: Glib::Threads::Thread* run_loop_thread; - Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::RWLock request_buffer_map_lock; }; static MyEventLoop *event_loop; diff --git a/tools/signal-test/signal-test.cc b/tools/signal-test/signal-test.cc index 307e842203..c328e142cd 100644 --- a/tools/signal-test/signal-test.cc +++ b/tools/signal-test/signal-test.cc @@ -62,14 +62,14 @@ public: ; // process Events, if any } - Glib::Threads::Mutex& slot_invalidation_mutex () + Glib::Threads::RWlock& slot_invalidation_rwlock () { return request_buffer_map_lock; } private: Glib::Threads::Thread* run_loop_thread; - Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::RWLock request_buffer_map_lock; }; struct MyInvalidationRecord : public PBD::EventLoop::InvalidationRecord {