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

If executor has been cancelled should not spin #2218

Closed

Conversation

mauropasse
Copy link
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()

@alsora
Copy link
Collaborator

alsora commented Jun 19, 2023

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.

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.

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

@mauropasse
Copy link
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