This is an automated email from the ASF dual-hosted git repository. pnoltes pushed a commit to branch feature/scheduled_event_on_event_thread in repository https://gitbox.apache.org/repos/asf/celix.git
commit 8ca23ebf718586917b4f78d4373c7b8ee7554242 Author: Pepijn Noltes <[email protected]> AuthorDate: Tue Jun 13 20:30:46 2023 +0200 Refactor scheduled event wakeup --- documents/scheduled_events .md | 4 -- .../framework/gtest/src/ScheduledEventTestSuite.cc | 81 +++++++++------------- libs/framework/include/celix/ScheduledEvent.h | 28 ++------ libs/framework/include/celix_bundle_context.h | 17 ++--- libs/framework/src/bundle_context.c | 7 +- libs/framework/src/celix_scheduled_event.c | 2 +- libs/framework/src/celix_scheduled_event.h | 2 +- libs/framework/src/framework.c | 25 ++----- libs/framework/src/framework_private.h | 12 +--- 9 files changed, 52 insertions(+), 126 deletions(-) diff --git a/documents/scheduled_events .md b/documents/scheduled_events .md index 1c81a0ed..d01dfeba 100644 --- a/documents/scheduled_events .md +++ b/documents/scheduled_events .md @@ -133,7 +133,3 @@ CELIX_GEN_CXX_BUNDLE_ACTIVATOR(ScheduleEventsBundleActivator) To process a scheduled event directly, you can use the `celix_bundleContext_wakeupScheduledEvent` C function or `celix::ScheduledEvent::wakup` C++ method. This will wake up the scheduled event and call its callback function. - -An optional wait time can be provided to block until the event is done. -If the wait time is not zero, the function will block until the scheduled event callback is called or the wait time -duration has elapsed. If the wait time is zero, the function will return immediately. diff --git a/libs/framework/gtest/src/ScheduledEventTestSuite.cc b/libs/framework/gtest/src/ScheduledEventTestSuite.cc index f4459b8c..3a833e98 100644 --- a/libs/framework/gtest/src/ScheduledEventTestSuite.cc +++ b/libs/framework/gtest/src/ScheduledEventTestSuite.cc @@ -232,8 +232,8 @@ TEST_F(ScheduledEventTestSuite, WakeUpEventTest) { std::atomic<int> count{0}; celix_scheduled_event_options_t opts{}; opts.name = "test wakeup"; - opts.initialDelayInSeconds = 0.1; - opts.intervalInSeconds = 0.1; + opts.initialDelayInSeconds = 0.01; + opts.intervalInSeconds = 0.01; opts.callbackData = static_cast<void*>(&count); opts.callback = [](void* countPtr) { auto* count = static_cast<std::atomic<int>*>(countPtr); @@ -245,43 +245,34 @@ TEST_F(ScheduledEventTestSuite, WakeUpEventTest) { EXPECT_EQ(0, count.load()); // When the scheduled event is woken up - celix_status_t status = celix_bundleContext_wakeupScheduledEvent( - fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId, 1); + celix_status_t status = celix_bundleContext_wakeupScheduledEvent(fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId); // Then the status is CELIX_SUCCESS ASSERT_EQ(CELIX_SUCCESS, status); - // And the count is increased + // When waiting a bit to let the scheduled event trigger + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + + // Then the count is increased EXPECT_EQ(1, count.load()); // When waiting longer than the interval - std::this_thread::sleep_for(std::chrono::milliseconds{110}); + std::this_thread::sleep_for(std::chrono::milliseconds{11}); // Then the count is increased EXPECT_EQ(2, count.load()); - // When the scheduled event is woken up again without waiting (waitTimeInSec = 0) - status = celix_bundleContext_wakeupScheduledEvent( - fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId, 0); - - // And the process is delayed to ensure the event is called - std::this_thread::sleep_for(std::chrono::milliseconds{10}); + // When waking up the scheduled event again + status = celix_bundleContext_wakeupScheduledEvent(fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId); // Then the status is CELIX_SUCCESS ASSERT_EQ(CELIX_SUCCESS, status); - // And the count is increased - EXPECT_EQ(3, count.load()); + // When waiting a bit to let the scheduled event trigger + std::this_thread::sleep_for(std::chrono::milliseconds{2}); - // When the scheduled event is woken up again - status = celix_bundleContext_wakeupScheduledEvent( - fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId, 1); - - // Then the status is CELIX_SUCCESS - ASSERT_EQ(CELIX_SUCCESS, status); - - // And the count is increased - EXPECT_EQ(4, count.load()); + // Then the count is increased + EXPECT_EQ(3, count.load()); celix_bundleContext_removeScheduledEvent(fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId); } @@ -304,19 +295,22 @@ TEST_F(ScheduledEventTestSuite, WakeUpOneShotEventTest) { // When the scheduled event is woken up celix_status_t status = celix_bundleContext_wakeupScheduledEvent( - fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId, 1); + fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId); // Then the status is CELIX_SUCCESS ASSERT_EQ(CELIX_SUCCESS, status); + // When waiting a bit to ensure the scheduled event is woken up + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + // And the count is increased EXPECT_EQ(1, count.load()); // And when the scheduled event is woken up again - status = celix_bundleContext_wakeupScheduledEvent( - fw->getFrameworkBundleContext()->getCBundleContext(), scheduledEventId, 0); + status = celix_bundleContext_wakeupScheduledEvent(fw->getFrameworkBundleContext()->getCBundleContext(), + scheduledEventId); - // Then the status is ILLEGAL_ARGUMENT, becuase the scheduled event is already woken up and a one-shot event + // Then the status is ILLEGAL_ARGUMENT, because the scheduled event is already woken up and a one-shot event ASSERT_EQ(CELIX_ILLEGAL_ARGUMENT, status); } @@ -347,8 +341,11 @@ TEST_F(ScheduledEventTestSuite, CxxScheduledEventTest) { // Then the count is increased EXPECT_EQ(1, count.load()); - // When waking up the event with a wait time of 1s - event.wakeup(std::chrono::seconds{1}); + // When waking up the event + event.wakeup(); + + // And waiting a bit to ensure the event is called + std::this_thread::sleep_for(std::chrono::milliseconds{2}); // Then the count is increased EXPECT_EQ(2, count.load()); @@ -356,8 +353,8 @@ TEST_F(ScheduledEventTestSuite, CxxScheduledEventTest) { // When waking up the event without waiting event.wakeup(); - // And the process is delayed to ensure the event is called - std::this_thread::sleep_for(std::chrono::milliseconds{10}); + // And waiting a bit to ensure the event is called + std::this_thread::sleep_for(std::chrono::milliseconds{2}); // Then the count is increased EXPECT_EQ(3, count.load()); @@ -445,7 +442,10 @@ TEST_F(ScheduledEventTestSuite, CxxOneShotScheduledEventTest) { EXPECT_TRUE(removed.load()); // When waking up the event with a wait time of 1s - event.wakeup(std::chrono::seconds{1}); + event.wakeup(); + + // And waiting a bit to ensure the event is called + std::this_thread::sleep_for(std::chrono::milliseconds{2}); // Then the count is not increased, because the event is a one-shot event EXPECT_EQ(1, count.load()); @@ -491,23 +491,6 @@ TEST_F(ScheduledEventTestSuite, CxxCreateScheduledEventWithNoCallbackTest) { EXPECT_ANY_THROW(fw->getFrameworkBundleContext()->scheduledEvent().build()); // Note no callback } -TEST_F(ScheduledEventTestSuite, TimeoutOnWakeupTest) { - auto callback = [](void*) { - // delay process to check if wakeup timeout works - std::this_thread::sleep_for(std::chrono::milliseconds{10}); - }; - - celix_scheduled_event_options_t opts{}; - opts.callback = callback; - opts.initialDelayInSeconds = 1; - long eventId = celix_bundleContext_scheduleEvent(fw->getFrameworkBundleContext()->getCBundleContext(), &opts); - EXPECT_GE(eventId, 0); - - auto status = - celix_bundleContext_wakeupScheduledEvent(fw->getFrameworkBundleContext()->getCBundleContext(), eventId, 0.001); - EXPECT_EQ(CELIX_TIMEOUT, status);; -} - TEST_F(ScheduledEventTestSuite, CxxCancelOneShotEventBeforeFiredTest) { auto callback = []() { FAIL() << "Should not be called"; diff --git a/libs/framework/include/celix/ScheduledEvent.h b/libs/framework/include/celix/ScheduledEvent.h index 510c8a77..9b9e2b1b 100644 --- a/libs/framework/include/celix/ScheduledEvent.h +++ b/libs/framework/include/celix/ScheduledEvent.h @@ -32,13 +32,14 @@ namespace celix { * This class uses RAII to automatically remove the (non one-shot) scheduled event from the bundle context * when it is destroyed. For one-shot scheduled events, the destructor will not remove the scheduled event. */ - //TODO update blocking calls with a timeout exception class ScheduledEvent final { public: friend class ScheduledEventBuilder; /** * @brief Constructs a empty / not-active scheduled event. + * + * During destruction the scheduled event will be removed asynchronously from the bundle context. */ ScheduledEvent() = default; @@ -71,10 +72,10 @@ class ScheduledEvent final { } /** - * @brief Wakes up the scheduled event and returns immediately, not waiting for the scheduled event callback to be + * @brief Wakeup a scheduled event and returns immediately, not waiting for the scheduled event callback to be * called. */ - void wakeup() { wakeup(std::chrono::duration<double>{0}); } + void wakeup() { celix_bundleContext_wakeupScheduledEvent(ctx.get(), eventId); } /** * @brief Wait until the next scheduled event is processed. @@ -95,27 +96,6 @@ class ScheduledEvent final { return status == CELIX_SUCCESS; } - /** - * @brief Wakes up the scheduled event with an optional wait time. - * - * If `waitTime` is not zero, this function will block until the scheduled event callback is called or the - * `waitTime` duration has elapsed. If `waitTime` is zero, this function will return immediately. - * - * @tparam Rep The representation type of the duration. - * @tparam Period The period type of the duration. - * @param[in] waitTime The wait time duration (default is zero). - * @return true if the scheduled event was woken up, false if a timeout occurred. - */ - template <typename Rep, typename Period> - bool wakeup(std::chrono::duration<Rep, Period> waitTime) { - double waitTimeInSeconds = std::chrono::duration_cast<std::chrono::duration<double>>(waitTime).count(); - celix_status_t status = CELIX_SUCCESS; - if (ctx) { - status = celix_bundleContext_wakeupScheduledEvent(ctx.get(), eventId, waitTimeInSeconds); - } - return status == CELIX_SUCCESS; - } - private: struct Callbacks { std::function<void()> callback{}; /**< The callback for the scheduled event. */ diff --git a/libs/framework/include/celix_bundle_context.h b/libs/framework/include/celix_bundle_context.h index 4a6383be..63c1e495 100644 --- a/libs/framework/include/celix_bundle_context.h +++ b/libs/framework/include/celix_bundle_context.h @@ -1318,22 +1318,15 @@ CELIX_FRAMEWORK_EXPORT long celix_bundleContext_scheduleEvent(celix_bundle_conte const celix_scheduled_event_options_t* options); /** - * @brief Wakeup a scheduled event. - * - * If waitTimeInSeconds is not 0, this function will block until the scheduled event callback is called. - * If waitTimeInSeconds is 0, this function will return immediately. + * @brief Wakeup a scheduled event and returns immediately, not waiting for the scheduled event callback to be + * called. * * @param[in] ctx The bundle context. * @param[in] scheduledEventId The scheduled event id to wakeup. - * @param[in] waitTimeInSeconds If not 0, this function will block until the scheduled event callback - * is called or the provided timeout is reached. - * @return CELIX_SUCCESS if the scheduled event is woken up, CELIX_ILLEGAL_ARGUMENT if the scheduled event id is not - * known and CELIX_TIMEOUT if the waitTimeInSeconds is reached. + * @return CELIX_SUCCESS if the scheduled event is woken up, CELIX_ILLEGAL_ARGUMENT if the scheduled event id is not */ -CELIX_FRAMEWORK_EXPORT celix_status_t celix_bundleContext_wakeupScheduledEvent( - celix_bundle_context_t* ctx, - long scheduledEventId, - double waitTimeInSeconds); +CELIX_FRAMEWORK_EXPORT celix_status_t celix_bundleContext_wakeupScheduledEvent(celix_bundle_context_t* ctx, + long scheduledEventId); /** * @brief Wait until the next scheduled event is processed. diff --git a/libs/framework/src/bundle_context.c b/libs/framework/src/bundle_context.c index a0c26ea7..0c3e2031 100644 --- a/libs/framework/src/bundle_context.c +++ b/libs/framework/src/bundle_context.c @@ -1502,11 +1502,8 @@ long celix_bundleContext_scheduleEvent(celix_bundle_context_t* ctx, options->removeCallback); } -celix_status_t celix_bundleContext_wakeupScheduledEvent( - celix_bundle_context_t* ctx, - long scheduledEventId, - double waitTimeInSeconds) { - return celix_framework_wakeupScheduledEvent(ctx->framework, scheduledEventId, waitTimeInSeconds); +celix_status_t celix_bundleContext_wakeupScheduledEvent(celix_bundle_context_t* ctx, long scheduledEventId) { + return celix_framework_wakeupScheduledEvent(ctx->framework, scheduledEventId); } celix_status_t celix_bundleContext_waitForScheduledEvent(celix_bundle_context_t* ctx, diff --git a/libs/framework/src/celix_scheduled_event.c b/libs/framework/src/celix_scheduled_event.c index 99d94178..7537a67e 100644 --- a/libs/framework/src/celix_scheduled_event.c +++ b/libs/framework/src/celix_scheduled_event.c @@ -219,7 +219,7 @@ bool celix_scheduledEvent_isSingleShot(celix_scheduled_event_t* event) { return isDone; } -size_t celix_scheduledEvent_configureWakeup(celix_scheduled_event_t* event) { +size_t celix_scheduledEvent_markForWakeup(celix_scheduled_event_t* event) { celixThreadMutex_lock(&event->mutex); event->processForWakeup = true; size_t currentCallCount = event->callCount; diff --git a/libs/framework/src/celix_scheduled_event.h b/libs/framework/src/celix_scheduled_event.h index 20c2cec9..5b0e79fc 100644 --- a/libs/framework/src/celix_scheduled_event.h +++ b/libs/framework/src/celix_scheduled_event.h @@ -138,7 +138,7 @@ bool celix_scheduledEvent_isMarkedForRemoval(celix_scheduled_event_t* event); * @param[in] event The event to configure for wakeup. * @return The future call count of the event after the next processing is done. */ -size_t celix_scheduledEvent_configureWakeup(celix_scheduled_event_t* event); +size_t celix_scheduledEvent_markForWakeup(celix_scheduled_event_t* event); /** * @brief Wait for a scheduled event to reach at least the provided call count. diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c index 8232ed04..a6cc385d 100644 --- a/libs/framework/src/framework.c +++ b/libs/framework/src/framework.c @@ -2584,15 +2584,12 @@ long celix_framework_scheduleEvent(celix_framework_t* fw, return celix_scheduledEvent_getId(event); } -celix_status_t celix_framework_wakeupScheduledEvent(celix_framework_t* fw, - long scheduledEventId, - double waitTimeInSeconds) { - size_t callCountAfterWakeup; +celix_status_t celix_framework_wakeupScheduledEvent(celix_framework_t* fw, long scheduledEventId) { celixThreadMutex_lock(&fw->dispatcher.mutex); celix_scheduled_event_t* event = celix_longHashMap_get(fw->dispatcher.scheduledEvents, scheduledEventId); if (event != NULL) { - celix_scheduledEvent_retain(event); - callCountAfterWakeup = celix_scheduledEvent_configureWakeup(event); + celix_scheduledEvent_markForWakeup(event); + celixThreadCondition_broadcast(&fw->dispatcher.cond); //notify dispatcher thread for configured wakeup } celixThreadMutex_unlock(&fw->dispatcher.mutex); @@ -2604,21 +2601,7 @@ celix_status_t celix_framework_wakeupScheduledEvent(celix_framework_t* fw, return CELIX_ILLEGAL_ARGUMENT; } - celixThreadMutex_lock(&fw->dispatcher.mutex); - celixThreadCondition_broadcast(&fw->dispatcher.cond); //notify dispatcher thread for configured wakeup - celixThreadMutex_unlock(&fw->dispatcher.mutex); - - celix_status_t status = CELIX_SUCCESS; - if (waitTimeInSeconds > 0) { - if (celix_framework_isCurrentThreadTheEventLoop(fw)) { - fw_log(fw->logger, CELIX_LOG_LEVEL_WARNING, "celix_framework_wakeupScheduledEvent called from the " - "event loop thread. This can result in a deadlock!"); - } - status = celix_scheduledEvent_waitForAtLeastCallCount(event, callCountAfterWakeup, waitTimeInSeconds); - } - celix_scheduledEvent_release(event); - - return status; + return CELIX_SUCCESS; } celix_status_t diff --git a/libs/framework/src/framework_private.h b/libs/framework/src/framework_private.h index e9e5173b..bd37b6b9 100644 --- a/libs/framework/src/framework_private.h +++ b/libs/framework/src/framework_private.h @@ -475,20 +475,14 @@ long celix_framework_scheduleEvent(celix_framework_t* fw, void (*removeCallback)(void*)); /** - * @brief Wakeup a scheduled event. - * - * If waitTimeInSeconds is not 0, this function will block until the scheduled event callback is called. - * If waitTimeInSeconds is 0, this function will return immediately. + * @brief Wakeup a scheduled event and returns immediately, not waiting for the scheduled event callback to be + * called. * * @param[in] ctx The bundle context. * @param[in] scheduledEventId The scheduled event id to wakeup. - * @param[in] waitTimeInSeconds If not 0, this function will block until the scheduled event callback - * is called or the provided timeout is reached. * @return CELIX_SUCCESS if the scheduled event is woken up, CELIX_ILLEGAL_ARGUMENT if the scheduled event id is not */ -celix_status_t celix_framework_wakeupScheduledEvent(celix_framework_t* fw, - long scheduledEventId, - double waitTimeInSeconds); +celix_status_t celix_framework_wakeupScheduledEvent(celix_framework_t* fw, long scheduledEventId); /** * @brief Wait for the next scheduled event to be processed.
