ros2 / ros2/rcutils

Make logging functionality truly thread-safe

Open
#397 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

backlog
Dominant language
C
Stars
77
Forks
135
Avg merge
1d 22h
Merged PRs (30d)
10

Description

rcutils_logging_get_logger_level (which is called when doing any logging in ROS 2) has never been thread-safe with respect to rcutils_logging_set_logger_level.

However, prior to PR #381, rcutils_logging_get_logger_level was thread-safe with respect to itself. That is, it was only ever reading data, so it was safe for multiple threads to be reading that data at once.

PR #381 changed it so that we cached the results of calculating the logging level and stored that back in the hashmap. While that is an important optimization, it also caused rcutils_logging_get_logger_level to not be thread-safe with respect to itself. So we disabled that optimization in #393.

However, we really should re-enable that optimization and also make rcutils_logging_get_logger_level thread-safe with respect to rcutils_logging_set_logger_level. To do that, what we need to do is to add a reader/writer lock around the hashmap. That will allow the very common case of multiple threads calling rcutils_logging_get_logger_level to continue to work like today (holding a read-lock), while also making it so that the less common case of either setting the log level or updating the cache temporarily blocks readers (holding a write lock). The wrinkle here is that all of this is in C, so we need to come up with some cross-platform way to do this. It needs a bit of thought; we may be able to get away with only using atomics here.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with include/rcutils/logging.h and the rcutils_logging_get_logger_level and rcutils_logging_set_logger_level entry points. Review PRs #381 and #393 to understand the cache change and its rollback. Done means restoring cached level computation while keeping concurrent reads safe and coordinating reads with level updates across supported platforms.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
operating-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.