]> Kevux Git Server - controller/commitdiff
Bugfix: Program deadlocks on entry error.
authorKevin Day <Kevin@kevux.org>
Thu, 13 Nov 2025 03:05:26 +0000 (21:05 -0600)
committerKevin Day <Kevin@kevux.org>
Thu, 13 Nov 2025 03:05:26 +0000 (21:05 -0600)
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.

12 files changed:
data/build/stand_alone/config.h
sources/c/program/controller/init/signal.c
sources/c/program/controller/init/signal.h
sources/c/program/controller/main/common/type.h
sources/c/program/controller/main/common/type/instance.c
sources/c/program/controller/main/common/type/thread.h
sources/c/program/controller/main/print/debug/perform/pid.c
sources/c/program/controller/main/process.c
sources/c/program/controller/main/thread/entry.c
sources/c/program/controller/main/thread/instance.c
sources/c/program/controller/main/thread/signal.c
sources/c/program/controller/main/thread/signal.h

index 4c111c2382a6184ac273f5d190765bcbc6c311c4..e45e8700fd83cc42b224dad0cb1988fb38b7e81c 100644 (file)
 #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_
index b37f74be59667ef59f874eafc47cf18a17ba33ec..e51dcee3bd58d3d03f3baed0424a587f745cb94f 100644 (file)
@@ -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_
 
index 9ac91d40d7e5c24c21bb107dc88b9641028e7eee..9c5598f1e39497a59e80f14ca1e8826b314da0bf 100644 (file)
@@ -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
index 1799e175dc50c570a8c01bb4fd768afb356f9e5a..10b0c140e5ae8aab534b3773a4c336a866fcd2f9 100644 (file)
@@ -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 \
index bebfbdd582643d4ca82a8ddb1df8601213099b89..cc8889b9fd23a57d213672aea35cd9bbb4fc9695 100644 (file)
@@ -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);
 
index 0f11527668fac560888982202ae986fab26092db..9a0462dccc942290a61bb07da4304214a9945033 100644 (file)
@@ -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
index 1bfce5545a805618bc0f66d4846d6912f93465df..cab1d25305a24b470cc4fb512572f82d50b8c0b8 100644 (file)
@@ -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);
index 97d5664730895cc5de0211a35998cbf3b89a8148..639fd5e7b79e9399fca4ac74403dc6c310a69dd5 100644 (file)
@@ -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)) {
 
index 3a459cddf77a36467cd9cca7c94bf62fd6543e10..9e4ef5842c038ef822199c6d157cf095e0c1db53 100644 (file)
@@ -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);
index e377ac7b96130a77bc56190132508ce64524266a..24f0dff3047473b6c47ce358af35544827be0f53 100644 (file)
@@ -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;
       }
index 631fab58ab6247138262418e45b4e807d570a768..7ab56139871331a26fbb49428bb501804c089461 100644 (file)
@@ -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;
index 222705e28bc02637a1ba8c123a8a6b155f257c36..f99c7b7c68227425df7540bda2e232eb5a7ff37c 100644 (file)
  * @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.