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

Be much more careful about cleanup in the tf2_ros_py tests. #499

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

clalancette
Copy link
Contributor

In certain circumstances on Windows, we were seeing crashes
while the tf2_ros_py tests were running. After some investigation,
I found out that the tf2_ros_py buffer tests were not being
careful about when they cleaned up. This lead to the crashes.

What this change does is to make the cleanup in the tf2_ros_py
tests very explicit, which fixes the crashes on Windows in
my testing.

Arguably, this change is deeply un-Pythonic. Really the cleanup
should just happen naturally as part of the regular Python destruction.
There are a few reasons I think we should take this change anyway:

  1. Throughout the rest of the ROS 2 core, the Python code is very
    careful to cleanup after itself, as is done here. So I would
    say that the current problem is more of a design/low-level
    issue in rclpy than something here in tf2_ros_py
  2. Cleaning up like this is arguably "more correct" than waiting
    for the Python garbage collector to do it. That is, when
    we are no longer using an Action server or client, we really
    want it to be destructed ASAP, not eventually.
  3. We need this change in order to change the default DDS vendor
    to Fast-RTPS. This could be a bug in Fast-RTPS, though it
    is not totally clear to me where or how given the number of
    layers involved here (Python unittest -> tf2_ros_py -> rclpy
    -> rcl -> rmw -> rmw_fastrtps -> Fast-RTPS).

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

I believe this will unblock ros2/rmw#315 and ros2/rmw_fastrtps#571

In certain circumstances on Windows, we were seeing crashes
while the tf2_ros_py tests were running.  After some investigation,
I found out that the tf2_ros_py buffer tests were not being
careful about when they cleaned up.  This lead to the crashes.

What this change does is to make the cleanup in the tf2_ros_py
tests very explicit, which fixes the crashes on Windows in
my testing.

Arguably, this change is deeply un-Pythonic.  Really the cleanup
should just happen naturally as part of the regular Python destruction.
There are a few reasons I think we should take this change anyway:

1. Throughout the rest of the ROS 2 core, the Python code is very
   careful to cleanup after itself, as is done here.  So I would
   say that the current problem is more of a design/low-level
   issue in rclpy than something here in tf2_ros_py
2. Cleaning up like this is arguably "more correct" than waiting
   for the Python garbage collector to do it.  That is, when
   we are no longer using an Action server or client, we really
   want it to be destructed ASAP, not eventually.
3. We need this change in order to change the default DDS vendor
   to Fast-RTPS.  This could be a bug in Fast-RTPS, though it
   is not totally clear to me where or how given the number of
   layers involved here (Python unittest -> tf2_ros_py -> rclpy
   -> rcl -> rmw -> rmw_fastrtps -> Fast-RTPS).

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

CI:

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

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@clalancette clalancette merged commit 90cbdfd into ros2 Jan 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the clalancette/cleanup-tf2-ros-py-buffer-client branch January 14, 2022 22:16
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