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 027d9cfd0d4911defe017779ded26ec69fe0453f..81944c9808f696a478b38dfbd5f5bb0ddb1e04da 100644 (file)
--- 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 82c1f3cecb2cfbd4882702e287c0ad9867188fa7..25dba48c651f5db28b1c836c1f476142b7add351 100644 (file)
--- 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 dc53af2eb595e63caec37cb93fd2adad9c7b936f..52cfd7dd35cb7fd4a55e52fc306b4ef74ac65290 100644 (file)
--- 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 08228b09046f3e7f66bfd19b8408ae4805228e7c..e6ad4540a707e4b92add9a4e2efa444edfc9f153 100644 (file)
--- 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_