Synchronize signal context destruction

With this patch, the 'Signal_receiver::dissolve()' function does not return
as long as the signal context to be dissolved is still referenced by one
or more 'Signal' objects. This is supposed to delay the destruction of the
signal context while it is still in use.

Fixes #594.

base/include/base/signal.h

@@ -48,43 +48,54 @@ namespace Genode {
 	{
 		private:
 
-			friend class Signal_receiver;
-			friend class Signal_context;
+			struct Data
+			{
+				Signal_context *context;
+				unsigned        num;
 
-			Signal_context *_context;
-			unsigned        _num;
+				/**
+				 * Constructor
+				 *
+				 * \param context  signal context specific for the
+				 *                 signal-receiver capability used for signal
+				 *                 transmission
+				 * \param num      number of signals received from the same
+				 *                 transmitter
+				 */
+				Data(Signal_context *context, unsigned num)
+				: context(context), num(num) { }
+
+				/**
+				 * Default constructor, representing an invalid signal
+				 */
+				Data() : context(0), num(0) { }
+
+			} _data;
 
 			/**
 			 * Constructor
 			 *
-			 * \param context  signal context specific for the signal-receiver
-			 *                 capability used for signal transmission
-			 * \param num      number of signals received from the same transmitter
-			 *
-			 * Signal objects are constructed only by signal receivers.
+			 * Signal objects are constructed by signal receivers only.
 			 */
-			Signal(Signal_context *context, unsigned num)
-			: _context(context), _num(num)
-			{ }
+			Signal(Data data);
+
+			friend class Signal_receiver;
+			friend class Signal_context;
+
+			void _dec_ref_and_unlock();
+			void _inc_ref();
 
 		public:
 
-			/**
-			 * Default constructor, creating an invalid signal
-			 */
-			Signal() : _context(0), _num(0) { }
+			Signal(Signal const &other);
+			Signal &operator=(Signal const &other);
+			~Signal();
 
-			/**
-			 * Return signal context
-			 */
-			Signal_context *context() { return _context; }
-
-			/**
-			 * Return number of signals received from the same transmitter
-			 */
-			unsigned num() const { return _num; }
+			Signal_context *context()       { return _data.context; }
+			unsigned        num()     const { return _data.num; }
 	};
 
+
 	/**
 	 * Signal context
 	 *
@@ -115,9 +126,13 @@ namespace Genode {
 			 */
 			Signal_receiver *_receiver;
 
-			Lock   _lock;          /* protect '_curr_signal'         */
-			Signal _curr_signal;   /* most-currently received signal */
-			bool   _pending;       /* current signal is valid        */
+			Lock         _lock;          /* protect '_curr_signal'         */
+			Signal::Data _curr_signal;   /* most-currently received signal */
+			bool         _pending;       /* current signal is valid        */
+			unsigned int _ref_cnt;       /* number of references to this context */
+			Lock         _destroy_lock;  /* prevent destruction while the
+			                                context is in use */
+
 
 			/**
 			 * Capability assigned to this context after being assocated with
@@ -127,6 +142,7 @@ namespace Genode {
 			 */
 			Signal_context_capability _cap;
 
+			friend class Signal;
 			friend class Signal_receiver;
 			friend class Signal_context_registry;
 
@@ -137,7 +153,7 @@ namespace Genode {
 			 */
 			Signal_context()
 			: _receiver_le(this), _registry_le(this),
-			  _receiver(0), _pending(0) { }
+			  _receiver(0), _pending(0), _ref_cnt(0) { }
 
 			/**
 			 * Destructor
@@ -271,7 +287,7 @@ namespace Genode {
 			/**
 			 * Locally submit signal to the receiver
 			 */
-			void local_submit(Signal signal);
+			void local_submit(Signal::Data signal);
 
 			/**
 			 * Framework-internal signal-dispatcher

base/src/base/signal/signal.cc

@@ -165,6 +165,68 @@ Genode::Signal_context_registry *signal_context_registry()
 }
 
 
+/************
+ ** Signal **
+ ************/
+
+void Signal::_dec_ref_and_unlock()
+{
+	if (_data.context) {
+		Lock::Guard lock_guard(_data.context->_lock);
+		_data.context->_ref_cnt--;
+		if (_data.context->_ref_cnt == 0)
+			_data.context->_destroy_lock.unlock();
+	}
+}
+
+
+void Signal::_inc_ref()
+{
+	if (_data.context) {
+		Lock::Guard lock_guard(_data.context->_lock);
+		_data.context->_ref_cnt++;
+	}
+}
+
+
+Signal::Signal(Signal::Data data) : _data(data)
+{
+	if (_data.context) {
+		_data.context->_ref_cnt = 1;
+		_data.context->_destroy_lock.lock();
+	}
+}
+
+
+Signal::Signal(Signal const &other) : _data(other._data)
+{
+	_inc_ref();
+}
+
+
+Signal::~Signal()
+{
+	_dec_ref_and_unlock();
+}
+
+
+Signal &Signal::operator=(Signal const &other)
+{
+	if ((_data.context == other._data.context)
+	 && (_data.num     == other._data.num))
+		return *this;
+
+	_dec_ref_and_unlock();
+
+	_data.context = other._data.context;
+	_data.num     = other._data.num;
+
+	_inc_ref();
+
+	return *this;
+}
+
+
 /************************
  ** Signal transmitter **
  ************************/
@@ -270,6 +332,8 @@ void Signal_receiver::dissolve(Signal_context *context)
 	Lock::Guard list_lock_guard(_contexts_lock);
 
 	_unsynchronized_dissolve(context);
+
+	Lock::Guard context_destroy_lock_guard(context->_destroy_lock);
 }
 
 
@@ -312,12 +376,12 @@ Signal Signal_receiver::wait_for_signal()
 				continue;
 
 			context->_pending = false;
-			Signal result = context->_curr_signal;
+			Signal::Data result = context->_curr_signal;
 
 			/* invalidate current signal in context */
-			context->_curr_signal = Signal(0, 0);
+			context->_curr_signal = Signal::Data(0, 0);
 
-			if (result.num() == 0)
+			if (result.num == 0)
 				PWRN("returning signal with num == 0");
 
 			/* return last received signal */
@@ -334,21 +398,21 @@ Signal Signal_receiver::wait_for_signal()
 		 * the signal-causing context is absent from the list.
 		 */
 	}
