pnoltes commented on code in PR #780:
URL: https://github.com/apache/celix/pull/780#discussion_r1905920793
##########
libs/framework/src/framework.c:
##########
@@ -2686,14 +2704,26 @@ static bool
celix_framework_isGenericEventInProgress(celix_framework_t* fw, long
void celix_framework_waitForGenericEvent(celix_framework_t* fw, long eventId) {
assert(!celix_framework_isCurrentThreadTheEventLoop(fw));
- struct timespec logAbsTime = celixThreadCondition_getDelayedTime(5);
+ double logTimeoutInSeconds =
+ celix_framework_getConfigPropertyAsDouble(fw,
+
CELIX_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS,
+
CELIX_DEFAULT_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS,
+ NULL);
+ struct timespec logAbsTime =
celixThreadCondition_getDelayedTime(logTimeoutInSeconds);
celixThreadMutex_lock(&fw->dispatcher.mutex);
+ celix_framework_event_t* event = celix_framework_getGenericEvent(fw,
eventId);
Review Comment:
Note that `celix_framework_isGenericEventInProgress` checks whether the
event can be found in the eventQueue or dynamicEventQueue and return true if
found.
Imo the `celix_framework_isGenericEventInProgress` function can be removed
and instead a check for `if (event /*event exists, so in progress*/) {` can be
done.
##########
libs/framework/src/framework.c:
##########
@@ -2686,14 +2704,26 @@ static bool
celix_framework_isGenericEventInProgress(celix_framework_t* fw, long
void celix_framework_waitForGenericEvent(celix_framework_t* fw, long eventId) {
assert(!celix_framework_isCurrentThreadTheEventLoop(fw));
- struct timespec logAbsTime = celixThreadCondition_getDelayedTime(5);
+ double logTimeoutInSeconds =
+ celix_framework_getConfigPropertyAsDouble(fw,
+
CELIX_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS,
+
CELIX_DEFAULT_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS,
+ NULL);
+ struct timespec logAbsTime =
celixThreadCondition_getDelayedTime(logTimeoutInSeconds);
celixThreadMutex_lock(&fw->dispatcher.mutex);
+ celix_framework_event_t* event = celix_framework_getGenericEvent(fw,
eventId);
while (celix_framework_isGenericEventInProgress(fw, eventId)) {
celix_status_t waitStatus =
celixThreadCondition_waitUntil(&fw->dispatcher.cond,
&fw->dispatcher.mutex, &logAbsTime);
if (waitStatus == ETIMEDOUT) {
- fw_log(fw->logger, CELIX_LOG_LEVEL_WARNING, "Generic event with id
%li not finished.", eventId);
- logAbsTime = celixThreadCondition_getDelayedTime(5);
+ fw_log(fw->logger,
+ CELIX_LOG_LEVEL_WARNING,
+ "Generic event '%s' (id=%li) for bundle '%s' (id=%li) not
finished",
Review Comment:
Nice that some more info is printed about the event 👍
##########
libs/framework/src/framework.c:
##########
@@ -2686,14 +2704,26 @@ static bool
celix_framework_isGenericEventInProgress(celix_framework_t* fw, long
void celix_framework_waitForGenericEvent(celix_framework_t* fw, long eventId) {
assert(!celix_framework_isCurrentThreadTheEventLoop(fw));
- struct timespec logAbsTime = celixThreadCondition_getDelayedTime(5);
+ double logTimeoutInSeconds =
Review Comment:
Good to have this configureable, but in the current setup every call to
`celix_framework_waitForGenericEvent` will check the environment. I think this
is not needed, maybe retrieving this value during framework creating and store
it the framework structure. Note that if this is done during framework create
no mutex protection is needed, because the value will only be written once.
##########
libs/framework/gtest/src/CelixFrameworkTestSuite.cc:
##########
@@ -88,6 +88,34 @@ TEST_F(CelixFrameworkTestSuite, TimedWaitEventQueueTest) {
EXPECT_EQ(CELIX_SUCCESS, status);
}
+TEST_F(CelixFrameworkTestSuite, GenericEventTimeoutPropertyTest) {
Review Comment:
Nice that there is also a test :)
##########
libs/framework/include/celix_constants.h:
##########
@@ -281,6 +281,21 @@ extern "C" {
*/
#define CELIX_DEFAULT_ALLOWED_PROCESSING_TIME_FOR_SCHEDULED_EVENT_IN_SECONDS
2.0
+/*!
+ * @brief Celix framework environment property (named
"CELIX_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS")
+ * to configure the allowed processing time for a generic event callback or a
remove callback before a warning
+ * log message is printed that the processing time is too long.
+ * Should be a double value in seconds.
+ */
+#define CELIX_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS
\
Review Comment:
Could you also add this information to documents/framework.md and as C++
constexpr to libs/framework/include/celix/Constants.h
--
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
