pnoltes commented on code in PR #753:
URL: https://github.com/apache/celix/pull/753#discussion_r1641154805
##########
libs/framework/src/celix_launcher.c:
##########
@@ -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;
Review Comment:
nitpick: Maybe also good to document that shutdown and signal are treated
with atomic functions.
##########
libs/framework/gtest/src/CelixLauncherTestSuite.cc:
##########
@@ -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));
Review Comment:
:+1:
##########
libs/framework/src/celix_launcher.c:
##########
@@ -364,40 +377,74 @@ 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) {
+ // assert(data == &g_launcher)
Review Comment:
nitpick: Commented out code.
##########
libs/framework/src/celix_launcher.c:
##########
@@ -364,40 +377,74 @@ 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);
Review Comment:
:+1: Nice to see the signal handler now does not use any mutexes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
