Be much more careful about cleanup in the tf2_ros_py tests. #499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
|
CI:
|
delete-merged-branch
bot
deleted the
clalancette/cleanup-tf2-ros-py-buffer-client
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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
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.
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