-	return Signal(0, 0); /* unreachable */
+	return Signal::Data(0, 0); /* unreachable */
 }
 
 
-void Signal_receiver::local_submit(Signal ns)
+void Signal_receiver::local_submit(Signal::Data ns)
 {
-	Signal_context *context = ns.context();
+	Signal_context *context = ns.context;
 
 	/*
 	 * Replace current signal of the context by signal with accumulated
 	 * counters. In the common case, the current signal is an invalid
 	 * signal with a counter value of zero.
 	 */
-	unsigned num = context->_curr_signal.num() + ns.num();
-	context->_curr_signal = Signal(context, num);
+	unsigned num = context->_curr_signal.num + ns.num;
+	context->_curr_signal = Signal::Data(context, num);
 
 	/* wake up the receiver if the context becomes pending */
 	if (!context->_pending) {
@@ -372,7 +436,7 @@ void Signal_receiver::dispatch_signals(Signal_source *signal_source)
 		}
 
 		/* construct and locally submit signal object */
-		Signal signal(context, source_signal.num());
+		Signal::Data signal(context, source_signal.num());
 		context->_receiver->local_submit(signal);
 
 		/* free context lock that was taken by 'test_and_lock' */

os/src/test/signal/main.cc

@@ -13,6 +13,7 @@
 
 #include <base/printf.h>
 #include <base/signal.h>
+#include <base/sleep.h>
 #include <base/thread.h>
 #include <timer_session/connection.h>
 #include <cap_session/connection.h>
@@ -456,21 +457,25 @@ static void lazy_receivers_test()
 	transmitter_1.submit();
 	transmitter_2.submit();
 
-	Signal signal = rec_1.wait_for_signal();
-	printf("returned from wait_for_signal for receiver 1\n");
+	{
+		Signal signal = rec_1.wait_for_signal();
+		printf("returned from wait_for_signal for receiver 1\n");
 
-	signal = rec_2.wait_for_signal();
-	printf("returned from wait_for_signal for receiver 2\n");
+		signal = rec_2.wait_for_signal();
+		printf("returned from wait_for_signal for receiver 2\n");
+	}
 
 	printf("submit and receive signals with multiple receivers out of order\n");
 	transmitter_1.submit();
 	transmitter_2.submit();
 
-	signal = rec_2.wait_for_signal();
-	printf("returned from wait_for_signal for receiver 2\n");
+	{
+		Signal signal = rec_2.wait_for_signal();
+		printf("returned from wait_for_signal for receiver 2\n");
 
-	signal = rec_1.wait_for_signal();
-	printf("returned from wait_for_signal for receiver 1\n");
+		signal = rec_1.wait_for_signal();
+		printf("returned from wait_for_signal for receiver 1\n");
+	}
 
 	printf("TEST %d FINISHED\n", test_cnt);
 }
