search

ros2 / rmw_fastrtps

Implementation of the ROS Middleware (rmw) Interface using eProsima's Fast RTPS.
Apache License 2.0
157 stars 117 forks source link

Rewrite how Topics are tracked in rmw_fastrtps_cpp. #669

Closed clalancette closed 1 year ago

clalancette commented 1 year ago

Every DataReader and DataWriter in the system needs to have an associated Topic. Ideally we would create these as one per publisher/subscriber, but Fast-DDS does not allow you to create multiple topics within the same DomainParticipant with the same topic name. Instead, what we need to do is track ourselves whether a topic should be reused.

This was previously done with a combination of TopicHolder and cast_or_create_topic, but that had a couple of problems. First of all, a TopicHolder is really a SCOPE_EXIT by a different name. Second, cast_or_create_topic worked, but really left the semantics of who owned a Topic up in the air. If you created multiple topics with the same name, the first one would own it, and thus own the lifetime. The subsequent ones would reuse that. But if you happened to remove the first one, then everything would probably crash.

Rewrite this whole thing to instead store the list of Topic pointers in the CustomParticipantInfo, which seems like a much more appropriate place for it. When we go to add them, we first look if it already exists and if so, just increase the use_count. If it doesn't exist, we create it with a use_count of 1. On deletion, we do the opposite; decrease the use_count, and call delete_topic iff the use_count <= 0. This data is wrapped in another class called UseCountTopic; we'll also be using this in the future to associate TopicListener with the Topic.

With this implementation, the semantics are much more clearly defined, and this should be able to handle deletion as well. It also happens to remove a bunch of code from the implementation, which is an added bonus.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Once we get this in (or something like it), I'll be able to redo the code in #654 to take advantage of this refactor.

Still a draft while I run CI on it.

@MiguelCompany I could especially use review from you.

clalancette commented 1 year ago

CI:

EduPonz commented 1 year ago

Hi guys, I wanted to point out the existence of the DomainParticipant::find_topic() API. This API takes a topic name and returns a Topic corresponding with the name. If the topic had not yet been created, it waits for a max timeout duration until the topic is created by another thread. Important point is that all topics obtained by the means of find_topic must be deleted with delete_topic.

I can see that you already solve your need with some reference counting, but I wanted to leave this here in case it is of interest for you.

MiguelCompany commented 1 year ago

@clalancette @methylDragon I have been too busy today, but I promise to review this tomorrow.

clalancette commented 1 year ago

Hi guys, I wanted to point out the existence of the DomainParticipant::find_topic() API. This API takes a topic name and returns a Topic corresponding with the name. If the topic had not yet been created, it waits for a max timeout duration until the topic is created by another thread. Important point is that all topics obtained by the means of find_topic must be deleted with delete_topic.

Thanks for the info. I did briefly consider using find_topic, but the fact that I had to destroy it every time made me shy away from it.

clalancette commented 1 year ago

Another CI with the latest fixes:

clalancette commented 1 year ago

All right, CI is green and this is approved. Going ahead and merging this one, thanks for looking!