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

Cleanup rmw publisher/subscription on exception #553

Merged
merged 2 commits into from
May 4, 2020

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Apr 30, 2020

As mentioned at ros2/ros2cli#500 (comment) and ros2/ros2cli#500 (comment), when create_publisher()/create_subscription() fails to register QoS event callbacks, there should not remain a dangling rmw_publisher_t/rmw_subscription_t instance until Python performs garbage collection.

This pull request changes the behavior of create_publisher()/create_subscription() so that it eagerly destroys the just newly created rmw_publisher_t/rmw_subscription_t instance if an exception occurs.

…ion() fails

Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
Copy link
Member Author

mm318 commented May 1, 2020

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/mm318/2dcf03fee968a55c3fbb677f66146b1e/raw/a6e21f917b2793f1a1e33526bb739e09b4287827/ros2_create_pubsub.repos
BUILD args: --packages-up-to rclpy ros2topic
TEST args: --packages-select rclpy ros2topic
Job: ci_launcher

Can you also please additionally run with RMW_IMPLEMENTATION=rmw_connext_cpp and RMW_IMPLEMENTATION=rmw_cyclonedds_cpp?

Note: This .repos gist also includes changes for ros2/ros2cli#496.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Thanks @mm318 !

@ivanpauno
Copy link
Member

See CI jobs here ros2/ros2cli#496 (comment).

@ivanpauno
Copy link
Member

Merging this one, thanks for the fix @mm318 !

@ivanpauno ivanpauno merged commit 827849e into ros2:master May 4, 2020
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.

2 participants