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

Fix error when SubscriptionIntraProcess is being executed more than once by the MultiThreadedExecutor #1275

Closed

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Aug 12, 2020

This ensures that executing is a noop if there's nothing to be executed.

Alternative to #1241.

@ivanpauno ivanpauno marked this pull request as draft August 12, 2020 20:29
@ivanpauno ivanpauno self-assigned this Aug 12, 2020
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.

I like how minimal this solution is. As you mention that this is just a fix for SubscriptionIntraProcess, the same kind of fix should be applied to all classes that inherit from Waitable. If we go with this approach, it should probably be documented explicitly that execute can be called twice on one event and that this should be handled within execute.

@ivanpauno
Copy link
Member Author

ivanpauno commented Aug 19, 2020

I like how minimal this solution is. As you mention that this is just a fix for SubscriptionIntraProcess, the same kind of fix should be applied to all classes that inherit from Waitable

I can apply a fix to action clients and services, doing the same that subscriptions do: ignore when taking data fails.
I would prefer to do that in a separate PR.

For QOSEventHandler I don't see how to apply a similar approach that the one applied here.
A possible fix is to add the concept of a Waitable that is mutually exclusive with itself (which can be implemented by creating a callback group and adding only that single Waitable to it).
I think that approach is nice, and I like it for timers too.

(edit) I have corrected the comment, because I originally said something that was wrong (I get confused each time I read these patches again).

@ivanpauno ivanpauno marked this pull request as ready for review August 19, 2020 19:43
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/fix-entities-being-executed-twice branch from 3d0f8df to 948bb25 Compare September 23, 2020 17:14
@ivanpauno ivanpauno requested review from sloretz and audrow September 23, 2020 17:14
@ivanpauno ivanpauno changed the title Fix Timer and SubscriptionIntraProcess being executed more than once by the MultiThreadedExecutor Fix error when SubscriptionIntraProcess is being executed more than once by the MultiThreadedExecutor Sep 23, 2020
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

@ivanpauno
Copy link
Member Author

CI:

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

@wjwwood
Copy link
Member

wjwwood commented Nov 3, 2020

I generally do not like this approach, as I said here:

#1241 (comment)

But I'll leave it to the other rclcpp maintainers to decide, since I don't have time to help architect a better solution.

@ivanpauno ivanpauno closed this Nov 4, 2020
@clalancette clalancette deleted the ivanpauno/fix-entities-being-executed-twice branch January 15, 2021 16:33
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.

5 participants