Skip to content

If executor has been cancelled should not spin - #2218

Closed
mauropasse wants to merge 1 commit into
ros2:rollingfrom
mauropasse:mauro/fix-cancel-executor
Closed

mauropasse wants to merge 1 commit into
ros2:rollingfrom
mauropasse:mauro/fix-cancel-executor

Conversation

@mauropasse

Copy link
Copy Markdown
Collaborator

This simple code would hang forever:

  ExecutorType executor; // Any executor
  auto executor_thread = std::thread([&](){ executor.spin();});
  executor.cancel();
  executor_thread.join();

If executor.cancel() gets called before executor.spin(), cancel won't have any effect and the executor will keep spinning.

In this PR I add an extra bool to check if the executor was cancelled before blocking in spin()

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
@alsora

alsora commented Jun 19, 2023 •

Copy link
Copy Markdown
Collaborator

I'm not convinced by this approach.
The following code should result in the executor spinning

auto executor = std::make_shared<executor>();
executor->cancel();
.... do some stuff ...
executor->spin(); // This should spin, the cancel that we called before should not affect it

The problem that you describe is a race condition and I think that the already existing executor->is_spinning() method is the correct way to synchronize and avoid it.

@fujitatomoya fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mauropasse thanks for opening PR. I am with @alsora on this. executor should be able to spin after cancel.

@mauropasse

Copy link
Copy Markdown
Collaborator Author

Makes sense, a cancel() call should only affect an spinning executor, so the user should make sure the executor is spinning before cancelling.

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.

3 participants