rcl_configure_logging and rcl_logging_fini are called multiple times when creating multiple contexts

[ivanpauno]

Feature request

Feature description

Currently, if many contexts are created, rcl_logging_configure is called many times, overriding preexisting configurations silently.
Same happens with rcl_logging_fini. That destroys the logger when the first context is destructed.

The logger should be only initialized once, and initialization of future contexts should be ignored logging a warning (or handling the error somehow).

Implementation considerations

To avoid destructing the logger at the first call, an initialization count can be implemented.
The logger should be destroyed after rcl_logging_destroy was called the same times that init.

Note

Check if something similar may be happening with other "global" configurations.

[fujitatomoya]

@ivanpauno @wjwwood

let me take care of this, will get back to you.

Best

[fujitatomoya]

@ivanpauno

my thoughts are the followings,

• logging confiration is based on global arguments, so should be initalized and finalized once in the process space.
• rcl_logging_configure will be changed into rcl_logging_init to keep the name consistency for rcl_logging_fini.
• Thread-Safe is not supported, just to get along with current implementation.
• reference counter is provided to keep the consistency, how many times initialized and finalized.
• reference counter and implementation is declared in static scope in logging.

just let me know if i got anything missing and wrong.

Best,

[ivanpauno]

let me take care of this, will get back to you.

Sounds good, thanks!

logging confiration is based on global arguments, so should be initalized and finalized once in the process space.

Yeap, I agree. Alternativly, we could have some context dependent logging configs, but I don't think that's a good idea (and definetly, much more work).

rcl_logging_configure will be changed into rcl_logging_init to keep the name consistency for rcl_logging_fini.

That sounds reasonable, though it's not required. You can skip that in the first iteration, in order to minimize changes in upstream packages.

Thread-Safe is not supported, just to get along with current implementation.

I would ensure thread safety in logging init-fini functions, to avoid strange runtime errors.

reference counter is provided to keep the consistency, how many times initialized and finalized.
reference counter and implementation is declared in static scope in logging.

Sounds good. Besides that, it would be good to show a warning when logging is initialized more than once, passing different argv options. Maybe, just showing a warning when argv is not NULL and logging was already initialized.

[ivanpauno]

@fujitatomoya are you planning to contribute in this one?

[fujitatomoya]

@ivanpauno

yes, i am going to do it, i am positive that i can do that in this week.
if urgent, somebody else takes over.

it was just hard to squeeze time...sorry to be late to get back to you.

[fujitatomoya]

@ivanpauno

could you share your thoughts on #560 ?

i am not so sure that we need to ensure thread safe on this function, maybe warning should be enough.

[fujitatomoya]

@ivanpauno

i guess that we can close this since #560 is merged.

[ivanpauno]

Yes, #579 ros2/rclpy#518 and ros2/rclcpp#998 fixed this.

It would nice too modify the logger so that its configuration isn't global, but that's a different issue.