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 typesupport loading #400

Merged
merged 4 commits into from
May 7, 2020
Merged

Fix typesupport loading #400

merged 4 commits into from
May 7, 2020

Conversation

Karsten1987
Copy link
Collaborator

The change to rcpputils::SharedLibrary in #322 introduced a regression to the converter logic.

copied from #322

I haven't paid enough attention while reviewing this PR, but I think it's really bad practice to pass in a shared_pointer by reference (std::shared_pointerrcpputils::SharedLibrary & library) and somewhat blindly overwrite it by calling std::make_shared on it. Also, this assumes that the pointer is empty, which isn't checked here either.
The reason why it fails in the end is that the same pointer is being passed in to load multiple typesupports, which are stored in add_topics. Later on, when using these typesupport symbols to create a new introspection message, these pointers have dangled.

This PR is fixing this by extending the ConverterTypeSupport struct with the shared library. It is currently using a shared pointer to it, even though I don't understand why this is needed and a reference isn't sufficient. Maybe @ahcorde can give some more insights here.

This is needed to unblock ros2/rosbag2_bag_v2#28

Signed-off-by: Knese Karsten <karsten@openrobotics.org>
@Karsten1987 Karsten1987 self-assigned this May 2, 2020
@Karsten1987 Karsten1987 added the bug Something isn't working label May 2, 2020
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Knese Karsten <karsten@openrobotics.org>
@Karsten1987
Copy link
Collaborator Author

Karsten1987 commented May 2, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Knese Karsten <karsten@openrobotics.org>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator Author

That is up for review.

I am still a bit concerned about this as every introspection message is now strongly coupled with the rcpputils::SharedLibrary in order to be cleaned up correctly. However, this would need a bigger round of code review and in order to get the beta going and the rosbag2_bag_v2_plugin unblocked, this should be good to go.

@Karsten1987
Copy link
Collaborator Author

@emersonknapp @zmichaels11 friendly ping for review

@Karsten1987 Karsten1987 merged commit 5337067 into master May 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix_typesupport_loading branch May 7, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants