From: Kevin Day <Kevin@kevux.org>
Date: Thu, 6 Nov 2025 03:15:33 +0000 (-0600)
Subject: Update: Wrap up changes to locking.
X-Git-Tag: 0.7.3~19
X-Git-Url: https://www.git.kevux.org/?a=commitdiff_plain;h=73cbd71c2fc88f73d7c146ac6832331b660f6c3c;p=controller

Update: Wrap up changes to locking.

I am pretty much done with the locking changes.
I might want to do other changes pending additional review, but this is good enough for now.

Fix issue where a write lock is returning `F_lock_read` on error instead of `F_lock_write` on error.

Properly set write lock on child processes when assigning the `process_child_id`.
The `process_child_id` is a pointer to a memory region protected by locks and therefore needs to be write locked protected before changes are made to it.

Fix case where `controller_lock_read_instance()` is being called but its return value is not being checked for `F_interrupt`.
When `F_interrupt` is received in this (`sources/c/program/controller/main/rule/instance.c:152`), then break out of the loop and properly return interrupt status code.

The clean up thread should only try to lock the cancel mutex.
If a cancel is on going, then do not wait.
This should not wait because the clean up thread itself will be removed by the cancel operation.

Make a copy of the instances to operate and loop over the sets.
This reduces how long the instances lock shall be set.
The related instances lock can now also be a read lock rather than a write lock because no changes are to be made on the structure itself.

Ensure that the `controller_thread_instance_cancel()` function should exclusively wait for the lock rather than be interrupt driven.
This ensures that if the clean up thread is operating, then the instance cancel should wait for it to be finished.

Signal the reap condition before attempting to join the clean up thread to ensure that the reap lock is released, if it is held.
---

diff --git a/sources/c/program/controller/main/rule/execute.c b/sources/c/program/controller/main/rule/execute.c
index 027d9cf..81944c9 100644
--- a/sources/c/program/controller/main/rule/execute.c
+++ b/sources/c/program/controller/main/rule/execute.c
@@ -438,7 +438,7 @@ extern "C" {
     f_execute_result_t result = f_execute_result_t_initialize;
 
     status = controller_lock_write_standard(instance->type != controller_instance_type_exit_e, controller_lock_check_flag_no_d, &main->thread, &instance->use);
-    if (status != F_okay) return F_status_set_error(F_lock_read);
+    if (status != F_okay) return F_status_set_error(F_lock_write);
 
     status = f_memory_array_increase(controller_allocation_small_d, sizeof(pid_t), (void **) &instance->childs.array, &instance->childs.used, &instance->childs.size);
 
@@ -479,13 +479,23 @@ extern "C" {
       const pid_t id_child = result.pid;
       result.status = 0;
 
+      f_status_t status_lock = controller_lock_write_instance(instance, &instance->use);
+
+      if (F_status_is_error(status_lock)) {
+        controller_print_error_lock_critical(&main->program.error, F_status_set_fine(status_lock), F_false, controller_debug_file_line_d);
+
+        return status_lock;
+      }
+
       // Assign the child instance id to allow for the cancel instance to send appropriate termination signals to the child instance.
       *process_child_id = id_child;
 
+      f_thread_unlock(&instance->use);
+
       // Have the parent wait for the child instance to finish.
       waitpid(id_child, &result.status, 0);
 
-      f_status_t status_lock = controller_lock_write_instance(instance, &instance->use);
+      status_lock = controller_lock_write_instance(instance, &instance->use);
 
       if (F_status_is_error(status_lock)) {
         controller_print_error_lock_critical(&main->program.error, F_status_set_fine(status_lock), F_false, controller_debug_file_line_d);
@@ -667,15 +677,9 @@ extern "C" {
       const pid_t id_child = result.pid;
       result.status = 0;
 
-      // Assign the child instance id to allow for the cancel instance to send appropriate termination signals to the child instance.
-      *process_child_id = id_child;
-
-      // Have the parent wait for the child instance to finish.
-      waitpid(id_child, &result.status, 0);
-
       f_status_t status_lock = controller_lock_write_instance(instance, &instance->use);
 
-      if (status_lock != F_okay) {
+      if (F_status_is_error(status_lock)) {
         controller_print_error_lock_critical(&main->program.error, F_status_set_fine(status_lock), F_false, controller_debug_file_line_d);
 
         return status_lock;
diff --git a/sources/c/program/controller/main/rule/instance.c b/sources/c/program/controller/main/rule/instance.c
index 82c1f3c..25dba48 100644
--- a/sources/c/program/controller/main/rule/instance.c
+++ b/sources/c/program/controller/main/rule/instance.c
@@ -152,6 +152,12 @@ extern "C" {
 
             status = F_false;
             dependency = 0;
+
+            if (F_status_set_fine(status_lock) == F_interrupt) {
+              status = status_lock;
+
+              break;
+            }
           }
           else {
             status = controller_rule_find(main, instance->type != controller_instance_type_exit_e, dynamics[i]->array[j], main->process.rules, &rule);
diff --git a/sources/c/program/controller/main/thread/cleanup.c b/sources/c/program/controller/main/thread/cleanup.c
index dc53af2..52cfd7d 100644
--- a/sources/c/program/controller/main/thread/cleanup.c
+++ b/sources/c/program/controller/main/thread/cleanup.c
@@ -62,122 +62,115 @@ extern "C" {
         break;
       }
 
-      {
-        f_time_spec_t time;
+      status = f_thread_mutex_lock_try(&main->thread.lock.cancel.mutex);
+      if (status != F_okay) continue;
 
-        if (controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
-          status = F_enable_not;
+      if (f_thread_lock_read_try(&main->thread.lock.instance) == F_okay) {
+        if (main->thread.instances.used) {
+          const f_number_unsigned_t total = main->thread.instances.used;
 
-          break;
-        }
+          controller_instance_t *instances[total];
 
-        memset((void *) &time, 0, sizeof(struct timespec));
+          memcpy(instances, main->thread.instances.array, sizeof(controller_instance_t *) * total);
 
-        controller_time_now(controller_thread_timeout_cancel_seconds_d, controller_thread_timeout_cancel_nanoseconds_d, &time);
+          f_thread_unlock(&main->thread.lock.instance);
 
-        status = f_thread_mutex_lock_timed(&time, &main->thread.lock.cancel.mutex);
-        if (status != F_okay) continue;
-      }
+          status = F_okay;
 
-      if (f_thread_lock_write_try(&main->thread.lock.instance) == F_okay) {
-        controller_instance_t *instance = 0;
+          for (f_number_unsigned_t i = 0; i < total && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; ++i) {
 
-        f_number_unsigned_t i = 0;
+            if (!instances[i]) continue;
 
-        for (status = F_okay; i < main->thread.instances.used && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; ++i) {
+            // If "active" has a read or write lock, then do not attempt to clean it.
+            // Once a write lock is achieved on "active", then there should only ever be one process operating on it.
+            // Therefore, the instance.use lock does not need to be used in any way.
+            if (f_thread_lock_write_try(&instances[i]->active) != F_okay) continue;
 
-          if (!main->thread.instances.array[i]) continue;
+            // If instance is active or busy, then do not attempt to clean it.
+            if (instances[i]->state == controller_instance_state_active_e || instances[i]->state == controller_instance_state_busy_e || controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
+              f_thread_unlock(&instances[i]->active);
 
-          instance = main->thread.instances.array[i];
+              if (controller_thread_enable_get(&main->thread) == controller_thread_enable_e) continue;
 
-          // If "active" has a read or write lock, then do not attempt to clean it.
-          // Once a write lock is achieved on "active", then there should only ever be one process operating on it.
-          // Therefore, the instance.use lock does not need to be used in any way.
-          if (f_thread_lock_write_try(&instance->active) != F_okay) continue;
+              break;
+            }
 
-          // If instance is active or busy, then do not attempt to clean it.
-          if (instance->state == controller_instance_state_active_e || instance->state == controller_instance_state_busy_e || controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
-            f_thread_unlock(&instance->active);
+            // If instance has a PID file, then it is running in the background, only cleanup if the PID file no longer exists.
+            if (instances[i]->path_pids.used) {
+              f_number_unsigned_t j = 0;
 
-            if (controller_thread_enable_get(&main->thread) == controller_thread_enable_e) continue;
+              for (; j < instances[i]->path_pids.used && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; ++j) {
 
-            break;
-          }
+                if (instances[i]->path_pids.array[j].used && f_file_exists(instances[i]->path_pids.array[j], F_true) == F_true) {
+                  break;
+                }
+              } // for
 
-          // If instance has a PID file, then it is running in the background, only cleanup if the PID file no longer exists.
-          if (instance->path_pids.used) {
-            f_number_unsigned_t j = 0;
+              if (j < instances[i]->path_pids.used || controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
+                f_thread_unlock(&instances[i]->active);
 
-            for (; j < instance->path_pids.used && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; ++j) {
+                if (controller_thread_enable_get(&main->thread) == controller_thread_enable_e) continue;
 
-              if (instance->path_pids.array[j].used && f_file_exists(instance->path_pids.array[j], F_true) == F_true) {
                 break;
               }
-            } // for
-
-            if (j < instance->path_pids.used || controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
-              f_thread_unlock(&instance->active);
-
-              if (controller_thread_enable_get(&main->thread) == controller_thread_enable_e) continue;
-
-              break;
             }
-          }
 
-          // Close any still open thread.
-          if (instance->thread) {
-            status = controller_thread_join(&instance->thread);
+            // Close any still open thread.
+            if (instances[i]->thread) {
+              status = controller_thread_join(&instances[i]->thread);
 
-            if (F_status_is_error_not(status) || F_status_set_fine(status) == F_found_not) {
-              instance->state = controller_instance_state_idle_e;
+              if (F_status_is_error_not(status) || F_status_set_fine(status) == F_found_not) {
+                instances[i]->state = controller_instance_state_idle_e;
 
-              f_thread_condition_signal_all(&instance->wait_condition);
-            }
-            else {
-              f_thread_unlock(&instance->active);
+                f_thread_condition_signal_all(&instances[i]->wait_condition);
+              }
+              else {
+                f_thread_unlock(&instances[i]->active);
 
-              continue;
+                continue;
+              }
             }
-          }
 
-          // De-allocate dynamic portions of the structure that are only ever needed while the instance is running.
-          controller_cache_delete(&instance->cache);
+            // De-allocate dynamic portions of the structure that are only ever needed while the instance is running.
+            controller_cache_delete(&instances[i]->cache);
 
-          f_memory_array_resize(0, sizeof(controller_rule_t *), (void **) &instance->stack.array, &instance->stack.used, &instance->stack.size);
+            f_memory_array_resize(0, sizeof(controller_rule_t *), (void **) &instances[i]->stack.array, &instances[i]->stack.used, &instances[i]->stack.size);
 
-          // Shrink the childs array.
-          if (instance->childs.used && instance->childs.used < instance->childs.size) {
-            for (; instance->childs.used && instance->childs.used < instance->childs.size && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; --instance->childs.used) {
-              if (instance->childs.array[instance->childs.used]) break;
-            } // for
+            // Shrink the childs array.
+            if (instances[i]->childs.used && instances[i]->childs.used < instances[i]->childs.size) {
+              for (; instances[i]->childs.used && instances[i]->childs.used < instances[i]->childs.size && controller_thread_enable_get(&main->thread) == controller_thread_enable_e; --instances[i]->childs.used) {
+                if (instances[i]->childs.array[instances[i]->childs.used]) break;
+              } // for
 
-            if (controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
-              f_thread_unlock(&instance->active);
+              if (controller_thread_enable_get(&main->thread) != controller_thread_enable_e) {
+                f_thread_unlock(&instances[i]->active);
 
-              break;
-            }
+                break;
+              }
 
-            if (instance->childs.used < instance->childs.size) {
-              f_memory_array_resize(instance->childs.used, sizeof(pid_t), (void **) &instance->childs.array, &instance->childs.used, &instance->childs.size);
+              if (instances[i]->childs.used < instances[i]->childs.size) {
+                f_memory_array_resize(instances[i]->childs.used, sizeof(pid_t), (void **) &instances[i]->childs.array, &instances[i]->childs.used, &instances[i]->childs.size);
+              }
             }
-          }
 
-          // De-allocate the PID files.
-          if (instance->path_pids.used) {
-            instance->path_pids.used = 0;
+            // De-allocate the PID files.
+            if (instances[i]->path_pids.used) {
+              instances[i]->path_pids.used = 0;
 
-            f_memory_arrays_resize(0, sizeof(f_string_dynamic_t), (void **) &instance->path_pids.array, &instance->path_pids.used, &instance->path_pids.size, &f_string_dynamics_delete_callback);
-          }
-
-          // De-allocate any rules in the space that is declared to be unused.
-          if (i >= main->thread.instances.used) {
-            controller_rule_delete(&instance->rule);
-          }
+              f_memory_arrays_resize(0, sizeof(f_string_dynamic_t), (void **) &instances[i]->path_pids.array, &instances[i]->path_pids.used, &instances[i]->path_pids.size, &f_string_dynamics_delete_callback);
+            }
 
-          f_thread_unlock(&instance->active);
-        } // for
+            // De-allocate any rules in the space that is declared to be unused.
+            if (i >= main->thread.instances.used) {
+              controller_rule_delete(&instances[i]->rule);
+            }
 
-        f_thread_unlock(&main->thread.lock.instance);
+            f_thread_unlock(&instances[i]->active);
+          } // for
+        }
+        else {
+          f_thread_unlock(&main->thread.lock.instance);
+        }
       }
 
       f_thread_mutex_unlock(&main->thread.lock.cancel.mutex);
diff --git a/sources/c/program/controller/main/thread/instance.c b/sources/c/program/controller/main/thread/instance.c
index 08228b0..e6ad454 100644
--- a/sources/c/program/controller/main/thread/instance.c
+++ b/sources/c/program/controller/main/thread/instance.c
@@ -41,7 +41,7 @@ extern "C" {
 
     if (!main) return;
 
-    f_status_t status = controller_mutex_lock(is_normal, controller_lock_check_flag_time_d, controller_thread_timeout_cancel_seconds_d, controller_thread_timeout_cancel_nanoseconds_d, &main->thread, &main->thread.lock.cancel);
+    f_status_t status = f_thread_mutex_lock(&main->thread.lock.cancel.mutex);
     if (F_status_is_error(status)) return;
 
     // Only cancel when enabled.
@@ -103,6 +103,9 @@ extern "C" {
 
     if (main->thread.id_cleanup) {
       f_thread_cancel(main->thread.id_cleanup);
+
+      f_thread_condition_signal_all(&main->thread.lock.reap_condition);
+
       f_thread_join(main->thread.id_cleanup, 0);
 
       main->thread.id_cleanup = 0;
@@ -138,11 +141,19 @@ extern "C" {
       return;
     }
 
-    for (int signal = 0; i < main->thread.instances.used; ++i) {
+    const f_number_unsigned_t total = main->thread.instances.used;
+
+    controller_instance_t *instances[total];
 
-      if (!main->thread.instances.array[i]) continue;
+    memcpy(instances, main->thread.instances.array, sizeof(controller_instance_t *) * total);
+
+    f_thread_unlock(&main->thread.lock.instance);
 
-      instance = main->thread.instances.array[i];
+    for (int signal = 0; i < total; ++i) {
+
+      if (!instances[i]) continue;
+
+      instance = instances[i];
 
       // Do not cancel Exit Instances, when not performing "execute" during exit.
       if (instance->type == controller_instance_type_exit_e && controller_thread_enable_get(&main->thread) != controller_thread_enable_exit_execute_e) {
@@ -189,11 +200,11 @@ extern "C" {
     if (entry->timeout_exit && !(entry->flag & controller_entry_flag_timeout_exit_no_e)) {
       f_number_unsigned_t lapsed = 0;
 
-      for (i = 0; i < main->thread.instances.used && lapsed < entry->timeout_exit; ++i) {
+      for (i = 0; i < total && lapsed < entry->timeout_exit; ++i) {
 
-        if (!main->thread.instances.array[i]) continue;
+        if (!instances[i]) continue;
 
-        instance = main->thread.instances.array[i];
+        instance = instances[i];
 
         // Do not wait for Instances, when not performing "execute" during exit.
         if (instance->type == controller_instance_type_exit_e && controller_thread_enable_get(&main->thread) != controller_thread_enable_exit_execute_e) {
@@ -264,11 +275,11 @@ extern "C" {
       } // for
     }
 
-    for (i = 0; i < main->thread.instances.size; ++i) {
+    for (i = 0; i < total; ++i) {
 
-      if (!main->thread.instances.array[i]) continue;
+      if (!instances[i]) continue;
 
-      instance = main->thread.instances.array[i];
+      instance = instances[i];
 
       status = controller_lock_read(is_normal, controller_lock_check_flag_error_d, controller_thread_timeout_cancel_seconds_d, controller_thread_timeout_cancel_nanoseconds_d, &main->thread, &instance->active);
       if (F_status_is_error(status)) continue;
@@ -403,8 +414,8 @@ extern "C" {
       f_thread_unlock(&instance->active);
     } // for
 
-    f_thread_unlock(&main->thread.lock.instance);
     f_thread_mutex_unlock(&main->thread.lock.cancel.mutex);
+
   }
 #endif // _di_controller_thread_instance_cancel_