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

@@ -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);
 	}

libs/pbd/pbd/abstract_ui.cc

@@ -68,9 +68,6 @@ cleanup_request_buffer (void* ptr)
 	rb->dead = true;
 }
 
-template<typename R>
-Glib::Threads::Private<typename AbstractUI<R>::RequestBuffer> AbstractUI<R>::per_thread_request_buffer (cleanup_request_buffer<AbstractUI<R>::RequestBuffer>);
-
 template <typename RequestObject>
 AbstractUI<RequestObject>::AbstractUI (const string& name)
 	: BaseUI (name)
@@ -88,7 +85,8 @@ AbstractUI<RequestObject>::AbstractUI (const string& name)
 	vector<EventLoop::ThreadBufferMapping> 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<EventLoop::ThreadBufferMapping>::iterator t = tbm.begin(); t != tbm.end(); ++t) {
 			request_buffers[t->emitting_thread] = static_cast<RequestBuffer*> (t->request_buffer);
 		}
@@ -126,46 +124,54 @@ AbstractUI<RequestObject>::register_thread (pthread_t thread_id, string thread_n
 	   event loop will catch.
 	*/
 
-	RequestBuffer* b = per_thread_request_buffer.get();
+	RequestBuffer* b = 0;
-
+	bool store = false;
-	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));
-	}
 
 	{
+		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 RequestObject> typename AbstractUI<RequestObject>::RequestBuffer*
+AbstractUI<RequestObject>::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 <typename RequestObject> RequestObject*
 AbstractUI<RequestObject>::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<RequestObject>::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<RequestObject>::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<RequestObject>::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<typename RequestObject> void*
 AbstractUI<RequestObject>::request_buffer_factory (uint32_t num_requests)
 {
 	RequestBuffer*  mcr = new RequestBuffer (num_requests); // XXX leaks
-	per_thread_request_buffer.set (mcr);
 	return mcr;
 }

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<void()>&);
-	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<RequestBuffer> per_thread_request_buffer;
 
 	std::list<RequestObject*> 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__ */

libs/pbd/pbd/event_loop.h

@@ -90,7 +90,7 @@ public:
 	};
 
 	virtual bool call_slot (InvalidationRecord*, const boost::function<void()>&) = 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; }
 

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;

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;

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 {