ros2 / rcpputils

Apache License 2.0
32 stars 54 forks source link

Add unique_lock implementation with clang thread safety annotations #180

Closed emersonknapp closed 1 year ago

emersonknapp commented 1 year ago

Can be used to replace rosbag2 implementation https://github.com/ros2/rosbag2/blob/rolling/rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp#L31 and false positive errors in RMW builds like https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1628/clang/new/source.cf52365c-c9dc-4c3e-8639-55ffa17e1b64/#115

emersonknapp commented 1 year ago

Probably needs a test

clalancette commented 1 year ago

And just for verification, it looks like using this instead of std::unique_lock in rmw_fastrtps makes a difference: https://github.com/ros2/rmw_fastrtps/pull/712#issuecomment-1701294278 (there are about 140 fewer violations than on e.g. https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1629/)

fujitatomoya commented 1 year ago

@clalancette that is actually good question. i was thinking that we can simulate https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1628/clang/new/source.cf52365c-c9dc-4c3e-8639-55ffa17e1b64/#115 but it actually is the test for RCPPUTILS_TSA_SCOPED_CAPABILITY.

we could test rcpputils::unique_lock works, but i dont think that is even necessary either.

clalancette commented 1 year ago

@emersonknapp When you get a chance, can you rebase this to fix the DCO bot?

emersonknapp commented 1 year ago

Added a simple compile-test that we can use multiple types of mutex.

emersonknapp commented 1 year ago

Pulls: ros2/rcpputils#180 Gist: https://gist.githubusercontent.com/emersonknapp/c4a008cb46d56f9548cffbcbaefac5a1/raw/22101fab9d965b83665b049428add3a1a64e682a/ros2.repos BUILD args: --packages-above-and-dependencies rcpputils TEST args: --packages-above rcpputils ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12608