-
Notifications
You must be signed in to change notification settings - Fork 224
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
Remove global logging mutex #713
Conversation
Reverts most of #562 ros2/rclcpp#1042 describes a crash that can occur in rclcpp when rcl logging functions are called in different threads. This was fixed in ros2/rclcpp#1125, and a similar fix was made for rclpy in #562. This fix is unnecessary in rclpy because it cannot call the logging macros from multiple threads unless the GIL is released. The only place the GIL is released is around rcl_wait(), so the logging methods are already protected. Removing this code makes it a little easier to divide the remaining work of porting _rclpy.c to pybind11. If for some reason we decide to release the GIL around logging methods in the future, then they can be protected in the future using `pybind11::call_guard<T>` with a type that locks a global logging mutex when it is default constructed and unlocks it when its destructed. Signed-off-by: Shane Loretz <sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Some rmw implementations create a background thread that can call the logging macros, so IMO reverting #562 isn't safe. |
And we might even be calling logging macros from some dds listener callbacks, which are called from threads controlled by the dds library. |
I don't think #562 helps. For it to be safe, the rmw implementations would have to acquire rclpy's logging mutex.
Same problem - the dds listener callbacks would have to acquire rclpy's logging mutex or the rclpy logging calls could happen at the same time as the dds listener callbacks. |
would they? rclpy/rclpy/src/rclpy/_rclpy.c Lines 658 to 661 in 3de8f43
|
We could also try to make sure that we're not calling the logging macros from any background thread (which sounds as a good idea anyway), but without that this doesn't seem to be safe. |
Ah you're right. That's how the mutex gets acquired. |
Reverts most of #562
ros2/rclcpp#1042 describes a crash that can occur in rclcpp when rcl
logging functions are called in different threads. This was fixed in
ros2/rclcpp#1125, and a similar fix was made for rclpy in #562.
This fix is unnecessary in rclpy because it cannot call the logging
macros from multiple threads unless the GIL is released. The only place
the GIL is released is around rcl_wait(), so the logging methods are
already protected.
Removing this code makes it a little easier to divide the remaining work
of porting _rclpy.c to pybind11 (#665). If for some reason we decide to release
the GIL around logging methods in the future, then they can be protected
in the future using
pybind11::call_guard<T>
with a type that locks aglobal logging mutex when it is default constructed and unlocks it when
its destructed.
Signed-off-by: Shane Loretz sloretz@openrobotics.org
Signed-off-by: Shane Loretz sloretz@osrfoundation.org