Make logging functionality truly thread-safe #397
Comments
|
@clalancette thank you very much for the explanation!
yeah agree, that can be the best performative way. having rwlock object in the hashmap object and issue rwlock depends on the operation to protect the object.
agree, atomic can do the work but not performative in most reading cases as you mentioned. rcutils abstraction rwlock is work though. btw, AFAIK, mutex and rwlock, these kind of things have never been used in rcutils or rcl before, if i am not mistaken, we tried to avoid these implementation until now? so question is can we actually take this path? i would like to hear from others and history. |
@fujitatomoya @clalancette |
Yes, we have attempted to keep the locks out of rcutils and rcl prior to this. I think we could take that path, but I think @jrutgeer 's proposal is simpler. I'm going to comment over there as I have a couple of thoughts. |
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_levelwas 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_levelto 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_levelthread-safe with respect torcutils_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 callingrcutils_logging_get_logger_levelto 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.The text was updated successfully, but these errors were encountered: