PengZheng commented on code in PR #583: URL: https://github.com/apache/celix/pull/583#discussion_r1247368794
########## libs/framework/include/celix/ScheduledEvent.h: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include "celix_bundle_context.h" + +namespace celix { + +/** + * @brief A C++ abstraction for a scheduled event in Celix. + * + * A scheduled event is an event that is scheduled to be executed at a certain initial delay and/or interval. + * A new scheduld event should be created using celix::BundleContext::createScheduledEvent. + * + * 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. + */ +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; + + /** + * @brief Destroys the scheduled event by removes it from the bundle context if it is not a one-short event. + */ + ~ScheduledEvent() noexcept { + if (!isOneShot) { + celix_bundleContext_tryRemoveScheduledEventAsync(ctx.get(), eventId); + } + } + + ScheduledEvent(const ScheduledEvent&) = delete; + ScheduledEvent& operator=(const ScheduledEvent&) = delete; + + ScheduledEvent(ScheduledEvent&&) noexcept = default; + ScheduledEvent& operator=(ScheduledEvent&&) noexcept = default; + + /** + * @brief Cancels the scheduled event. + * + * This method will block until a possible in-progress scheduled event callback is finished, the scheduled event + * is removed and, if configured, the remove callback is called. + * Should not be called multiple times. + */ + void cancel() { + if (ctx) { + celix_bundleContext_removeScheduledEvent(ctx.get(), eventId); + } + } + + /** + * @brief Wakeup a scheduled event and returns immediately, not waiting for the scheduled event callback to be + * called. + */ + void wakeup() { celix_bundleContext_wakeupScheduledEvent(ctx.get(), eventId); } Review Comment: Missing ctx check? ########## libs/framework/include/celix_bundle_context.h: ########## @@ -1073,10 +1073,10 @@ typedef struct celix_bundle_tracker_options { void (*trackerCreatedCallback)(void *trackerCreatedCallbackData) CELIX_OPTS_INIT; } celix_bundle_tracking_options_t; -/** +#ifndef __cplusplus +/*! Review Comment: Is there any reason to change to the qt style? ########## libs/framework/src/framework.c: ########## @@ -1185,6 +1199,9 @@ static void* framework_shutdown(void *framework) { //NOTE possible starvation. fw_bundleEntry_waitTillUseCountIs(entry, 1); //note this function has 1 use count. + //note race between condition (use count == 1) and bundle stop, meaning use count can be > 1 when + //celix_framework_stopBundleEntry is called. Review Comment: IMO, the whole `for` can be removed altogether, since all state transitions can be done reliably within `celix_framework_uninstallBundleEntry` now. ########## libs/framework/include/celix/ScheduledEvent.h: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include "celix_bundle_context.h" + +namespace celix { + +/** + * @brief A C++ abstraction for a scheduled event in Celix. + * + * A scheduled event is an event that is scheduled to be executed at a certain initial delay and/or interval. + * A new scheduld event should be created using celix::BundleContext::createScheduledEvent. + * + * 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. + */ +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; + + /** + * @brief Destroys the scheduled event by removes it from the bundle context if it is not a one-short event. + */ + ~ScheduledEvent() noexcept { + if (!isOneShot) { + celix_bundleContext_tryRemoveScheduledEventAsync(ctx.get(), eventId); Review Comment: Missing ctx nullity check? ########## libs/framework/src/framework.c: ########## @@ -2391,14 +2535,29 @@ celix_array_list_t* celix_framework_listInstalledBundles(celix_framework_t* fram return celix_framework_listBundlesInternal(framework, false); } -void celix_framework_waitForEmptyEventQueue(celix_framework_t *fw) { +celix_status_t celix_framework_waitForEmptyEventQueueFor(celix_framework_t *fw, double periodInSeconds) { assert(!celix_framework_isCurrentThreadTheEventLoop(fw)); + celix_status_t status = CELIX_SUCCESS; + struct timespec absTimeout = celixThreadCondition_getDelayedTime(periodInSeconds); celixThreadMutex_lock(&fw->dispatcher.mutex); - while (fw->dispatcher.eventQueueSize > 0 || celix_arrayList_size(fw->dispatcher.dynamicEventQueue) > 0) { - celixThreadCondition_wait(&fw->dispatcher.cond, &fw->dispatcher.mutex); + while (celix_framework_eventQueueSize(fw) > 0) { Review Comment: It takes all events including framework (not associated with a bundle) events into account, while `celix_framework_waitForEmptyEventQueue` only count bundle events. Shall we unify them? ########## libs/framework/src/framework_private.h: ########## @@ -437,4 +447,83 @@ celix_status_t celix_framework_uninstallBundleEntry(celix_framework_t* fw, celix */ celix_status_t celix_framework_updateBundleEntry(celix_framework_t* fw, celix_framework_bundle_entry_t* bndEntry, const char* updatedBundleUrl); + +/** @brief Return the next scheduled event id. + * @param[in] fw The Celix framework + * @return The next scheduled event id. + */ +long celix_framework_nextScheduledEventId(framework_t *fw); + +/** + * @brief Add a scheduled event to the Celix framework. + * + * + * @param[in] fw The Celix framework + * @param[in] bndId The bundle id to add the scheduled event for. If < 0 the framework bundle is used. + * @param[in] eventName The event name to use for the scheduled event. If NULL, a default event name is used. + * @param[in] initialDelayInSeconds The initial delay in seconds before the first event callback is called. + * @param[in] intervalInSeconds The interval in seconds between event callbacks. + * @param[in] eventData The event data to pass to the event callback. + * @param[in] eventCallback The event callback to call when the scheduled event is triggered. + * @return The scheduled event id of the scheduled event. Can be used to cancel the event. + * @retval <0 If the event could not be added. + */ +long celix_framework_scheduleEvent(celix_framework_t* fw, + long bndId, + const char* eventName, + double initialDelayInSeconds, + double intervalInSeconds, + void* callbackData, + void (*callback)(void*), + void* removeCallbackData, + void (*removeCallback)(void*)); + +/** + * @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. + * @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); + +/** + * @brief Wait for the next scheduled event to be processed. + * @param[in] fw The Celix framework + * @param[in] scheduledEventId The scheduled event id to wait for. + * @param[in] waitTimeInSeconds The maximum time to wait for the next scheduled event. If <= 0 the function will return + * immediately. + * @return CELIX_SUCCESS if the scheduled event is woken up, CELIX_ILLEGAL_ARGUMENT if the scheduled event id is not + * known and ETIMEDOUT if the waitTimeInSeconds is reached. + */ +celix_status_t celix_framework_waitForScheduledEvent(celix_framework_t* fw, + long scheduledEventId, + double waitTimeInSeconds); + +/** + * @brief Cancel a scheduled event. + * + * When this function returns, no more scheduled event callbacks will be called. + * + * @param[in] fw The Celix framework + * @param[in] async If true, the scheduled event will be cancelled asynchronously and the function will not block. + * @param[in] errorIfNotFound If true, removal of a non existing scheduled event id will not be logged. Review Comment: Should it be the opposite? ########## libs/framework/src/framework.c: ########## @@ -1185,6 +1199,9 @@ static void* framework_shutdown(void *framework) { //NOTE possible starvation. fw_bundleEntry_waitTillUseCountIs(entry, 1); //note this function has 1 use count. + //note race between condition (use count == 1) and bundle stop, meaning use count can be > 1 when + //celix_framework_stopBundleEntry is called. Review Comment: Just noticed that the classical `ShellTestSuite.quitTest` bug did not bite us in the last two months. If it were solved by our refactoring efforts, I won't be too surprised. ########## libs/framework/src/framework.c: ########## @@ -1408,32 +1426,153 @@ static inline bool fw_removeTopEventFromQueue(celix_framework_t* fw) { static inline void fw_handleEvents(celix_framework_t* framework) { celixThreadMutex_lock(&framework->dispatcher.mutex); int size = framework->dispatcher.eventQueueSize + celix_arrayList_size(framework->dispatcher.dynamicEventQueue); - if (size == 0 && framework->dispatcher.active) { - celixThreadCondition_timedwaitRelative(&framework->dispatcher.cond, &framework->dispatcher.mutex, 1, 0); - } - size = framework->dispatcher.eventQueueSize + celix_arrayList_size(framework->dispatcher.dynamicEventQueue); celixThreadMutex_unlock(&framework->dispatcher.mutex); while (size > 0) { celix_framework_event_t* topEvent = fw_topEventFromQueue(framework); fw_handleEventRequest(framework, topEvent); - bool dynamiclyAllocatedEvent = fw_removeTopEventFromQueue(framework); + bool dynamicallyAllocatedEvent = fw_removeTopEventFromQueue(framework); if (topEvent->bndEntry != NULL) { celix_framework_bundleEntry_decreaseUseCount(topEvent->bndEntry); } free(topEvent->serviceName); - if (dynamiclyAllocatedEvent) { + if (dynamicallyAllocatedEvent) { free(topEvent); } celixThreadMutex_lock(&framework->dispatcher.mutex); size = framework->dispatcher.eventQueueSize + celix_arrayList_size(framework->dispatcher.dynamicEventQueue); - celixThreadCondition_broadcast(&framework->dispatcher.cond); celixThreadMutex_unlock(&framework->dispatcher.mutex); } } +/** + * @brief Process all scheduled events. + */ +static double celix_framework_processScheduledEvents(celix_framework_t* fw) { + struct timespec ts = celixThreadCondition_getTime(); + + double nextClosestScheduledEvent; + celix_scheduled_event_t* callEvent; + celix_scheduled_event_t* removeEvent; + do { + nextClosestScheduledEvent = -1; //negative means no event next event + callEvent = NULL; + removeEvent = NULL; + double nextEvent; + 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)) { + removeEvent = visit; + celix_longHashMap_remove(fw->dispatcher.scheduledEvents, celix_scheduledEvent_getId(visit)); + break; + } + + bool call = celix_scheduledEvent_deadlineReached(visit, &ts, &nextEvent); + if (nextClosestScheduledEvent < 0 || nextEvent < nextClosestScheduledEvent) { + nextClosestScheduledEvent = nextEvent; + } + if (call) { + callEvent = visit; + if (celix_scheduledEvent_isSingleShot(visit)) { + removeEvent = visit; + celix_longHashMap_remove(fw->dispatcher.scheduledEvents, celix_scheduledEvent_getId(visit)); + } + break; + } + } + celixThreadMutex_unlock(&fw->dispatcher.mutex); + + if (callEvent != NULL) { + celix_scheduledEvent_process(callEvent, &ts); + } + if (removeEvent != NULL) { + const char* formatStr = celix_scheduledEvent_isSingleShot(removeEvent) ? + "Removing processed one-shot scheduled event '%s' (id=%li) for bundle if %li.": + "Removing processed scheduled event '%s' (id=%li) for bundle if %li."; + fw_log(fw->logger, + CELIX_LOG_LEVEL_DEBUG, + formatStr, Review Comment: Does format string checking still work this way? ########## libs/framework/src/framework.c: ########## @@ -2440,6 +2599,127 @@ void celix_framework_waitUntilNoPendingRegistration(celix_framework_t* fw) celixThreadMutex_unlock(&fw->dispatcher.mutex); } +long celix_framework_scheduleEvent(celix_framework_t* fw, + long bndId, + const char* eventName, + double initialDelayInSeconds, + double intervalInSeconds, + void* callbackData, + void (*callback)(void*), + void* removeCallbackData, + void (*removeCallback)(void*)) { + if (callback == NULL) { + fw_log(fw->logger, + CELIX_LOG_LEVEL_ERROR, + "Cannot add scheduled event for bundle id %li. Invalid NULL event callback.", + bndId); + return -1; + } + + celix_framework_bundle_entry_t* bndEntry = celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(fw, bndId); + if (bndEntry == NULL) { + fw_log(fw->logger, CELIX_LOG_LEVEL_ERROR, "Cannot add scheduled event for non existing bundle id %li.", bndId); + return -1; + } + celix_scheduled_event_t* newEvent = celix_scheduledEvent_create(fw, + bndEntry->bndId, + celix_framework_nextScheduledEventId(fw), + eventName, + initialDelayInSeconds, + intervalInSeconds, + callbackData, + callback, + removeCallbackData, + removeCallback); + CELIX_SCHEDULED_EVENT_RETAIN_GUARD(event, newEvent); + celix_framework_bundleEntry_decreaseUseCount(bndEntry); + + if (event == NULL) { + return -1L; //error logged by celix_scheduledEvent_create + } + + fw_log(fw->logger, + CELIX_LOG_LEVEL_DEBUG, + "Added scheduled event '%s' (id=%li) for bundle '%s' (id=%li).", + celix_scheduledEvent_getName(event), + celix_scheduledEvent_getId(event), + celix_bundle_getSymbolicName(bndEntry->bnd), + bndId); + + celixThreadMutex_lock(&fw->dispatcher.mutex); + celix_longHashMap_put(fw->dispatcher.scheduledEvents, celix_scheduledEvent_getId(event), event); + celixThreadCondition_broadcast(&fw->dispatcher.cond); //notify dispatcher thread for newly added scheduled event + celixThreadMutex_unlock(&fw->dispatcher.mutex); + + return celix_scheduledEvent_getId(event); +} + +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_markForWakeup(event); + celixThreadCondition_broadcast(&fw->dispatcher.cond); //notify dispatcher thread for configured wakeup + } + celixThreadMutex_unlock(&fw->dispatcher.mutex); + + if (event == NULL) { + fw_log(fw->logger, + CELIX_LOG_LEVEL_WARNING, + "celix_framework_wakeupScheduledEvent called with unknown scheduled event id %li.", + scheduledEventId); + return CELIX_ILLEGAL_ARGUMENT; + } + + return CELIX_SUCCESS; +} + +celix_status_t +celix_framework_waitForScheduledEvent(celix_framework_t* fw, long scheduledEventId, double waitTimeInSeconds) { + celixThreadMutex_lock(&fw->dispatcher.mutex); + CELIX_SCHEDULED_EVENT_RETAIN_GUARD(event, celix_longHashMap_get(fw->dispatcher.scheduledEvents, scheduledEventId)); + celixThreadMutex_unlock(&fw->dispatcher.mutex); + + if (event == NULL) { Review Comment: What if a one-shot event is processed and removed here? I think the next `wait` will block forever. Similar thinking also applies to normal events about to be removed. In `wait`, we should also check `isRemoved`? ########## libs/framework/include/celix_framework.h: ########## @@ -318,53 +317,83 @@ CELIX_FRAMEWORK_EXPORT celix_array_list_t* celix_framework_listBundles(celix_fra */ CELIX_FRAMEWORK_EXPORT celix_array_list_t* celix_framework_listInstalledBundles(celix_framework_t* framework); +/** + * @brief Sets the log function for this framework. + * Default the celix framework will log to stdout/stderr. + * + * A log function can be injected to change how the Celix framework logs. + * Can be reset by setting the log function to NULL. + */ +CELIX_FRAMEWORK_EXPORT void celix_framework_setLogCallback(celix_framework_t* fw, void* logHandle, void (*logFunction)(void* handle, celix_log_level_e level, const char* file, const char *function, int line, const char *format, va_list formatArgs)); + /** * @brief Wait until the framework event queue is empty. * * The Celix framework has an event queue which (among others) handles bundle events. - * This function can be used to ensure that all queue event are handled, mainly useful - * for testing. + * This function can be used to ensure that all queue event are handled. + * + * Note scheduled events are not part of the event queue. * * @param fw The Celix Framework */ CELIX_FRAMEWORK_EXPORT void celix_framework_waitForEmptyEventQueue(celix_framework_t *fw); /** - * @brief Sets the log function for this framework. - * Default the celix framework will log to stdout/stderr. + * @brief Wait until the framework event queue is empty or the provided period is reached. * - * A log function can be injected to change how the Celix framework logs. - * Can be reset by setting the log function to NULL. + * The Celix framework has an event queue which (among others) handles bundle events. + * This function can be used to ensure that all queue event are handled. + * + * Note scheduled events are not part of the event queue. + * + * @param[in] fw The Celix Framework. + * @param[in] timeoutInSeconds The period in seconds to wait for the event queue to be empty. 0 means wait forever. + * @return CELIX_SUCCESS if the event queue is empty or ETIMEDOUT if the timeoutInSeconds is reached. */ -CELIX_FRAMEWORK_EXPORT void celix_framework_setLogCallback(celix_framework_t* fw, void* logHandle, void (*logFunction)(void* handle, celix_log_level_e level, const char* file, const char *function, int line, const char *format, va_list formatArgs)); - +CELIX_FRAMEWORK_EXPORT celix_status_t celix_framework_waitForEmptyEventQueueFor(celix_framework_t *fw, double timeoutInSeconds); /** - * @brief wait until all events for the bundle identified by the bndId are processed. + * @brief wait until all events from the event queue for the bundle identified by the bndId are processed. + * + * If bndId < 0, wait until all events from the event queue are processed. + * Note scheduled events are not part of the event queue. + * */ CELIX_FRAMEWORK_EXPORT void celix_framework_waitUntilNoEventsForBnd(celix_framework_t* fw, long bndId); /** - * @brief wait until all pending service registration are processed. + * @brief wait until all pending service registration are processed. */ CELIX_FRAMEWORK_EXPORT void celix_framework_waitUntilNoPendingRegistration(celix_framework_t* fw); /** * @brief Returns whether the current thread is the Celix framework event loop thread. + * + * Note scheduled events are not part of the event queue. Review Comment: It seems irrelevant. ########## libs/framework/src/framework_private.h: ########## @@ -437,4 +447,83 @@ celix_status_t celix_framework_uninstallBundleEntry(celix_framework_t* fw, celix */ celix_status_t celix_framework_updateBundleEntry(celix_framework_t* fw, celix_framework_bundle_entry_t* bndEntry, const char* updatedBundleUrl); + +/** @brief Return the next scheduled event id. + * @param[in] fw The Celix framework + * @return The next scheduled event id. + */ +long celix_framework_nextScheduledEventId(framework_t *fw); + +/** + * @brief Add a scheduled event to the Celix framework. + * + * + * @param[in] fw The Celix framework + * @param[in] bndId The bundle id to add the scheduled event for. If < 0 the framework bundle is used. Review Comment: If < 0, `celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount` returns NULL, and -1 will be returned. ########## libs/framework/include/celix_framework.h: ########## @@ -318,53 +317,83 @@ CELIX_FRAMEWORK_EXPORT celix_array_list_t* celix_framework_listBundles(celix_fra */ CELIX_FRAMEWORK_EXPORT celix_array_list_t* celix_framework_listInstalledBundles(celix_framework_t* framework); +/** + * @brief Sets the log function for this framework. + * Default the celix framework will log to stdout/stderr. + * + * A log function can be injected to change how the Celix framework logs. + * Can be reset by setting the log function to NULL. + */ +CELIX_FRAMEWORK_EXPORT void celix_framework_setLogCallback(celix_framework_t* fw, void* logHandle, void (*logFunction)(void* handle, celix_log_level_e level, const char* file, const char *function, int line, const char *format, va_list formatArgs)); + /** * @brief Wait until the framework event queue is empty. * * The Celix framework has an event queue which (among others) handles bundle events. - * This function can be used to ensure that all queue event are handled, mainly useful - * for testing. + * This function can be used to ensure that all queue event are handled. + * + * Note scheduled events are not part of the event queue. * * @param fw The Celix Framework */ CELIX_FRAMEWORK_EXPORT void celix_framework_waitForEmptyEventQueue(celix_framework_t *fw); /** - * @brief Sets the log function for this framework. - * Default the celix framework will log to stdout/stderr. + * @brief Wait until the framework event queue is empty or the provided period is reached. * - * A log function can be injected to change how the Celix framework logs. - * Can be reset by setting the log function to NULL. + * The Celix framework has an event queue which (among others) handles bundle events. + * This function can be used to ensure that all queue event are handled. + * + * Note scheduled events are not part of the event queue. + * + * @param[in] fw The Celix Framework. + * @param[in] timeoutInSeconds The period in seconds to wait for the event queue to be empty. 0 means wait forever. + * @return CELIX_SUCCESS if the event queue is empty or ETIMEDOUT if the timeoutInSeconds is reached. */ -CELIX_FRAMEWORK_EXPORT void celix_framework_setLogCallback(celix_framework_t* fw, void* logHandle, void (*logFunction)(void* handle, celix_log_level_e level, const char* file, const char *function, int line, const char *format, va_list formatArgs)); - +CELIX_FRAMEWORK_EXPORT celix_status_t celix_framework_waitForEmptyEventQueueFor(celix_framework_t *fw, double timeoutInSeconds); /** - * @brief wait until all events for the bundle identified by the bndId are processed. + * @brief wait until all events from the event queue for the bundle identified by the bndId are processed. + * + * If bndId < 0, wait until all events from the event queue are processed. Review Comment: Not all events, just events associated with bundles, i.e. not including framework events. -- 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: dev-unsubscr...@celix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
