From 03de9224fc45fdccf772b975a63851284a4579b3 Mon Sep 17 00:00:00 2001 From: Romain Vimont Date: Fri, 12 Nov 2021 09:49:37 +0100 Subject: [PATCH] Introduce process observer Add a tool to easily observe process termination. This allows to move this complexity out of the server code. --- app/src/server.c | 71 +++++++++++++------------------------- app/src/server.h | 8 ++--- app/src/util/process.c | 78 ++++++++++++++++++++++++++++++++++++++++++ app/src/util/process.h | 67 ++++++++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 53 deletions(-) diff --git a/app/src/server.c b/app/src/server.c index 326d0e79..f108c7cc 100644 --- a/app/src/server.c +++ b/app/src/server.c @@ -329,19 +329,6 @@ server_init(struct server *server) { server->serial = NULL; server->process = SC_PROCESS_NONE; - bool ok = sc_mutex_init(&server->mutex); - if (!ok) { - return false; - } - - ok = sc_cond_init(&server->process_terminated_cond); - if (!ok) { - sc_mutex_destroy(&server->mutex); - return false; - } - - server->process_terminated = false; - server->server_socket = SC_INVALID_SOCKET; server->video_socket = SC_INVALID_SOCKET; server->control_socket = SC_INVALID_SOCKET; @@ -354,25 +341,20 @@ server_init(struct server *server) { return true; } -static int -run_wait_server(void *data) { - struct server *server = data; - sc_process_wait(server->process, false); // ignore exit code +static void +server_on_terminated(void *userdata) { + struct server *server = userdata; - sc_mutex_lock(&server->mutex); - server->process_terminated = true; - sc_cond_signal(&server->process_terminated_cond); - sc_mutex_unlock(&server->mutex); - - // no need for synchronization, server_socket is initialized before this - // thread was created + // No need for synchronization, server_socket is initialized before the + // observer thread is created. if (server->server_socket != SC_INVALID_SOCKET) { - // Unblock any accept() + // If the server process dies before connecting to the server socket, + // then the client will be stuck forever on accept(). To avoid the + // problem, wake up the accept() call when the server dies. net_interrupt(server->server_socket); } LOGD("Server terminated"); - return 0; } bool @@ -400,14 +382,11 @@ server_start(struct server *server, const struct server_params *params) { goto error; } - // If the server process dies before connecting to the server socket, then - // the client will be stuck forever on accept(). To avoid the problem, we - // must be able to wake up the accept() call when the server dies. To keep - // things simple and multiplatform, just spawn a new thread waiting for the - // server process and calling shutdown()/close() on the server socket if - // necessary to wake up any accept() blocking call. - bool ok = sc_thread_create(&server->wait_server_thread, run_wait_server, - "wait-server", server); + static const struct sc_process_listener listener = { + .on_terminated = server_on_terminated, + }; + bool ok = sc_process_observer_init(&server->observer, server->process, + &listener, server); if (!ok) { sc_process_terminate(server->process); sc_process_wait(server->process, true); // ignore exit code @@ -516,27 +495,25 @@ server_stop(struct server *server) { } // Give some delay for the server to terminate properly - sc_mutex_lock(&server->mutex); - bool signaled = false; - if (!server->process_terminated) { #define WATCHDOG_DELAY SC_TICK_FROM_SEC(1) - signaled = sc_cond_timedwait(&server->process_terminated_cond, - &server->mutex, - sc_tick_now() + WATCHDOG_DELAY); - } - sc_mutex_unlock(&server->mutex); + sc_tick deadline = sc_tick_now() + WATCHDOG_DELAY; + bool terminated = + sc_process_observer_timedwait(&server->observer, deadline); // After this delay, kill the server if it's not dead already. // On some devices, closing the sockets is not sufficient to wake up the // blocking calls while the device is asleep. - if (!signaled) { - // The process is terminated, but not reaped (closed) yet, so its PID - // is still valid. + if (!terminated) { + // The process may have terminated since the check, but it is not + // reaped (closed) yet, so its PID is still valid, and it is ok to call + // sc_process_terminate() even in that case. LOGW("Killing the server..."); sc_process_terminate(server->process); } - sc_thread_join(&server->wait_server_thread, NULL); + sc_process_observer_join(&server->observer); + sc_process_observer_destroy(&server->observer); + sc_process_close(server->process); } @@ -558,6 +535,4 @@ server_destroy(struct server *server) { } } free(server->serial); - sc_cond_destroy(&server->process_terminated_cond); - sc_mutex_destroy(&server->mutex); } diff --git a/app/src/server.h b/app/src/server.h index 18721cf7..c06b3562 100644 --- a/app/src/server.h +++ b/app/src/server.h @@ -22,12 +22,10 @@ struct server_info { struct server { char *serial; - sc_pid process; - sc_thread wait_server_thread; - sc_mutex mutex; - sc_cond process_terminated_cond; - bool process_terminated; + sc_pid process; + // alive only between start() and stop() + struct sc_process_observer observer; sc_socket server_socket; // only used if !tunnel_forward sc_socket video_socket; diff --git a/app/src/util/process.c b/app/src/util/process.c index 6743438e..28f51edd 100644 --- a/app/src/util/process.c +++ b/app/src/util/process.c @@ -1,5 +1,6 @@ #include "process.h" +#include #include #include "log.h" @@ -41,3 +42,80 @@ sc_pipe_read_all(sc_pipe pipe, char *data, size_t len) { } return copied; } + +static int +run_observer(void *data) { + struct sc_process_observer *observer = data; + sc_process_wait(observer->pid, false); // ignore exit code + + sc_mutex_lock(&observer->mutex); + observer->terminated = true; + sc_cond_signal(&observer->cond_terminated); + sc_mutex_unlock(&observer->mutex); + + if (observer->listener) { + observer->listener->on_terminated(observer->listener_userdata); + } + + return 0; +} + +bool +sc_process_observer_init(struct sc_process_observer *observer, sc_pid pid, + const struct sc_process_listener *listener, + void *listener_userdata) { + // Either no listener, or on_terminated() is defined + assert(!listener || listener->on_terminated); + + bool ok = sc_mutex_init(&observer->mutex); + if (!ok) { + return false; + } + + ok = sc_cond_init(&observer->cond_terminated); + if (!ok) { + sc_mutex_destroy(&observer->mutex); + return false; + } + + observer->pid = pid; + observer->listener = listener; + observer->listener_userdata = listener_userdata; + observer->terminated = false; + + ok = sc_thread_create(&observer->thread, run_observer, "process_observer", + observer); + if (!ok) { + sc_cond_destroy(&observer->cond_terminated); + sc_mutex_destroy(&observer->mutex); + return false; + } + + return true; +} + +bool +sc_process_observer_timedwait(struct sc_process_observer *observer, + sc_tick deadline) { + sc_mutex_lock(&observer->mutex); + bool timed_out = false; + while (!observer->terminated && !timed_out) { + timed_out = !sc_cond_timedwait(&observer->cond_terminated, + &observer->mutex, deadline); + } + bool terminated = observer->terminated; + sc_mutex_unlock(&observer->mutex); + + return terminated; +} + +void +sc_process_observer_join(struct sc_process_observer *observer) { + sc_thread_join(&observer->thread, NULL); +} + +void +sc_process_observer_destroy(struct sc_process_observer *observer) { + sc_cond_destroy(&observer->cond_terminated); + sc_mutex_destroy(&observer->mutex); +} diff --git a/app/src/util/process.h b/app/src/util/process.h index b4980b43..7964be5c 100644 --- a/app/src/util/process.h +++ b/app/src/util/process.h @@ -4,6 +4,7 @@ #include "common.h" #include +#include "util/thread.h" #ifdef _WIN32 @@ -32,6 +33,34 @@ #endif +struct sc_process_listener { + void (*on_terminated)(void *userdata); +}; + +/** + * Tool to observe process termination + * + * To keep things simple and multiplatform, it runs a separate thread to wait + * for process termination (without closing the process to avoid race + * conditions). + * + * It allows a caller to block until the process is terminated (with a + * timeout), and to be notified asynchronously from the observer thread. + * + * The process is not owned by the observer (the observer will never close it). + */ +struct sc_process_observer { + sc_pid pid; + + sc_mutex mutex; + sc_cond cond_terminated; + bool terminated; + + sc_thread thread; + const struct sc_process_listener *listener; + void *listener_userdata; +}; + enum sc_process_result { SC_PROCESS_SUCCESS, SC_PROCESS_ERROR_GENERIC, @@ -106,4 +135,42 @@ sc_pipe_read_all(sc_pipe pipe, char *data, size_t len); void sc_pipe_close(sc_pipe pipe); +/** + * Start observing process + * + * The listener is optional. If set, its callback will be called from the + * observer thread once the process is terminated. + */ +bool +sc_process_observer_init(struct sc_process_observer *observer, sc_pid pid, + const struct sc_process_listener *listener, + void *listener_userdata); + +/** + * Wait for process termination until a deadline + * + * Return true if the process is already terminated. Return false if the + * process terminatation has not been detected yet (however, it may have + * terminated in the meantime). + * + * To wait without timeout/deadline, just use sc_process_wait() instead. + */ +bool +sc_process_observer_timedwait(struct sc_process_observer *observer, + sc_tick deadline); + +/** + * Join the observer thread + */ +void +sc_process_observer_join(struct sc_process_observer *observer); + +/** + * Destroy the observer + * + * This does not close the associated process. + */ +void +sc_process_observer_destroy(struct sc_process_observer *observer); + #endif