From: Kevin Day Date: Thu, 13 Nov 2025 03:05:26 +0000 (-0600) Subject: Bugfix: Program deadlocks on entry error. X-Git-Tag: 0.7.3~11 X-Git-Url: https://www.git.kevux.org/?a=commitdiff_plain;h=1317dd79d7af7ca388f5696a2d08b175fb532862;p=controller Bugfix: Program deadlocks on entry error. The signal design by the standard libc signals and the stand pthread signals is rather lacking. Sending a signal to the thread might cause it to deadlock because the signal handler is trying to exit but has locks and the cancellation function has called `f_thread_join()` on that signal handler thread. First, switch to `f_thread_detach()` rather than join the thread. This allows the signal thread to close gracefully without blocking the cancellation function. A detached signal thread needs to know that once it is detached, it should not handle signals and should exit. This is not straight-forward and pthreads fails to provide an easy way to do this. (Where an easy way would be calling a function that gives the thread state of the current thread.) The main signal ID could be checked for by getting the current thread ID and comparing the main signal ID to this current thread ID. This works, but introduces bad threaded application practices. The signal handler could read the main signal ID at any time before a potential change to it. That is fine, it just missed the check in that case. Still, this should be avoided as there could even be undefined behavior in this. This takes an alternate approach of creating the `f_thread_attribute_t` and passing it to the thread. The thread can then use this value to get the current detached state. This should be much safer. Due to the nature of threads detaching, this design requires that the `f_thread_attribute_t` to be de-allocated by the callback itself. A new structure, called `controller_thread_arguments_t`, for passing this information to the thread is now provided. The `f_thread_signal_write()` call handles certain libc functions but in other cases `f_thread_cancel()` should be called. Add calls to `f_thread_cancel()` for any threads that have `f_thread_signal_write()` passed to it. This helps ensure that both cancellation cases can be triggered. These changes cause the interrupt behavior to be slightly different than previous operations. I still plan on reviewing and determining how I want to make signal handling operate across the board for controller and init programs and for both entries and exits. This behavior is therefore considered acceptable and provides a good chance to test out alternative approaches in practice rather than just in theory. --- diff --git a/data/build/stand_alone/config.h b/data/build/stand_alone/config.h index 4c111c2..e45e870 100644 --- a/data/build/stand_alone/config.h +++ b/data/build/stand_alone/config.h @@ -1506,11 +1506,11 @@ #define _di_f_thread_attribute_affinity_set_ #define _di_f_thread_attribute_concurrency_get_ #define _di_f_thread_attribute_concurrency_set_ -#define _di_f_thread_attribute_create_ +//#define _di_f_thread_attribute_create_ #define _di_f_thread_attribute_default_get_ #define _di_f_thread_attribute_default_set_ -#define _di_f_thread_attribute_delete_ -#define _di_f_thread_attribute_detach_get_ +//#define _di_f_thread_attribute_delete_ +//#define _di_f_thread_attribute_detach_get_ #define _di_f_thread_attribute_detach_set_ #define _di_f_thread_attribute_guard_get_ #define _di_f_thread_attribute_guard_set_ diff --git a/sources/c/program/controller/init/signal.c b/sources/c/program/controller/init/signal.c index b37f74b..e51dcee 100644 --- a/sources/c/program/controller/init/signal.c +++ b/sources/c/program/controller/init/signal.c @@ -5,17 +5,23 @@ extern "C" { #endif #ifndef _di_controller_init_signal_thread_ - void controller_init_signal_thread(controller_t * const main, const uint8_t is_normal) { + void controller_init_signal_thread(controller_t * const main, const uint8_t is_normal, f_thread_attribute_t * const attribute) { - if (!main) return; + if (!main || !attribute) return; if (!controller_thread_enable_is_normal(&main->thread, is_normal)) return; if (!(main->setting.flag & controller_main_flag_interruptible_d)) return; siginfo_t information; f_time_spec_t time = f_time_spec_t_initialize; + int state = 0; while (controller_thread_enable_is_normal(&main->thread, is_normal)) { + state = 0; + + f_thread_attribute_detach_get(*attribute, &state); + if (state == PTHREAD_CREATE_DETACHED) break; + memset((void *) &information, 0, sizeof(siginfo_t)); controller_time_now(controller_thread_timeout_exit_ready_seconds_d, controller_thread_timeout_exit_ready_nanoseconds_d, &time); @@ -28,6 +34,11 @@ extern "C" { // Prevent thread from being interrupted and auto-cancelled. f_thread_cancel_state_set(PTHREAD_CANCEL_DEFERRED, 0); + state = 0; + + f_thread_attribute_detach_get(*attribute, &state); + if (state == PTHREAD_CREATE_DETACHED) break; + // Send F_signal_reserved_35 to cleanup process to reap child processes. if (information.si_signo == F_signal_child || information.si_signo == F_signal_reserved_35) { f_thread_condition_signal_all(&main->thread.lock.reap_condition); @@ -45,6 +56,8 @@ extern "C" { break; } } // while + + f_thread_attribute_delete(attribute); } #endif // _di_controller_init_signal_thread_ diff --git a/sources/c/program/controller/init/signal.h b/sources/c/program/controller/init/signal.h index 9ac91d4..9c5598f 100644 --- a/sources/c/program/controller/init/signal.h +++ b/sources/c/program/controller/init/signal.h @@ -26,9 +26,15 @@ extern "C" { * @param is_normal * If TRUE, then process as if this operates during a normal operation (Entry and Control). * If FALSE, then process as if this operates during an Exit operation. + * @param attribute + * The thread signal attribute. + * + * This must be de-allocated by this function before exit. + * + * Must not be NULL. */ #ifndef _di_controller_init_signal_thread_ - extern void controller_init_signal_thread(controller_t * const main, const uint8_t is_normal); + extern void controller_init_signal_thread(controller_t * const main, const uint8_t is_normal, f_thread_attribute_t * const attribute); #endif // _di_controller_init_signal_thread_ #ifdef __cplusplus diff --git a/sources/c/program/controller/main/common/type.h b/sources/c/program/controller/main/common/type.h index 1799e17..10b0c14 100644 --- a/sources/c/program/controller/main/common/type.h +++ b/sources/c/program/controller/main/common/type.h @@ -60,7 +60,7 @@ extern "C" { f_status_t (*print_message_help)(fl_print_t * const print); f_status_t (*process_entry_setup)(controller_t * const main, controller_entry_t * const entry); - void (*process_thread_signal)(controller_t * const main, const uint8_t is_normal); + void (*process_thread_signal)(controller_t * const main, const uint8_t is_normal, f_thread_attribute_t * const attribute); } controller_callback_t; #define controller_callback_t_initialize \ diff --git a/sources/c/program/controller/main/common/type/instance.c b/sources/c/program/controller/main/common/type/instance.c index bebfbdd..cc8889b 100644 --- a/sources/c/program/controller/main/common/type/instance.c +++ b/sources/c/program/controller/main/common/type/instance.c @@ -10,6 +10,7 @@ extern "C" { if (!instance) return; if (instance->thread) { + f_thread_cancel(instance->thread); f_thread_signal_write(instance->thread, F_signal_kill); f_thread_join(instance->thread, 0); diff --git a/sources/c/program/controller/main/common/type/thread.h b/sources/c/program/controller/main/common/type/thread.h index 0f11527..9a0462d 100644 --- a/sources/c/program/controller/main/common/type/thread.h +++ b/sources/c/program/controller/main/common/type/thread.h @@ -71,6 +71,34 @@ extern "C" { #endif // _di_controller_thread_t_ /** + * A helper structure used to pass special data to a thread. + * + * The thread is expected to expand this into function parameters as needed. + * + * This values must not be de-allocated or unassigned by the caller. + * + * The functions should handle their arguments as appropriate (such as id_signal). + * + * Properties: + * - main: The main program data (must not be NULL). + * - attribute: The attributes of the thread. + */ +#ifndef _di_controller_thread_arguments_t_ + typedef struct { + controller_t * const main; + f_thread_attribute_t * const attribute; + } controller_thread_arguments_t; + + // This really should not be used under normal circumstances because these values are constants. + #define controller_thread_arguments_t_initialize { \ + 0, \ + 0, \ + } + + #define macro_controller_thread_arguments_t_initialize_1(main, attribute) { main, attribute } +#endif // _di_controller_thread_arguments_t_ + +/** * Fully de-allocate all memory for the given setting without caring about return status. * * @param thread diff --git a/sources/c/program/controller/main/print/debug/perform/pid.c b/sources/c/program/controller/main/print/debug/perform/pid.c index 1bfce55..cab1d25 100644 --- a/sources/c/program/controller/main/print/debug/perform/pid.c +++ b/sources/c/program/controller/main/print/debug/perform/pid.c @@ -37,8 +37,6 @@ extern "C" { controller_t * const main = (controller_t *) print->custom; - controller_lock_print(print->to, &main->thread); - if (F_status_set_fine(status) == F_read_only) { fl_print_format("%r%[%QThe pid file '%]", print->to, f_string_eol_s, print->context, print->prefix, print->context); fl_print_format(f_string_format_Q_single_s.string, print->to, print->notable, main->process.path_pid, print->notable); diff --git a/sources/c/program/controller/main/process.c b/sources/c/program/controller/main/process.c index 97d5664..639fd5e 100644 --- a/sources/c/program/controller/main/process.c +++ b/sources/c/program/controller/main/process.c @@ -54,7 +54,7 @@ extern "C" { controller_print_error_status(&main->program.error, F_status_debug_source_d, status); } else { - status = f_thread_create(0, &main->thread.id_signal, &controller_thread_signal_normal, (void *) main); + status = controller_thread_signal_create(main, &controller_thread_signal_normal); if (F_status_is_error_not(status)) { diff --git a/sources/c/program/controller/main/thread/entry.c b/sources/c/program/controller/main/thread/entry.c index 3a459cd..9e4ef58 100644 --- a/sources/c/program/controller/main/thread/entry.c +++ b/sources/c/program/controller/main/thread/entry.c @@ -54,9 +54,7 @@ extern "C" { controller_thread_enable_set(&main->thread, controller_thread_enable_e, &original_enabled); // Restart the signal main->thread to allow for signals while operating the failsafe Items. - if (!main->thread.id_signal) { - f_thread_create(0, &main->thread.id_signal, &controller_thread_signal_normal, (void *) main); - } + controller_thread_signal_create(main, &controller_thread_signal_normal); const f_status_t status_failsafe = controller_entry_process(main, F_true, F_true); @@ -150,9 +148,7 @@ extern "C" { controller_thread_enable_set(&main->thread, controller_thread_enable_exit_e, &original_enabled); // Restart the signal thread to allow for signals while operating the failsafe Items. - if (!main->thread.id_signal) { - f_thread_create(0, &main->thread.id_signal, &controller_thread_signal_other, (void *) main); - } + controller_thread_signal_create(main, &controller_thread_signal_other); } const f_status_t status_failsafe = controller_entry_process(main, F_false, F_true); diff --git a/sources/c/program/controller/main/thread/instance.c b/sources/c/program/controller/main/thread/instance.c index e377ac7..24f0dff 100644 --- a/sources/c/program/controller/main/thread/instance.c +++ b/sources/c/program/controller/main/thread/instance.c @@ -120,8 +120,9 @@ extern "C" { // The sigtimedwait() function that is run inside of signal must be interrupted directly via f_thread_signal_write(). if (by != controller_thread_cancel_signal_e && main->thread.id_signal) { + f_thread_cancel(main->thread.id_signal); f_thread_signal_write(main->thread.id_signal, F_signal_termination); - f_thread_join(main->thread.id_signal, 0); + f_thread_detach(main->thread.id_signal); main->thread.id_signal = 0; } @@ -415,7 +416,6 @@ extern "C" { } // for f_thread_mutex_unlock(&main->thread.lock.cancel.mutex); - } #endif // _di_controller_thread_instance_cancel_ @@ -435,9 +435,7 @@ extern "C" { } // Restart the signal thread to allow for signals while operating the Exit. - if (!main->thread.id_signal) { - f_thread_create(0, &main->thread.id_signal, &controller_thread_signal_other, (void *) main); - } + controller_thread_signal_create(main, &controller_thread_signal_other); // Restart the cleanup thread to allow for reaping zombies while operating the Exit. if (!main->thread.id_cleanup) { @@ -473,8 +471,9 @@ extern "C" { // The sigtimedwait() function that is run inside of signal must be interrupted directly via f_thread_signal_write(). if (main->thread.id_signal) { + f_thread_cancel(main->thread.id_signal); f_thread_signal_write(main->thread.id_signal, F_signal_termination); - f_thread_join(main->thread.id_signal, 0); + f_thread_detach(main->thread.id_signal); main->thread.id_signal = 0; } diff --git a/sources/c/program/controller/main/thread/signal.c b/sources/c/program/controller/main/thread/signal.c index 631fab5..7ab5613 100644 --- a/sources/c/program/controller/main/thread/signal.c +++ b/sources/c/program/controller/main/thread/signal.c @@ -5,17 +5,23 @@ extern "C" { #endif #ifndef _di_controller_thread_signal_ - void controller_thread_signal(controller_t * const main, const uint8_t is_normal) { + void controller_thread_signal(controller_t * const main, const uint8_t is_normal, f_thread_attribute_t * const attribute) { - if (!main) return; + if (!main || !attribute) return; if (!controller_thread_enable_is_normal(&main->thread, is_normal)) return; if (!(main->setting.flag & controller_main_flag_interruptible_d)) return; siginfo_t information; f_time_spec_t time = f_time_spec_t_initialize; + int state = 0; while (controller_thread_enable_is_normal(&main->thread, is_normal)) { + state = 0; + + f_thread_attribute_detach_get(*attribute, &state); + if (state == PTHREAD_CREATE_DETACHED) break; + memset((void *) &information, 0, sizeof(siginfo_t)); controller_time_now(controller_thread_timeout_exit_ready_seconds_d, controller_thread_timeout_exit_ready_nanoseconds_d, &time); @@ -28,6 +34,11 @@ extern "C" { // Prevent thread from being interrupted and auto-cancelled. f_thread_cancel_state_set(PTHREAD_CANCEL_DEFERRED, 0); + state = 0; + + f_thread_attribute_detach_get(*attribute, &state); + if (state == PTHREAD_CREATE_DETACHED) break; + // Send F_signal_reserved_35 to cleanup process to reap child processes. if (information.si_signo == F_signal_child || information.si_signo == F_signal_reserved_35) { f_thread_condition_signal_all(&main->thread.lock.reap_condition); @@ -45,9 +56,32 @@ extern "C" { break; } } // while + + f_thread_attribute_delete(attribute); } #endif // _di_controller_thread_signal_ +#ifndef _di_controller_thread_signal_create_ + f_status_t controller_thread_signal_create(controller_t * const main, const f_void_pointer_call_t routine) { + + if (!routine) return F_status_set_error(F_parameter); + if (!main->thread.id_signal) return F_none; + + f_thread_attribute_t attribute = f_thread_attribute_t_initialize; + + { + const f_status_t status = f_thread_attribute_create(&attribute); + if (F_status_is_error(status)) return status; + } + + controller_thread_arguments_t arguments = macro_controller_thread_arguments_t_initialize_1(main, &attribute); + + f_thread_create(arguments.attribute, &main->thread.id_signal, routine, (void *) &arguments); + + return F_okay; + } +#endif // _di_controller_thread_signal_create_ + #ifndef _di_controller_thread_signal_get_ int controller_thread_signal_get(controller_thread_t * const thread) { @@ -167,8 +201,12 @@ extern "C" { f_thread_cancel_state_set(PTHREAD_CANCEL_DEFERRED, 0); f_thread_name_set(f_thread_caller(), controller_thread_signal_s); - if (argument && ((controller_t *) argument)->callback.process_thread_signal) { - ((controller_t *) argument)->callback.process_thread_signal((controller_t *) argument, F_true); + if (argument) { + controller_thread_arguments_t * const arguments = (controller_thread_arguments_t *) argument; + + if (arguments->main && arguments->main->callback.process_thread_signal) { + arguments->main->callback.process_thread_signal(arguments->main, F_true, arguments->attribute); + } } return 0; @@ -181,8 +219,12 @@ extern "C" { f_thread_cancel_state_set(PTHREAD_CANCEL_DEFERRED, 0); f_thread_name_set(f_thread_caller(), controller_thread_signal_s); - if (argument && ((controller_t *) argument)->callback.process_thread_signal) { - ((controller_t *) argument)->callback.process_thread_signal((controller_t *) argument, F_false); + if (argument) { + controller_thread_arguments_t * const arguments = (controller_thread_arguments_t *) argument; + + if (arguments->main && arguments->main->callback.process_thread_signal) { + arguments->main->callback.process_thread_signal(arguments->main, F_false, arguments->attribute); + } } return 0; diff --git a/sources/c/program/controller/main/thread/signal.h b/sources/c/program/controller/main/thread/signal.h index 222705e..f99c7b7 100644 --- a/sources/c/program/controller/main/thread/signal.h +++ b/sources/c/program/controller/main/thread/signal.h @@ -22,12 +22,46 @@ * @param is_normal * If TRUE, then process as if this operates during a normal operation (Entry and Control). * If FALSE, then process as if this operates during an Exit operation. + * @param attribute + * The thread signal attribute. + * + * This must be de-allocated by this function before exit. + * + * Must not be NULL. */ #ifndef _di_controller_thread_signal_ - extern void controller_thread_signal(controller_t * const main, const uint8_t is_normal); + extern void controller_thread_signal(controller_t * const main, const uint8_t is_normal, f_thread_attribute_t * const attribute); #endif // _di_controller_thread_signal_ /** + * Create a new signal thread, if one doesn't already exist. + * + * This allocates the thread attribute and passes the pointer to the called function to use and handle deallocation. + * + * @param main + * The main program data. + * + * Must not be NULL. + * @param routine + * The function to execute. + * + * Must not be NULL. + * + * @return + * F_okay on success. + * + * F_parameter (with error bit) if a parmeter is invalid. + * + * Errors (with error bit) from: controller_rule_expand_iki(). + * + * @see f_thread_attribute_create() + * @see f_thread_lock_write_timed() + */ +#ifndef _di_controller_thread_signal_create_ + extern f_status_t controller_thread_signal_create(controller_t * const main, const f_void_pointer_call_t routine); +#endif // _di_controller_thread_signal_create_ + +/** * Get the signal state of the thread in a thread-safe manner. * * This infinitely tries to obtain the read lock.