-
Notifications
You must be signed in to change notification settings - Fork 42
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
Switch to custom logging macros to avoid deadlocks #223
Conversation
4223350
to
01cb3ce
Compare
Signed-off-by: Yadunund <[email protected]>
01cb3ce
to
85ab442
Compare
The problem with bringing in this dependency is that we'll have to deal with it on Windows. Also, in general, the more dependencies this has, the harder it will be to get
That's a really good idea, I'd prefer that. While it won't get us all of the features of the |
Thanks for the quick feedback! I'll update to rely on |
@clalancette I've updated accordingly. I've ran the problematic case from #182 several times and it works most of the time*. # terminal 1
ros2 run rclcpp_components component_container # terminal 2
for i in {1..100}; do echo "ros2 component load --node-namespace /talker${i} /ComponentManager composition composition::Talker"; done | parallel
|
41a919f
to
886b0e6
Compare
Signed-off-by: Yadunund <[email protected]>
886b0e6
to
c20ee54
Compare
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.
Overall, this looks great! I've left a couple of pieces of minor feedback inline.
One other thing we might want to consider is adding a GitHub action here to check if a PR contains RCUTILS_LOG_*
, or does #include <rcutils/logging_macros.h>
. That would help ensure that we don't add these by accident in the future.
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
2ff5d65
to
157f879
Compare
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.
This is fantastic! I'll note that the new check_logging.yaml
job is currently failing, because of course this PR does contain RCUTILS_LOG macros (that we are removing). It would be nice if there was a diffDoesNotAdd
configuration variable, but after this PR all further ones shouldn't have this problem. So I'm going ahead and approving this.
Address logging related deadlock discovered in #182.
@clalancette I went with a more C++ implementation here by relying on the popular fmt library which is apparently more performant than
printf
andostringstream
. Let me know if you prefer followingrcutils/logging.h
type of implementation with a specifiable allocator. We could also directly invoke rcutils_logging_console_output_handler.