Merge pull request #753 from apache/feature/215-stop-launcher-by-signal

Feature/215 async-signal-safe launcher shutdown
apache · Jun 16, 2024 · 63683c0 · 63683c0
2 parents 58860bb + 2378bac
commit 63683c0
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 45 deletions.
diff --git a/libs/framework/gtest/src/CelixLauncherTestSuite.cc b/libs/framework/gtest/src/CelixLauncherTestSuite.cc
@@ -19,10 +19,12 @@
 
 #include <gtest/gtest.h>
 
+#include <csignal>
 #include <thread>
 #include <future>
 #include <vector>
 #include <string>
+#include <pthread.h>
 
 #include "celix_constants.h"
 #include "celix_launcher.h"
@@ -190,19 +192,31 @@ TEST_F(CelixLauncherTestSuite, LaunchWithInvalidConfigPropertiesTest) {
     //Then the launch will exit
     auto status = future.wait_for(std::chrono::milliseconds{LAUNCH_WAIT_TIMEOUT});
     EXPECT_EQ(status, std::future_status::ready);
+
+    //When launching the framework with a properties set with a negative shutdown period
+    auto* props = celix_properties_create();
+    ASSERT_TRUE(props != nullptr);
+    celix_properties_setDouble(props, CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS, -1.0);
+    future = launchInThread({"programName"}, props, 1);
+    //Then launch will exit
+    status = future.wait_for(std::chrono::milliseconds{LAUNCH_WAIT_TIMEOUT});
+    EXPECT_EQ(status, std::future_status::ready);
 }
 
