Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the unicode test string for opensplice rmw implementation #447

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Oct 23, 2019

As the title says, this fixes a particular test for opensplice.
I don't think this is a proper solution for the issue, but it keeps CI happy. What I was able to understand and find, is that opensplice caches the topics that are created. I know we are using a deb, but I took a peek to their community edition and it seems that the errors matched just fine. You can see that error here.
As I was saying, when one creates a publisher/subscriber, it reaches a point where it creates a topic, calling this function

https://github.com/ADLINK-IST/opensplice/blob/c98e118a2e4366d2a5a6af8cbcecdf112bf9e4ab/src/kernel/code/v_topicImpl.c#L477

Specifically, this is part of our interest

https://github.com/ADLINK-IST/opensplice/blob/c98e118a2e4366d2a5a6af8cbcecdf112bf9e4ab/src/kernel/code/v_topicImpl.c#L527

Here, it makes use of a lookuptable for topics that lives within the kernel.

https://github.com/ADLINK-IST/opensplice/blob/c98e118a2e4366d2a5a6af8cbcecdf112bf9e4ab/src/kernel/code/v_topicImpl.c#L532

That function is the one that checks the types of the new topic and the one it found (since the names are the same).
Digging a little bit in the rmw implementation for the publisher, I saw that we do call a delete_topic function for the participant

https://github.com/ros2/rmw_opensplice/blob/f9997162920857a46a782ad1fbf7b876f6ba05bd/rmw_opensplice_cpp/src/rmw_publisher.cpp#L274

And it seems that there's no issue in "deleting" the topic, otherwise an error/exception should show.
When a node is destroyed, in rmw_destroy_node, I found a curious comment.

https://github.com/ros2/rmw_opensplice/blob/f9997162920857a46a782ad1fbf7b876f6ba05bd/rmw_opensplice_cpp/src/rmw_node.cpp#L381-L386

The interesting thing is that it seems that we are somewhat aware that some stuff could be in memory yet even if they shouldn't.
Does anyone have a clue if this is a known issue or we are missing a function call in the destroy_publisher function?

  • Opensplice jobs:
  • Linux Build Status
  • Arch Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Thanks for debugging this one @BMarchi

@BMarchi
Copy link
Contributor Author

BMarchi commented Oct 23, 2019

Should we call someone from opensplice to give us an insight of what's going on?

@hidmic
Copy link
Contributor

hidmic commented Oct 23, 2019

Should we call someone from opensplice to give us an insight of what's going on?

I think we should, but that fix won't land before Eloquent is out so let's merge this one!

@mjcarroll mjcarroll merged commit e37feab into master Oct 24, 2019
@delete-merged-branch delete-merged-branch bot deleted the BMarchi/fix_opensplice_test_unicode_string branch October 24, 2019 02:31
suab321321 pushed a commit to suab321321/rclpy that referenced this pull request Jan 31, 2020
* Fix the unicode test string for opensplice rmw implementation

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>

* Specify action in TODO comment

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: AbhinavSingh <singhabhinav9051571833@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants