Discovery race condition mitigation #174
Conversation
…e of max_blocking_time
|
@fujitatomoya Could you please run the CI for us? I don't have access to ROS 2 CI yet |
|
Pulls: #174
|
rmw_connextdds_common/include/rmw_connextdds/resource_limits.hpp
Outdated
Show resolved
Hide resolved
|
@fujitatomoya can you run the CI please? Hopefully the changes fix the instabilities. |
|
Pulls: #174
|
|
I think the pull request is ready to be merged. @fujitatomoya can you run the CI for us? |
There was a problem hiding this comment.
@fgallegosalido, As long as the tests pass, I’m fine with merging this Pull Request.
|
Pulls: #174
|
There was a problem hiding this comment.
these changes aim to enhance the functionality and reliability of the RMW_Connext_Publisher (service server response sender) and RMW_Connext_Subscriber (service client response reader), particularly in managing endpoint relationships and synchronization for the service. IMO, this really makes sense and probably mitigate the racy condition to miss the response message between service server and clients.
just a minor comment that i would like to confirm.
|
Could you help with another CI validation run? I have updated the branch to the latest rolling state since the last run failed due to a build issue with a package unrelated to rmw_connext. |
|
Pulls: #174
|
|
The In any case, I’ve also fixed the uncrustify formatting issues. Do we need to rerun the Lastly, thanks a lot for running the CI for us! Do you know who we should contact to get access to run the builds ourselves in the future? |
|
@lobolanja i will restart the CI. |
|
Pulls: #174
|
|
@fujitatomoya the Windows build was aborted, but the test issues that affected previous builds seem to be finished as per the Linux build. Are we okay to merge? |
|
@fgallegosalido test was aborted, so not completed. i just started the new CI for windows.
when this comes green, i believe that it is good to go. |
|
@fujitatomoya a couple of tests failed in Windows, but they don't seem related to the changes in this pr. Is it okay to merge now? |
|
windows warnings are unrelated and https://ci.ros2.org/job/ci_windows/23727/testReport/ros2bag.test/test_record_qos_profiles/test_record_qos_profiles/ has been addressed by ros2/rosbag2#1949. |
This yet another attempt at merging a mitigation for the discovery race condition. The first attempt happened in #132. This pull request has the same fix plus a couple of fixes and improvements.