-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand logging information when genericEvent is waiting #780
Expand logging information when genericEvent is waiting #780
Conversation
…the genericEventTimeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request. Nice improvement on the framework logging.
I do have some small remarks.
Add CELIX_ALLOWED_PROCESSING_TIME_FOR_GENERIC_EVENT_IN_SECONDS to documentation and C++ header. Set genericEventTimeout in start of framework modified: documents/framework.md modified: libs/framework/gtest/src/CelixFrameworkTestSuite.cc modified: libs/framework/include/celix/Constants.h modified: libs/framework/src/framework.c modified: libs/framework/src/framework_private.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some nitpick comment, but for the rest LGTM :).
fw_log(fw->logger, CELIX_LOG_LEVEL_WARNING, "Generic event with id %li not finished.", eventId); | ||
logAbsTime = celixThreadCondition_getDelayedTime(5); | ||
} | ||
fw_log(fw->logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential use-after-free. dispatcher.mutex
is release when waiting for dispatcher.cond
, and thus event
could be removed. The intention is good but we can not really do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice observability improvement. Note that I fixed a corner-case use-after-free.
LGTM
First time commit here,
When making use of celix, situations can occur that a fired genericEvent is not yet finished, resulting in multiple warning messages being printed in the logging. These messages contain very limited information on which event from which bundle is waiting.
Hence this pull request to update the warning message with more information, containing eventName, eventId, bundleName and bundleId. This should help in deducing which generic Events from which bundles are waiting a long time.
Also added an environment variable to support setting of the timeout of generic events, similar as to how scheduled events have the variable for setting the timeout in seconds.