rework locking (fa07233a, 112fba182)

For now: use a single lock, which should fix all related crashes.
optimize (with less contended partial locks) if this works.

diff --git a/libs/pbd/event_loop.cc b/libs/pbd/event_loop.cc
index d3aaa3c672c125cd47a303e034dff8393f77a5fa..a1b3670a6870964d51389228e5fd86406c020780 100644 (file)
--- a/libs/pbd/event_loop.cc
+++ b/libs/pbd/event_loop.cc
@@ -89,7 +89,6 @@ 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<BaseRequestObject*>::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 152023f1d4dd31eafef1eaff038a793094bde5f3..de480e13bc1184404cf90ff30c2a7e717e91dcdf 100644 (file)
--- a/libs/pbd/pbd/abstract_ui.cc
+++ b/libs/pbd/pbd/abstract_ui.cc
@@ -78,7 +78,7 @@ AbstractUI<RequestObject>::AbstractUI (const string& name)
        vector<EventLoop::ThreadBufferMapping> tbm = EventLoop::get_request_buffers_for_target_thread (event_loop_name());
 
        {
-               Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
+               Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock);
                for (vector<EventLoop::ThreadBufferMapping>::iterator t = tbm.begin(); t != tbm.end(); ++t) {
                        request_buffers[t->emitting_thread] = static_cast<RequestBuffer*> (t->request_buffer);
                }
@@ -135,7 +135,7 @@ AbstractUI<RequestObject>::register_thread (pthread_t thread_id, string thread_n
                   only at thread initialization time, not repeatedly,
                   and so this is of little consequence.
                */
-               Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
+               Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock);
                request_buffers[thread_id] = b;
        }
 
@@ -192,8 +192,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
        RequestBufferVector vec;
 
        /* check all registered per-thread buffers first */
-
-       request_buffer_map_lock.lock ();
+       Glib::Threads::Mutex::Lock rbml (request_buffer_map_lock);
 
        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1 check %2 request buffers for requests\n", event_loop_name(), request_buffers.size()));
 
@@ -221,7 +220,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                        } else {
                                if (vec.buf[0]->valid) {
                                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: valid request, unlocking before calling\n", event_loop_name()));
-                                       request_buffer_map_lock.unlock ();
+                                       rbml.release ();
                                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: valid request, calling ::do_request()\n", event_loop_name()));
                                        do_request (vec.buf[0]);
 
@@ -237,13 +236,18 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                                                vec.buf[0]->the_slot = 0;
                                        }
 
-                                       request_buffer_map_lock.lock ();
+                                       rbml.acquire ();
                                        if (vec.buf[0]->invalidation) {
                                                DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: removing invalidation record for that request\n", event_loop_name()));
-                                               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) {
+                                               Glib::Threads::Mutex::Lock li (vec.buf[0]->invalidation->event_loop->slot_invalidation_mutex(), Glib::Threads::NOT_LOCK);
+                                               if (vec.buf[0]->invalidation->event_loop && vec.buf[0]->invalidation->event_loop != this) {
+                                                       li.acquire ();
+                                               }
+                                               //if (!(*i).second->dead) {
                                                        vec.buf[0]->invalidation->requests.remove (vec.buf[0]);
+                                               //}
+                                               if (vec.buf[0]->invalidation->event_loop && vec.buf[0]->invalidation->event_loop != this) {
+                                                       li.release ();
                                                }
                                        } else {
                                                DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: no invalidation record for that request\n", event_loop_name()));
@@ -258,17 +262,17 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 
        /* clean up any dead request buffers (their thread has exited) */
 
-       Glib::Threads::Mutex::Lock lr (request_invalidation_lock);
+       assert (rbml.locked ());
        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",
                                                                             event_loop_name(), pthread_name(), i->second));
+                       RequestBufferMapIterator tmp = i;
+                       ++tmp;
                        /* remove it from the EventLoop static map of all request buffers */
                        EventLoop::remove_request_buffer_from_map ((*i).second);
                        /* delete it */
                        delete (*i).second;
-                       RequestBufferMapIterator tmp = i;
-                       ++tmp;
                        /* remove it from this thread's list of request buffers */
                        request_buffers.erase (i);
                        i = tmp;
@@ -277,29 +281,13 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                }
        }
 
-       request_buffer_map_lock.unlock ();
-
        /* and now, the generic request buffer. same rules as above apply */
 
-       Glib::Threads::Mutex::Lock lm (request_list_lock);
-
        while (!request_list.empty()) {
+               assert (rbml.locked ());
                RequestObject* req = request_list.front ();
                request_list.pop_front ();
 
-               /* We need to use this lock, because its the one
-                * returned by slot_invalidation_mutex() and protects
-                * against request invalidation.
-                */
-
-               request_buffer_map_lock.lock ();
-               if (!req->valid) {
-                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 handling invalid heap request, type %3, deleting\n", event_loop_name(), pthread_name(), req->type));
-                       delete req;
-                       request_buffer_map_lock.unlock ();
-                       continue;
-               }
-
                /* we're about to execute this request, so its
                 * too late for any invalidation. mark
                 * the request as "done" before we start.
@@ -307,9 +295,10 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 
                if (req->invalidation) {
                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 remove request from its invalidation list\n", event_loop_name(), pthread_name()));
-                       Glib::Threads::Mutex::Lock lm (req->invalidation->event_loop->request_invalidation_mutex(), Glib::Threads::NOT_LOCK);
+
+                       Glib::Threads::Mutex::Lock li (req->invalidation->event_loop->slot_invalidation_mutex(), Glib::Threads::NOT_LOCK);
                        if (req->invalidation->event_loop && req->invalidation->event_loop != this) {
-                               lm.acquire ();
+                               li.acquire ();
                        }
 
                        /* after this call, if the object referenced by the
@@ -329,6 +318,16 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                         * and only provide a safeguard for modifications to the
                         * container itself.
                         */
+                       if (req->invalidation->event_loop && req->invalidation->event_loop != this) {
+                               li.release ();
+                       }
+               }
+
+               if (!req->valid) {
+                       DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 handling invalid heap request, type %3, deleting\n", event_loop_name(), pthread_name(), req->type));
+                       delete req;
+                       //rbml.release ();
+                       continue;
                }
 
                /* at this point, an object involved in a functor could be
@@ -340,14 +339,16 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                 * references to objects to enter into the request queue.
                 */
 
