From fa07233a17036bc1cab69d5854b5c768ff762f5b Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Tue, 13 Dec 2016 23:46:55 +0100 Subject: [PATCH] mutex 'er up Some overzealous locking to track down RequestObject related crashes. bc0fa4d689a4 wrongly locked the current event loop's request_invalidation_lock instead of the invalidation's list lock. Also Abstract UI is able to delete requests concurrently with with EventLoop invalidation. e.g. PortManager::PortRegisteredOrUnregistered and GlobalPortMatrixWindow so the lock needs to be exposed. If this solves various issues, mutexes should to be consolidated (request_buffer_map_lock + request_invalidation_lock) and be chosen such that there is as little contention as possible. --- libs/pbd/event_loop.cc | 3 ++- libs/pbd/pbd/abstract_ui.cc | 7 +++++-- libs/pbd/pbd/abstract_ui.h | 3 ++- libs/pbd/pbd/event_loop.h | 5 +++-- session_utils/common.cc | 2 ++ tools/luadevel/luasession.cc | 2 ++ 6 files changed, 16 insertions(+), 6 deletions(-) diff --git a/libs/pbd/event_loop.cc b/libs/pbd/event_loop.cc index ea3f7a46af..fbbf9c83aa 100644 --- a/libs/pbd/event_loop.cc +++ b/libs/pbd/event_loop.cc @@ -60,7 +60,7 @@ EventLoop::set_event_loop_for_thread (EventLoop* loop) void* EventLoop::invalidate_request (void* data) { - InvalidationRecord* ir = (InvalidationRecord*) data; + InvalidationRecord* ir = (InvalidationRecord*) data; /* Some of the requests queued with an EventLoop may involve functors * that make method calls to objects whose lifetime is shorter @@ -88,6 +88,7 @@ EventLoop::invalidate_request (void* data) if (ir->event_loop) { Glib::Threads::Mutex::Lock lm (ir->event_loop->slot_invalidation_mutex()); + Glib::Threads::Mutex::Lock lr (ir->event_loop->request_invalidation_mutex()); for (list::iterator i = ir->requests.begin(); i != ir->requests.end(); ++i) { (*i)->valid = false; (*i)->invalidation = 0; diff --git a/libs/pbd/pbd/abstract_ui.cc b/libs/pbd/pbd/abstract_ui.cc index 594dec34dd..ed358cb7cf 100644 --- a/libs/pbd/pbd/abstract_ui.cc +++ b/libs/pbd/pbd/abstract_ui.cc @@ -240,7 +240,8 @@ AbstractUI::handle_ui_requests () request_buffer_map_lock.lock (); if (vec.buf[0]->invalidation) { DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: removing invalidation record for that request\n", event_loop_name())); - Glib::Threads::Mutex::Lock lm (request_invalidation_lock); + assert (vec.buf[0]->invalidation->event_loop); + Glib::Threads::Mutex::Lock lm (vec.buf[0]->invalidation->event_loop->request_invalidation_mutex()); if (!(*i).second->dead) { vec.buf[0]->invalidation->requests.remove (vec.buf[0]); } @@ -257,6 +258,7 @@ AbstractUI::handle_ui_requests () /* clean up any dead request buffers (their thread has exited) */ + Glib::Threads::Mutex::Lock lr (request_invalidation_lock); for (i = request_buffers.begin(); i != request_buffers.end(); ) { if ((*i).second->dead) { DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 deleting dead per-thread request buffer for %3 @ %4\n", @@ -304,8 +306,9 @@ AbstractUI::handle_ui_requests () */ if (req->invalidation) { - Glib::Threads::Mutex::Lock lm (request_invalidation_lock); DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 remove request from its invalidation list\n", event_loop_name(), pthread_name())); + assert (req->invalidation->event_loop && req->invalidation->event_loop != this); + Glib::Threads::Mutex::Lock lm (req->invalidation->event_loop->request_invalidation_mutex()); /* after this call, if the object referenced by the * invalidation record is deleted, it will no longer diff --git a/libs/pbd/pbd/abstract_ui.h b/libs/pbd/pbd/abstract_ui.h index 278f8a2603..e23f443c25 100644 --- a/libs/pbd/pbd/abstract_ui.h +++ b/libs/pbd/pbd/abstract_ui.h @@ -60,7 +60,8 @@ class ABSTRACT_UI_API AbstractUI : public BaseUI void register_thread (pthread_t, std::string, uint32_t num_requests); void call_slot (EventLoop::InvalidationRecord*, const boost::function&); - Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; } + Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; } + Glib::Threads::Mutex& request_invalidation_mutex() { return request_invalidation_lock; } Glib::Threads::Mutex request_buffer_map_lock; Glib::Threads::Mutex request_invalidation_lock; diff --git a/libs/pbd/pbd/event_loop.h b/libs/pbd/pbd/event_loop.h index b6e07b44de..0001692d3d 100644 --- a/libs/pbd/pbd/event_loop.h +++ b/libs/pbd/pbd/event_loop.h @@ -76,9 +76,10 @@ class LIBPBD_API EventLoop }; virtual void call_slot (InvalidationRecord*, const boost::function&) = 0; - virtual Glib::Threads::Mutex& slot_invalidation_mutex() = 0; + virtual Glib::Threads::Mutex& slot_invalidation_mutex() = 0; + virtual Glib::Threads::Mutex& request_invalidation_mutex() = 0; - std::string event_loop_name() const { return _name; } + std::string event_loop_name() const { return _name; } static EventLoop* get_event_loop_for_thread(); static void set_event_loop_for_thread (EventLoop* ui); diff --git a/session_utils/common.cc b/session_utils/common.cc index 8d1cdec950..61a3a69f65 100644 --- a/session_utils/common.cc +++ b/session_utils/common.cc @@ -77,10 +77,12 @@ class MyEventLoop : public sigc::trackable, public EventLoop } Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; } + Glib::Threads::Mutex& request_invalidation_mutex() { return request_invalidation_lock; } private: Glib::Threads::Thread* run_loop_thread; Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::Mutex request_invalidation_lock; }; static MyEventLoop *event_loop; diff --git a/tools/luadevel/luasession.cc b/tools/luadevel/luasession.cc index a131bb443f..649346b3f7 100644 --- a/tools/luadevel/luasession.cc +++ b/tools/luadevel/luasession.cc @@ -109,10 +109,12 @@ class MyEventLoop : public sigc::trackable, public EventLoop } Glib::Threads::Mutex& slot_invalidation_mutex () { return request_buffer_map_lock; } + Glib::Threads::Mutex& request_invalidation_mutex() { return request_invalidation_lock; } private: Glib::Threads::Thread* run_loop_thread; Glib::Threads::Mutex request_buffer_map_lock; + Glib::Threads::Mutex request_invalidation_lock; }; static MyEventLoop *event_loop = NULL;