+
 TEST_F(CelixLauncherTestSuite, StopLauncherWithSignalTest) {
+    auto* props = celix_properties_create();
     // When launching the framework
-    auto future = launchInThread({"programName"}, nullptr, 0);
+    celix_properties_setDouble(props, CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS, 0.01);
+    auto future = launchInThread({"programName"}, props, 0);
 
     // Then the launch will not exit, because the framework is running
     auto status = future.wait_for(std::chrono::milliseconds{LAUNCH_WAIT_TIMEOUT});
     EXPECT_EQ(status, std::future_status::timeout);
 
-    // When I stop the framework by mimicking a SIGINT signal
+    // When I stop the framework by sending a SIGINT signal
     int signal = SIGINT;
-    celix_launcher_stopInternal(&signal);
+    EXPECT_EQ(0, pthread_kill(pthread_self(), signal));
 
     // Then the launch will exit
     status = future.wait_for(std::chrono::milliseconds{LAUNCH_WAIT_TIMEOUT});

diff --git a/libs/framework/gtest/src/ScheduledEventTestSuite.cc b/libs/framework/gtest/src/ScheduledEventTestSuite.cc
@@ -231,7 +231,21 @@ TEST_F(ScheduledEventTestSuite, InvalidOptionsAndArgumentsTest) {
     auto ctx = fw->getFrameworkBundleContext();
     celix_scheduled_event_options_t opts{}; // no callback
     long scheduledEventId = celix_bundleContext_scheduleEvent(ctx->getCBundleContext(), &opts);
+    // Then I expect an error
+    EXPECT_LT(scheduledEventId, 0);
+
+    // When I create a scheduled event with negative initial delay
+    opts.name = "Invalid scheduled event test";
+    opts.initialDelayInSeconds = -1;
+    opts.callback = [](void* /*data*/) { FAIL() << "Scheduled event called, but should not be called"; };
+    scheduledEventId = celix_bundleContext_scheduleEvent(ctx->getCBundleContext(), &opts);
+    // Then I expect an error
+    EXPECT_LT(scheduledEventId, 0);
 
+    // When I create a scheduled event with negative interval value
+    opts.initialDelayInSeconds = 0.1;
+    opts.intervalInSeconds = -1;
+    scheduledEventId = celix_bundleContext_scheduleEvent(ctx->getCBundleContext(), &opts);
     // Then I expect an error
     EXPECT_LT(scheduledEventId, 0);
 

diff --git a/libs/framework/include/celix_launcher.h b/libs/framework/include/celix_launcher.h
@@ -28,6 +28,15 @@
 extern "C" {
 #endif
 
+/**
+ * @brief Celix launcher environment property to specify interval of the periodic shutdown check performed by launcher.
+ *
+ * The launcher will perform a periodic check to see whether to perform a shutdown, and if so, the launcher will
+ * stop and destroy the framework. The interval of this check can be specified in seconds using this property.
+ */
+#define CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS           "CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS"
+#define CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS_DEFAULT   1.0
+
 /**
  * @brief Launch a celix framework, wait (block) until the framework is stopped and destroy the framework when stopped.
  *

diff --git a/libs/framework/src/bundle_context.c b/libs/framework/src/bundle_context.c
@@ -579,7 +579,7 @@ celix_dependency_manager_t* celix_bundleContext_getDependencyManager(bundle_cont
     if (ctx->mng) {
         return ctx->mng;
     }
-    celixThreadRwlock_unlock(celix_steal_ptr(rlockGuard.lock));
+    celixRwlockRlockGuard_deinit(&rlockGuard);
 
     celix_auto(celix_rwlock_wlock_guard_t) wlockGuard = celixRwlockWlockGuard_init(&ctx->lock);
     if (ctx->mng == NULL) {

diff --git a/libs/framework/src/celix_launcher.c b/libs/framework/src/celix_launcher.c
@@ -19,6 +19,7 @@
 
 #include "celix_launcher.h"
 #include "celix_launcher_private.h"
+#include "celix_compiler.h"
 
 #include <signal.h>
 #include <stdio.h>
@@ -36,14 +37,21 @@
 #include "celix_file_utils.h"
 #include "celix_framework_factory.h"
 #include "celix_framework_utils.h"
+#include "celix_threads.h"
 
 #define DEFAULT_CONFIG_FILE "config.properties"
 
 #define CELIX_LAUNCHER_OK_EXIT_CODE 0
 #define CELIX_LAUNCHER_ERROR_EXIT_CODE 1
 
-static bool g_framework_launched = false;
-static framework_t* g_framework = NULL;
+typedef struct {
+    celix_thread_mutex_t lock; // protect the following
+    framework_t* framework;
+    long shutdownEventId;
+    bool launched;
+    bool shutdown; // accessed through atomic operations
+    int signal; // accessed through atomic operations
+} celix_launcher_t;
 
 typedef struct {
     bool showHelp;
@@ -52,6 +60,8 @@ typedef struct {
     const char* configFile;
 } celix_launcher_options_t;
 
+static celix_launcher_t g_launcher = { PTHREAD_MUTEX_INITIALIZER, NULL, -1L, false, false, -1 };
+
 /**
  * @brief SIGUSR1 SIGUSR2 no-op callback handler.
  */
@@ -120,7 +130,7 @@ static celix_status_t celix_launcher_loadRuntimeProperties(const char* configFil
 /**
  * @brief Set the global framework instance.
  */
-static void celix_launcher_setGlobalFramework(celix_framework_t* fw);
+static celix_status_t celix_launcher_setGlobalFramework(celix_framework_t* fw);
 
 int celix_launcher_launchAndWait(int argc, char* argv[], const char* embeddedConfig) {
     celix_autoptr(celix_properties_t) embeddedProps = NULL;
@@ -167,18 +177,19 @@ int celix_launcher_launchAndWait(int argc, char* argv[], const char* embeddedCon
 
     celix_framework_t* framework = NULL;
     status = celix_launcher_createFramework(celix_steal_ptr(embeddedProps), runtimeProps, &framework);
+    if (status != CELIX_SUCCESS) {
+        celix_launcher_resetLauncher();
+        return CELIX_LAUNCHER_ERROR_EXIT_CODE;
+    }
+    status = celix_launcher_setGlobalFramework(framework);
     if (status == CELIX_SUCCESS) {
-        celix_launcher_setGlobalFramework(framework);
         celix_framework_waitForStop(framework);
-        celix_frameworkFactory_destroyFramework(framework);
+    }
+    celix_launcher_resetLauncher();
 #ifndef CELIX_NO_CURLINIT
-        // Cleanup Curl
-        curl_global_cleanup();
+    // Cleanup Curl
+    curl_global_cleanup();
 #endif
-        celix_launcher_resetLauncher();
-    } else {
-        celix_launcher_resetLauncher();
-    }
     return status == CELIX_SUCCESS ? CELIX_LAUNCHER_OK_EXIT_CODE : CELIX_LAUNCHER_ERROR_EXIT_CODE;
 }
 
@@ -245,12 +256,14 @@ static celix_status_t celix_launcher_createFramework(celix_properties_t* embedde
     return *frameworkOut != NULL ? CELIX_SUCCESS : CELIX_FRAMEWORK_EXCEPTION;
 }
 
+// LCOV_EXCL_START
 /**
  * @brief SIGUSR1 SIGUSR2 no-op callback
  */
 static void celix_launcher_noopSignalHandler(int signal __attribute__((unused))) {
     // ignoring signal SIGUSR1, SIGUSR2. Can be used on threads
 }
+// LCOV_EXCL_STOP
 
 static void celix_launcher_printUsage(char* progName) {
     printf("Usage:\n  %s [-h|-p|-c] [path/to/runtime/config.properties]\n", basename(progName));
@@ -364,40 +377,73 @@ static celix_status_t celix_launcher_loadRuntimeProperties(const char* configFil
 }
 
 static void celix_launcher_shutdownFrameworkSignalHandler(int signal) {
-    celix_launcher_stopInternal(&signal);
+    __atomic_store_n(&g_launcher.signal, signal, __ATOMIC_RELAXED);
+    __atomic_store_n(&g_launcher.shutdown, true, __ATOMIC_RELEASE);
 }
 
 void celix_launcher_triggerStop() {
-    celix_launcher_stopInternal(NULL);
-}
-
-static void celix_launcher_setGlobalFramework(celix_framework_t* fw) {
-    __atomic_store_n(&g_framework, fw, __ATOMIC_SEQ_CST);
+    celix_auto(celix_mutex_lock_guard_t) lck = celixMutexLockGuard_init(&g_launcher.lock);
+    if (g_launcher.framework == NULL || g_launcher.shutdownEventId == -1) {
+        fprintf(stderr, "No global framework instance to stop\n");
+        return;
+    }
+    __atomic_store_n(&g_launcher.shutdown, true, __ATOMIC_RELAXED);
+    celix_bundleContext_wakeupScheduledEvent(celix_framework_getFrameworkContext(g_launcher.framework),
+                                             g_launcher.shutdownEventId);
 }
 
-void celix_launcher_stopInternal(const int* signal) {
-    celix_framework_t* fw = __atomic_exchange_n(&g_framework, NULL, __ATOMIC_SEQ_CST);
-    if (fw) {
-        if (signal) {
-            celix_bundle_context_t* ctx = celix_framework_getFrameworkContext(fw);
+static void celix_launcher_shutdownCheck(void* data CELIX_UNUSED) {
+    celix_auto(celix_mutex_lock_guard_t) lck = celixMutexLockGuard_init(&g_launcher.lock);
+    if (__atomic_load_n(&g_launcher.shutdown, __ATOMIC_ACQUIRE)) {
+        int sig = __atomic_load_n(&g_launcher.signal, __ATOMIC_RELAXED);
+        if (sig != -1) {
             celix_bundleContext_log(
-                ctx, CELIX_LOG_LEVEL_INFO, "Stopping Celix framework due to signal %s", strsignal(*signal));
+                celix_framework_getFrameworkContext(g_launcher.framework), CELIX_LOG_LEVEL_INFO,
+                "Stopping Celix framework due to signal %s", strsignal(sig));
         }
-        celix_framework_stopBundle(fw, CELIX_FRAMEWORK_BUNDLE_ID);
-    } else {
-        fprintf(stderr, "No global framework instance to stop\n");
+        celix_bundleContext_removeScheduledEventAsync(celix_framework_getFrameworkContext(g_launcher.framework),
+                                                      g_launcher.shutdownEventId);
+        celix_framework_stopBundleAsync(g_launcher.framework, CELIX_FRAMEWORK_BUNDLE_ID);
+        g_launcher.shutdownEventId = -1;
     }
 }
 
+static celix_status_t celix_launcher_setGlobalFramework(celix_framework_t* fw) {
+    celix_auto(celix_mutex_lock_guard_t) lck = celixMutexLockGuard_init(&g_launcher.lock);
+    celix_bundle_context_t *ctx = celix_framework_getFrameworkContext(fw);
+    g_launcher.framework = fw;
+    celix_scheduled_event_options_t opts = CELIX_EMPTY_SCHEDULED_EVENT_OPTIONS;
+    opts.name = "celix_shutdown_check";
+    opts.callback = celix_launcher_shutdownCheck;
+    opts.callbackData = &g_launcher;
+    opts.initialDelayInSeconds = celix_bundleContext_getPropertyAsDouble(ctx, CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS,
+                                                                         CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS_DEFAULT);
+    opts.intervalInSeconds = celix_bundleContext_getPropertyAsDouble(ctx, CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS,
+                                                                     CELIX_LAUNCHER_SHUTDOWN_PERIOD_IN_SECONDS_DEFAULT);
+    g_launcher.shutdownEventId = celix_bundleContext_scheduleEvent(ctx, &opts);
+    return g_launcher.shutdownEventId >= 0 ? CELIX_SUCCESS : CELIX_FRAMEWORK_EXCEPTION;
+}
+
 static bool celix_launcher_checkFrameworkLaunched() {
-    bool alreadyLaunched = __atomic_exchange_n(&g_framework_launched, true, __ATOMIC_SEQ_CST);
-    if (alreadyLaunched) {
+    celix_auto(celix_mutex_lock_guard_t) lck = celixMutexLockGuard_init(&g_launcher.lock);
+    if (g_launcher.launched) {
         fprintf(stderr, "Cannot launch framework, already launched\n");
+        return false;
     }
-    return !alreadyLaunched;
+    g_launcher.launched = true;
+    return true;
 }
 
 static void celix_launcher_resetLauncher() {
-    __atomic_store_n(&g_framework_launched, false, __ATOMIC_SEQ_CST);
-    __atomic_store_n(&g_framework, NULL, __ATOMIC_SEQ_CST);
+    celix_auto(celix_mutex_lock_guard_t) lck = celixMutexLockGuard_init(&g_launcher.lock);
+    __atomic_store_n(&g_launcher.shutdown, false, __ATOMIC_RELAXED);
+    __atomic_store_n(&g_launcher.signal, -1, __ATOMIC_RELAXED);
+    if (g_launcher.framework) {
+        long schedId = g_launcher.shutdownEventId;
+        g_launcher.shutdownEventId = -1L;
+        celix_bundleContext_removeScheduledEvent(celix_framework_getFrameworkContext(g_launcher.framework), schedId);
+        celix_frameworkFactory_destroyFramework(g_launcher.framework);
+        g_launcher.framework = NULL;
+    }
+    g_launcher.launched = false;
 }
diff --git a/libs/framework/src/celix_launcher_private.h b/libs/framework/src/celix_launcher_private.h
@@ -24,15 +24,6 @@
 extern "C" {
 #endif
 
-/**
- * @brief Stop the framework, by stopping the framework bundle.
- *
- * Also logs a message if the framework is stopped due to a signal.
- *
- * @param signal The optional signal that caused the framework to stop.
- */
-void celix_launcher_stopInternal(const int* signal);
-
 /**
  * @brief Create a combined configuration properties set by combining the embedded properties with the runtime
  * properties.

diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c
@@ -1383,7 +1383,7 @@ static void celix_framework_processScheduledEvents(celix_framework_t* fw) {
         celixThreadMutex_lock(&fw->dispatcher.mutex);
         CELIX_LONG_HASH_MAP_ITERATE(fw->dispatcher.scheduledEvents, entry) {
             celix_scheduled_event_t* visit = entry.value.ptrValue;
-            if (celix_scheduledEvent_isMarkedForRemoval(visit)) {
+            if (!fw->dispatcher.active || celix_scheduledEvent_isMarkedForRemoval(visit)) {
                 removeEvent = visit;
                 celix_longHashMap_remove(fw->dispatcher.scheduledEvents, celix_scheduledEvent_getId(visit));
                 break;
@@ -1531,6 +1531,7 @@ static void *fw_eventDispatcher(void *fw) {
     }
 
     //not active anymore, extra runs for possible request leftovers
+    celix_framework_processScheduledEvents(framework);
     celixThreadMutex_lock(&framework->dispatcher.mutex);
     bool needExtraRun = celix_framework_eventQueueSize(fw) > 0;
     celixThreadMutex_unlock(&framework->dispatcher.mutex);
@@ -2542,6 +2543,13 @@ long celix_framework_scheduleEvent(celix_framework_t* fw,
                bndId);
         return -1;
     }
+    if (initialDelayInSeconds < 0 || intervalInSeconds < 0) {
+        fw_log(fw->logger,
+               CELIX_LOG_LEVEL_ERROR,
+               "Cannot add scheduled event for bundle id %li. Invalid intervals: (%f,%f).",
+               bndId, initialDelayInSeconds, intervalInSeconds);
+        return -1;
+    }
 
     celix_bundle_entry_t* bndEntry = celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(fw, bndId);
     if (bndEntry == NULL) {