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.

Sign up for GitHub

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 a concurrency problem in the multithreaded executor #703

Merged
merged 1 commit into from
Apr 22, 2019
Merged

Fix a concurrency problem in the multithreaded executor #703

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,8 @@ MultiThreadedExecutor::run(size_t)
scheduled_timers_.erase(it);
}
}
// Clear the callback_group to prevent the AnyExecutable destructor from
// resetting the callback group `can_be_taken_from`
any_exec.callback_group.reset();
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned this isn't the right fix, @guillaumeautran why did you not change the destructor of AnyExecutable instead?

Just changing the callback group to nullptr is fragile. There's nothing in the destructor of AnyExecutable which implies the callback group might be reset to affect behavior.

Instead, I think, at the very least, AnyExecutable should have had a boolean like should_reset_can_be_taken_from_automatically which defaulted to true, and the multi threaded executor could change to false. Then it's clearer from both the multi threaded executor's code and any executable's code what's happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if there was a specific reason why the destructor was setup the way it was and did not want to run the risk of leaving the group "locked". I'll make the requested changes then and push another PR.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible that since this exists:

https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/executor.cpp#L288

We should just remove the code in the destructor of AnyExecutable that changes the can_be_taken_from of the callback group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wjwwood Do you think I can add an argument to the constructor of the AnyExecutable class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be hesitant from removing the if statement. There is a comment that makes a lot of sense:

rclcpp/rclcpp/src/rclcpp/any_executable.cpp

Line 34 in 97ed34a

if (callback_group) {

}
}