Expressly destroy a node's objects before the node. #456
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
This seems to reduce hangs during test runs described in ros2/build_farmer#248. The handles corresponding to the destroyed objects *should* be getting destroyed explicitly when self.handle.destroy() is called below. It seems however that when running with Fast-RTPS it's possible to get into a state where multiple threads are waiting on futexes and none can move forward. The rclpy context of this apparent deadlock is while clearing a node's list of publishers or services (possibly others, although publishers and services were the only ones observed). I consider this patch to be a workaround rather than a fix. I think there may either be a race condition between the rcl/rmw layer and the rmw implementation layer which is being tripped by the haphazardness of Python's garbage collector or there is a logical problem with the handle destruction ordering in rclpy that only Fast-RTPS is sensitive to. Signed-off-by: Steven! Ragnarök <[email protected]>
nuclearsandwich
force-pushed
the
nuclearsandwich/destroy-dependents-first
branch
from
253f18e to
30600a1
Compare
|
|
This doesn't seem outrageous. It does seem like there may be a more elegant solution, but it probably requires some arcane knowledge of Python garbage collection's interactions with |
|
The new logic seems to remove items for the collections while iterating over them which shouldn't be done. The loops should be changed into |
|
Signed-off-by: Steven! Ragnarök <[email protected]>
nuclearsandwich
force-pushed
the
nuclearsandwich/destroy-dependents-first
branch
from
97d3ccf to
0337867
Compare
Signed-off-by: Steven! Ragnarök <[email protected]>
Signed-off-by: Steven! Ragnarök <[email protected]>
|
sloretz
reviewed
Nov 8, 2019
As pointed out by Shane, pop()ing each item from the list before passing it to the .destroy_ITEM() method prevents it from being destroyed as the individual methods first check that the item is present in the list then remove it before continuing to destroy it. Signed-off-by: Steven! Ragnarök <[email protected]>
sloretz
approved these changes
Nov 8, 2019
|
I've also been dissatisfied with this solution. After discussion with @sloretz I've started another experiment running with Python's GC disabled before 1461 and enabled after self.handle.destroy(). That has also seemingly stopped the hang from occurring over the last ~50 test runs which means we might need to look more closely at the handle destruction logic. |
|
I ran experimental tests most of the day Saturday and I was able to reproduce a hang using this branch of rclpy with rmw_cyclonedds_cpp as well. Although it's in a slightly different place. In this gdb log thread 2 is stuck during capsule destruction in a futex down in cyclonedds. |
|
@hidmic and I dove into this today and we couldn't pinpoint a reason for the hang. It is still possible to hang with even with this PR although the incidence rate is lower. We did a couple of experiments trying to reproduce the hang outside of launch tests using multiple threads and rclpy but were unable to do so. I do have a bit more detail from the current reproduction since the destruction doesn't occur as part of a garbage collection. In python, we're inside Handle.__destroy_self and in the C context we can see the pycapsule destructor calling rcl_publisher_fini which is carrying us into Fast-RTPS internals where we're stuck waiting for a lock: This PR still improves matters, but it's not a full solution. |
|
In discussion yesterday we've decided to merge this as an incremental improvement. I'll leave ros2/build_farmer#248 open or create a more focused issue to continue investigation. |
mjcarroll
approved these changes
Nov 13, 2019
delete-merged-branch
bot
deleted the
nuclearsandwich/destroy-dependents-first
branch
jaisontj
pushed a commit
to aws-ros-dev/rclpy
that referenced
this pull request
Nov 19, 2019
* Expressly destroy a node's objects before the node. This seems to reduce hangs during test runs described in ros2/build_farmer#248. The handles corresponding to the destroyed objects *should* be getting destroyed explicitly when self.handle.destroy() is called below. It seems however that when running with Fast-RTPS it's possible to get into a state where multiple threads are waiting on futexes and none can move forward. The rclpy context of this apparent deadlock is while clearing a node's list of publishers or services (possibly others, although publishers and services were the only ones observed). I consider this patch to be a workaround rather than a fix. I think there may either be a race condition between the rcl/rmw layer and the rmw implementation layer which is being tripped by the haphazardness of Python's garbage collector or there is a logical problem with the handle destruction ordering in rclpy that only Fast-RTPS is sensitive to. * Don't pre-emptively remove items from Node lists. As pointed out by Shane, pop()ing each item from the list before passing it to the .destroy_ITEM() method prevents it from being destroyed as the individual methods first check that the item is present in the list then remove it before continuing to destroy it. Signed-off-by: Steven! Ragnarök <[email protected]>
suab321321
pushed a commit
to suab321321/rclpy
that referenced
this pull request
Jan 31, 2020
* Expressly destroy a node's objects before the node. This seems to reduce hangs during test runs described in ros2/build_farmer#248. The handles corresponding to the destroyed objects *should* be getting destroyed explicitly when self.handle.destroy() is called below. It seems however that when running with Fast-RTPS it's possible to get into a state where multiple threads are waiting on futexes and none can move forward. The rclpy context of this apparent deadlock is while clearing a node's list of publishers or services (possibly others, although publishers and services were the only ones observed). I consider this patch to be a workaround rather than a fix. I think there may either be a race condition between the rcl/rmw layer and the rmw implementation layer which is being tripped by the haphazardness of Python's garbage collector or there is a logical problem with the handle destruction ordering in rclpy that only Fast-RTPS is sensitive to. * Don't pre-emptively remove items from Node lists. As pointed out by Shane, pop()ing each item from the list before passing it to the .destroy_ITEM() method prevents it from being destroyed as the individual methods first check that the item is present in the list then remove it before continuing to destroy it. Signed-off-by: Steven! Ragnarök <[email protected]> Signed-off-by: AbhinavSingh <[email protected]>
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.
This seems to reduce hangs during test runs described in
ros2/build_farmer#248.
Another few hours of testing will convince me to strengthen that statement.
The handles corresponding to the destroyed objects should be getting
destroyed explicitly when self.handle.destroy() is called below. It
seems however that when running with Fast-RTPS it's possible to get into
a state where multiple threads are waiting on futexes and none can move
forward. The rclpy context of this apparent deadlock is while clearing
a node's list of publishers or services (possibly others, although
publishers and services were the only ones observed).
I consider this patch to be a workaround rather than a fix as I'm not particularly proud of how little I understand why the existing destruction handling is insufficient.
I think there may either be a race condition between the rcl/rmw layer
and the rmw implementation layer which is being tripped by the
haphazardness of Python's garbage collector or there is a logical
problem with the handle destruction ordering in rclpy that only
Fast-RTPS is sensitive to. A further possibility is to play around with pausing the garbage collector and triggering it at explicit points to see if that changes results at all.