-               request_buffer_map_lock.unlock ();
-
                /* unlock the request lock while we execute the request, so
                 * that we don't needlessly block other threads (note: not RT
                 * threads since they have their own queue) from making requests.
                 */
 
-               lm.release ();
+               /* also the request may destroy the object itself resulting in a direct
+                * path to EventLoop::invalidate_request () from here
+                * which takes the lock */
+
+               rbml.release ();
 
                DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 execute request type %3\n", event_loop_name(), pthread_name(), req->type));
 
@@ -363,8 +364,10 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 
                /* re-acquire the list lock so that we check again */
 
-               lm.acquire();
+               rbml.acquire();
        }
+
+       rbml.release ();
 }
 
 template <typename RequestObject> void
@@ -410,7 +413,7 @@ AbstractUI<RequestObject>::send_request (RequestObject *req)
                           single-reader/single-writer semantics
                        */
                        DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send heap request type %3\n", event_loop_name(), pthread_name(), req->type));
-                       Glib::Threads::Mutex::Lock lm (request_list_lock);
+                       Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
                        request_list.push_back (req);
                }
 
@@ -466,7 +469,7 @@ AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const bo
                 * runs. So there is no race between signal-emissions/call_slot()
                 * and adding new requests after the object has been invalidated.
                 */
-               Glib::Threads::Mutex::Lock lm (request_invalidation_lock);
+               Glib::Threads::Mutex::Lock lm (request_buffer_map_lock);
                invalidation->requests.push_back (req);
                invalidation->event_loop = this;
        }

diff --git a/libs/pbd/pbd/abstract_ui.h b/libs/pbd/pbd/abstract_ui.h
index e23f443c25f8c55c3dda9809f8b6daebbe7c9f84..072de9b4c90d0debf0d2bbb327e3eba47c338a35 100644 (file)
--- a/libs/pbd/pbd/abstract_ui.h
+++ b/libs/pbd/pbd/abstract_ui.h
@@ -54,27 +54,25 @@ class Touchable;
 template<typename RequestObject>
 class ABSTRACT_UI_API AbstractUI : public BaseUI
 {
-  public:
+public:
        AbstractUI (const std::string& name);
        virtual ~AbstractUI() {}
 
        void register_thread (pthread_t, std::string, uint32_t num_requests);
        void call_slot (EventLoop::InvalidationRecord*, const boost::function<void()>&);
        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;
 
        static void* request_buffer_factory (uint32_t num_requests);
 
-  protected:
+protected:
        struct RequestBuffer : public PBD::RingBufferNPT<RequestObject> {
-                bool dead;
-                RequestBuffer (uint32_t size)
-                        : PBD::RingBufferNPT<RequestObject> (size)
-                       , dead (false) {}
-        };
+               bool dead;
+               RequestBuffer (uint32_t size)
+                       : PBD::RingBufferNPT<RequestObject> (size)
+                       , dead (false) {}
+       };
        typedef typename RequestBuffer::rw_vector RequestBufferVector;
 
 #if defined(COMPILER_MINGW) && defined(PTW32_VERSION)
@@ -93,9 +91,8 @@ class ABSTRACT_UI_API AbstractUI : public BaseUI
 #endif
 
        RequestBufferMap request_buffers;
-        static Glib::Threads::Private<RequestBuffer> per_thread_request_buffer;
+       static Glib::Threads::Private<RequestBuffer> per_thread_request_buffer;
 
-       Glib::Threads::Mutex               request_list_lock;
        std::list<RequestObject*> request_list;
 
        RequestObject* get_request (RequestType);

diff --git a/libs/pbd/pbd/event_loop.h b/libs/pbd/pbd/event_loop.h
index 0001692d3dff8305ca60236f665873ce1f90d130..f7765cb79200db5fc434502dea743e044dd06261 100644 (file)
--- a/libs/pbd/pbd/event_loop.h
+++ b/libs/pbd/pbd/event_loop.h
@@ -77,7 +77,6 @@ class LIBPBD_API EventLoop
 
        virtual void call_slot (InvalidationRecord*, const boost::function<void()>&) = 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; }
 

diff --git a/session_utils/common.cc b/session_utils/common.cc
index 61a3a69f65a1dd809ed5c03cb67158d3342886f3..8d1cdec950d8afb39c3b9c8650701b23ef9b6efb 100644 (file)
--- a/session_utils/common.cc
+++ b/session_utils/common.cc
@@ -77,12 +77,10 @@ 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 649346b3f773738c8baac2b6f5476f42a5b266a2..a131bb443f799b5720119ead85a0a42a9e3f75ec 100644 (file)
--- a/tools/luadevel/luasession.cc
+++ b/tools/luadevel/luasession.cc
@@ -109,12 +109,10 @@ 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;