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

[iron] Fix an inherent race in execution vs. destruction. (backport of #1150) #1256

Merged
merged 1 commit into from Mar 20, 2024

Conversation

Timple
Copy link
Contributor

@Timple Timple commented Mar 15, 2024

Backport of #1150

I ran into this and noticed a solution is already available.

Thank you @clalancette for the works!

* Fix an inherent race in execution vs. destruction.

The rclpy executor collects all of the entities in one
pass, then creates async tasks for each of the ready ones
and attempts to "take" and execute them.  If one of those entities
is destroyed after the collection but before we attempt
to "take" it, then we can end up attempting to __enter__
a Destroyable-derived class that has already been destroyed.
The Destroyable will then raise an InvalidHandle error.

Fix this by explicitly catching the InvalidHandle error
that can be raised in all of the Destroyable-derived entities.
If we do catch it, then we actually let the machinery
continue but tell things to just not execute; in a subsequent
executor iteration, the entity will be destroyed and
hence not looked at anymore.  This seems to fix the race
in my testing.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@Timple Timple changed the title Fix an inherent race in execution vs. destruction. (#1150) [iron] Fix an inherent race in execution vs. destruction. (backport of #1150) Mar 15, 2024
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Collaborator

@clalancette what do you think about this backport? i think that is fine, this gives us better user experience. the backport is straight-forward.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 17, 2024

CI:

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

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I think this is an OK one to backport. It does change API a bit, but it is deep in the executor, and changes underscore-prefixed methods.

@clalancette clalancette merged commit a19180c into ros2:iron Mar 20, 2024
3 checks passed
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.

None yet

3 participants