@@ -499,8 +504,10 @@ static void check_context_management()
 	sender->idle();
 
 	/* collect pending signals and dissolve context from receiver */
-	Signal signal = rec->wait_for_signal();
-	printf("got %d signal(s) from %p\n", signal.num(), signal.context());
+	{
+		Signal signal = rec->wait_for_signal();
+		printf("got %d signal(s) from %p\n", signal.num(), signal.context());
+	}
 	rec->dissolve(context);
 
 	/* let sender spin for some time */
@@ -515,6 +522,71 @@ static void check_context_management()
 }
 
 
+/**
+ * Test if 'Signal_receiver::dissolve()' blocks as long as the signal context
+ * is still referenced by one or more 'Signal' objects
+ */
+
+static Lock signal_context_destroyer_lock(Lock::LOCKED);
+static bool signal_context_destroyed = false;
+
+class Signal_context_destroyer : public Thread<4096>
+{
+	private:
+
+		Signal_receiver *_receiver;
+		Signal_context  *_context;
+
+	public:
+
+		Signal_context_destroyer(Signal_receiver *receiver, Signal_context *context)
+		: _receiver(receiver), _context(context) { }
+
+		void entry()
+		{
+			signal_context_destroyer_lock.lock();
+			_receiver->dissolve(_context);
+			signal_context_destroyed = true;
+			destroy(env()->heap(), _context);
+		}
+};
+
+static void synchronized_context_destruction_test()
+{
+	Signal_receiver receiver;
+
+	Signal_context *context = new (env()->heap()) Signal_context;
+
+	Signal_transmitter transmitter(receiver.manage(context));
+	transmitter.submit();
+
+	Signal_context_destroyer signal_context_destroyer(&receiver, context);
+	signal_context_destroyer.start();
+
+	/* The signal context destroyer thread should not be able to destroy the
+	 * signal context during the 'Signal' objects life time. */
+	{
+		Signal signal = receiver.wait_for_signal();
+
+		/* let the signal context destroyer thread try to destroy the signal context */
+		signal_context_destroyer_lock.unlock();
+		timer.msleep(1000);
+
+		Signal signal_copy = signal;
+		Signal signal_copy2 = signal;
+
+		signal_copy = signal_copy2;
+
+		if (signal_context_destroyed) {
+			PERR("signal context destroyed too early");
+			sleep_forever();
+		}
+	}
+
+	signal_context_destroyer.join();
+}
+
+
 /**
  * Main program
  */
@@ -527,6 +599,7 @@ int main(int, char **)
 	stress_test();
 	lazy_receivers_test();
 	check_context_management();
+	synchronized_context_destruction_test();
 
 	printf("--- signalling test finished ---\n");
 	return 0;

ports/src/app/gdb_monitor/signal_handler_thread.cc

@@ -39,10 +39,8 @@ Signal_handler_thread::Signal_handler_thread(Signal_receiver *receiver)
 
 void Signal_handler_thread::entry()
 {
-	Signal s;
-
 	while(1) {
-		s = _signal_receiver->wait_for_signal();
+		Signal s = _signal_receiver->wait_for_signal();
 
 		if (verbose)
 			PDBG("received exception signal");

ports/src/noux/main.cc

@@ -903,13 +903,19 @@ int main(int argc, char **argv)
 	/* handle asynchronous events */
 	while (init_child) {
 
-		Genode::Signal signal = sig_rec.wait_for_signal();
+		/*
+		 * limit the scope of the 'Signal' object, so the signal context may
+		 * get freed by the destruct queue
+		 */
+		{
+			Genode::Signal signal = sig_rec.wait_for_signal();
 
-		Signal_dispatcher_base *dispatcher =
-			static_cast<Signal_dispatcher_base *>(signal.context());
+			Signal_dispatcher_base *dispatcher =
+				static_cast<Signal_dispatcher_base *>(signal.context());
 
-		for (unsigned i = 0; i < signal.num(); i++)
-			dispatcher->dispatch(1);
+			for (unsigned i = 0; i < signal.num(); i++)
+				dispatcher->dispatch(1);
+		}
 
 		destruct_queue.flush();