From 5c8373bec3cef21a49ed3f0cfd21a3cee8f0c7eb Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Mon, 26 Nov 2012 20:18:25 +0100 Subject: [PATCH] Cleanup destruction of RPC entrypoints This patch introduces clean synchronization between the entrypoint thread and the caller of the 'Rpc_entrypoint' destructor. The most important change is the handling of the 'Ipc_server' destruction. This object is in the local scope of the server's entry function. However, since the server loop used to be an infinite loop, there was hardly any chance to destruct the object in a clean way. Hence, the 'Rpc_entrypoint' destructor used to explicitly call '~Ipc_server'. Unfortunately, this approach led to problems because there are indeed rare cases where the server thread leaves the scope of the entry function, namely uncaught exceptions. In such a case, the destructor would have been called twice. With the new protocol, we make sure to leave the scope of the entry function and thereby destroy the 'Ipc_server' object as expected. This is achieved by propagating the exit condition through a local RPC call to the entrypoint. This way, the blocking state of the entrypoint becomes unblocked. Furthermore, '~Rpc_entrypoint' makes use of the new 'join' function to wait for the completion of the server thread. --- base-foc/src/base/server/server.cc | 11 ++++++++++- base/include/base/rpc_server.h | 28 +++++++++++++++++++++++----- base/src/base/server/common.cc | 19 ++++++++++++++++++- base/src/base/server/server.cc | 10 +++++++++- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/base-foc/src/base/server/server.cc b/base-foc/src/base/server/server.cc index c4c5ad956e..7a75da0986 100644 --- a/base-foc/src/base/server/server.cc +++ b/base-foc/src/base/server/server.cc @@ -55,7 +55,8 @@ void Rpc_entrypoint::entry() */ _delay_start.lock(); - while (1) { + while (!_exit_handler.exit) { + int opcode = 0; srv >> IPC_REPLY_WAIT >> opcode; @@ -87,4 +88,12 @@ void Rpc_entrypoint::entry() _curr_obj->unlock(); _curr_obj = 0; } + + /* answer exit call, thereby wake up '~Rpc_entrypoint' */ + srv << IPC_REPLY; + + /* defer the destruction of 'Ipc_server' until '~Rpc_entrypoint' is ready */ + _delay_exit.lock(); + + } diff --git a/base/include/base/rpc_server.h b/base/include/base/rpc_server.h index 6369b7efff..37a72cdcfc 100644 --- a/base/include/base/rpc_server.h +++ b/base/include/base/rpc_server.h @@ -270,14 +270,32 @@ namespace Genode { */ static void _activation_entry(); + struct Exit + { + GENODE_RPC(Rpc_exit, void, _exit); + GENODE_RPC_INTERFACE(Rpc_exit); + }; + + struct Exit_handler : Rpc_object + { + int exit; + + Exit_handler() : exit(false) { } + + void _exit() { exit = true; } + }; + protected: Ipc_server *_ipc_server; - Rpc_object_base *_curr_obj; /* currently dispatched RPC object */ - Lock _curr_obj_lock; /* for the protection of '_curr_obj' */ - Lock _cap_valid; /* thread startup synchronization */ - Lock _delay_start; /* delay start of request dispatching */ - Cap_session *_cap_session; /* for creating capabilities */ + Rpc_object_base *_curr_obj; /* currently dispatched RPC object */ + Lock _curr_obj_lock; /* for the protection of '_curr_obj' */ + Lock _cap_valid; /* thread startup synchronization */ + Lock _delay_start; /* delay start of request dispatching */ + Lock _delay_exit; /* delay destructor until server settled */ + Cap_session *_cap_session; /* for creating capabilities */ + Exit_handler _exit_handler; + Capability _exit_cap; /** * Back-end function to associate RPC object with the entry point diff --git a/base/src/base/server/common.cc b/base/src/base/server/common.cc index c7e52f2fa7..11e17bd6bf 100644 --- a/base/src/base/server/common.cc +++ b/base/src/base/server/common.cc @@ -12,6 +12,7 @@ */ #include +#include #include using namespace Genode; @@ -96,6 +97,7 @@ Rpc_entrypoint::Rpc_entrypoint(Cap_session *cap_session, size_t stack_size, Thread_base(name, stack_size), _cap(Untyped_capability()), _curr_obj(0), _cap_valid(Lock::LOCKED), _delay_start(Lock::LOCKED), + _delay_exit(Lock::LOCKED), _cap_session(cap_session) { Thread_base::start(); @@ -103,6 +105,8 @@ Rpc_entrypoint::Rpc_entrypoint(Cap_session *cap_session, size_t stack_size, if (start_on_construction) activate(); + + _exit_cap = manage(&_exit_handler); } @@ -110,6 +114,11 @@ Rpc_entrypoint::~Rpc_entrypoint() { typedef Object_pool Pool; + /* leave server loop */ + _exit_cap.call(); + + dissolve(&_exit_handler); + if (Pool::first()) { PWRN("Object pool not empty in %s", __func__); @@ -118,5 +127,13 @@ Rpc_entrypoint::~Rpc_entrypoint() _dissolve(obj); } - _ipc_server->~Ipc_server(); + /* + * Now that we finished the 'dissolve' steps above (which need a working + * 'Ipc_server' in the context of the entrypoint thread), we can allow the + * entrypoint thread to leave the scope. Thereby, the 'Ipc_server' object + * will get destructed. + */ + _delay_exit.unlock(); + + join(); } diff --git a/base/src/base/server/server.cc b/base/src/base/server/server.cc index afc4723d4a..a246f4bc85 100644 --- a/base/src/base/server/server.cc +++ b/base/src/base/server/server.cc @@ -17,6 +17,7 @@ /* Genode includes */ #include +#include using namespace Genode; @@ -55,7 +56,8 @@ void Rpc_entrypoint::entry() */ _delay_start.lock(); - while (1) { + while (!_exit_handler.exit) { + int opcode = 0; srv >> IPC_REPLY_WAIT >> opcode; @@ -81,4 +83,10 @@ void Rpc_entrypoint::entry() _curr_obj->unlock(); _curr_obj = 0; } + + /* answer exit call, thereby wake up '~Rpc_entrypoint' */ + srv << IPC_REPLY; + + /* defer the destruction of 'Ipc_server' until '~Rpc_entrypoint' is ready */ + _delay_exit.lock(); }