Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Fix topic creation race condition#442

Merged
ivanpauno merged 5 commits intomasterfrom
ivanpauno/topic-creation-race-condition
Jul 20, 2020
Merged

Fix topic creation race condition#442
ivanpauno merged 5 commits intomasterfrom
ivanpauno/topic-creation-race-condition

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented Jul 15, 2020

We were checking if the topic was already creating and then creating it if not.
There's a TOCTTOU race condition here.

To avoid the problem, I've added a mutex for each participant, and locking it when creating a topic.

The race condition could be experienced when launching gazebo:

ros2 launch gazebo_ros gzserver.launch.py

though it was extremely hard to reproduce (I was never able to reproduce it, but @jacobperron was able to).

The failure looked like:

  • publisher failing
    [gzserver-1] terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
    [gzserver-1]   what():  could not create publisher: failed to create topic '/clock' for node namespace='/' name='gazebo', at /home/jacob/ws/ros/gazebo_ros/src/ros2/rmw_connext/rmw_connext_cpp/src/rmw_publisher.cpp:208, at /home/jacob/ws/ros/gazebo_ros/src/ros2/rcl/rcl/src/rcl/publisher.c:172
    [gzserver-1] Aborted (core dumped)
  • subscription failing
[gzserver-1] terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
[gzserver-1]   what():  could not create subscription: failed to create topic '/clock' for node namespace='/' name='gazebo', at /home/jacob/ws/ros/gazebo_ros/src/ros2/rmw_connext/rmw_connext_cpp/src/rmw_subscription.cpp:202, at /home/jacob/ws/ros/gazebo_ros/src/ros2/rcl/rcl/src/rcl/subscription.c:168

I haven't confirmed that gazebo is trying to create the publisher and subscription from different threads, but it sounds like that was the case.

@ivanpauno
Fix topic creation race condition
c04b063 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added bug Something isn't working in review Waiting for review (Kanban column) labels Jul 15, 2020
@ivanpauno ivanpauno self-assigned this Jul 15, 2020
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 15, 2020 •
edited
Loading

I was curious if there was a similar race condition for services and clients.
That code lives here and here, and there doesn't seem to be any race condition in that case 😃 .

@ivanpauno
Please linters
e23d75d 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno closed this Jul 15, 2020
@ivanpauno ivanpauno reopened this Jul 15, 2020
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 15, 2020

@ros-pull-request-builder retest this please

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 15, 2020

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

jacobperron
jacobperron reviewed Jul 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have fixed the issue for me 👍

clalancette
clalancette suggested changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this same fix in the dynamic version of the code as well:

rmw_connext/rmw_connext_dynamic_cpp/src/functions.cpp

Line 497 in da982d1

topic_description = participant->lookup_topicdescription(topic_str);
and

rmw_connext/rmw_connext_dynamic_cpp/src/functions.cpp

Line 497 in da982d1

topic_description = participant->lookup_topicdescription(topic_str);

I know we don't really build or test it, but since we are in here fixing this we may as well do the same there.

ivanpauno added 3 commits July 16, 2020 10:01
@ivanpauno
Check if topic was correctly created
a55098b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Similar changes to dynamic
79aa945 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Document preconditions of rmw_connext_shared_cpp::create_topic
a0c3fe1 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 16, 2020

I think we need this same fix in the dynamic version of the code as well:

Done in 79aa945.

I really don't think we should continue updating rmw_connext_dynamic, it is extremely outdated.

IMO, if we want to support it again, it will be easier to rewrite than update it (the only piece of useful code there is how to create a DDS type code from a ROS typesupport and how to convert between ROS and DDS messages with the introspection typesupport).

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Jul 16, 2020

I really don't think we should continue updating rmw_connext_dynamic, it is extremely outdated.

IMO, if we want to support it again, it will be easier to rewrite than update it (the only piece of useful code there is how to create a DDS type code from a ROS typesupport and how to convert between ROS and DDS messages with the introspection typesupport).

Yeah, I basically agree here. Personally I'd just remove the code from the repository altogether. If anyone wants to revive it, it would be in git history to serve as a starting point. I'll bring it up at the next meeting.

clalancette
clalancette approved these changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll suggest letting @jacobperron take one more look at it before merging.

jacobperron
jacobperron approved these changes Jul 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivanpauno ivanpauno merged commit dc3f59a into master Jul 20, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/topic-creation-race-condition branch July 20, 2020 13:22
@ivanpauno ivanpauno mentioned this pull request Jul 24, 2020
Merged
This was referenced Sep 23, 2020
Merged
Merged
@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented Oct 1, 2020

This change (and the follow-up fix in #444) are not ABI compatible due to changes to publicly accessible structs. I'm not sure of a nice way to make them backwards compatible so I'm removing from the Foxy board.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@clalancette clalancette clalancette approved these changes

@jacobperron jacobperron jacobperron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanpauno ivanpauno

Labels

bug Something isn't working in review Waiting for review (Kanban column)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ivanpauno @clalancette @jacobperron