Rewrite how Topics are tracked in rmw_fastrtps_cpp. #669
Conversation
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>
|
CI:
|
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Show resolved
Hide resolved
|
Hi guys, I wanted to point out the existence of the DomainParticipant::find_topic() API. This API takes a topic name and returns a 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. |
Caught in CI. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
@clalancette @methylDragon I have been too busy today, but I promise to review this tomorrow. |
Thanks for the info. I did briefly consider using |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
Another CI with the latest fixes:
|
|
All right, CI is green and this is approved. Going ahead and merging this one, thanks for looking! |
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.