Skip to content

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

Closed
ivanpauno wants to merge 1 commit intomasterfrom
ivanpauno/fix-entities-being-executed-twice
Closed

Fix error when SubscriptionIntraProcess is being executed more than once by the MultiThreadedExecutor#1275
ivanpauno wants to merge 1 commit intomasterfrom
ivanpauno/fix-entities-being-executed-twice

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented Aug 12, 2020 •
edited
Loading

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
@ivanpauno ivanpauno mentioned this pull request Aug 12, 2020
Merged
audrow
audrow reviewed Aug 12, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@audrow audrow mentioned this pull request Aug 12, 2020
Closed
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Aug 19, 2020 •
edited
Loading

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
sloretz
sloretz reviewed Sep 10, 2020
View reviewed changes
@ivanpauno
Avoid issue of subscription intra process being executed twice
948bb25 
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 audrow and sloretz 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
fujitatomoya
fujitatomoya approved these changes Oct 11, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Oct 23, 2020

CI:

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

@wjwwood
Copy link
Copy Markdown
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

Reviewers

@audrow audrow audrow approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@ivanpauno ivanpauno

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ivanpauno @wjwwood @sloretz @audrow @fujitatomoya