-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ros-pull-request-builder retest this please |
There was a problem hiding this 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 👍
There was a problem hiding this 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:
topic_description = participant->lookup_topicdescription(topic_str); |
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.
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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). |
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. |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
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:
though it was extremely hard to reproduce (I was never able to reproduce it, but @jacobperron was able to).
The failure looked like:
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.