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

make sure futures can be cancelled #797

Merged
merged 1 commit into from Oct 19, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 19, 2023

In order to cancel tasks, we need to have a handle on the Future, not just the Coroutine object.

The code we have implements TaskGroup-style cancellation of pending tasks, since gather doesn't cancel unfinished tasks unless gather itself is cancelled, but TaskGroups are new in Python 3.11.

See the note in the docs for asyncio.gather:

TaskGroup provides stronger safety guarantees than gather for scheduling a nesting of subtasks: if a task (or a subtask, a task scheduled by a task) raises an exception, TaskGroup will, while gather will not, cancel the remaining scheduled tasks).

i.e. TaskGroup has the behavior "if anything here fails, stop everything still running and then propagate the error", while gather has the behavior "if anything here fails, propagate the error, but leave any other tasks running for the user to clean up or not." And since we don't yet require Python 3.11, we use gather and then implement the cancellation ourselves.

closes #796

implement TaskGroup-style cancellation of pending tasks,
since gather doesn't cancel unfinished tasks unless gather _itself_ is cancelled.
@minrk minrk added the bug label Oct 19, 2023
@minrk
Copy link
Member Author

minrk commented Oct 19, 2023

It may well make sense to actually change the wait to return_when=ALL_COMPLETED so there's nothing to cancel, especially in the stop_all. But that's not the most important, really, since it's a cleanup utility purely for use in tests and always means a serious problem in the test suite if there's any error anyway.

Pro waiting for everything instead of cancelling:

  • don't need manual cancel
  • all tasks tried to complete

Con:

  • slightly more complex to re-raise errors
  • delays reporting of errors in what's already necessarily a serious failure (can't start or stop reflectors!), after which nothing really makes sense anyway

@minrk
Copy link
Member Author

minrk commented Oct 19, 2023

The third option is not cancelling at all. But then we're in the situation of a task still running with nothing waiting for it (e.g. stop could still be outstanding when the next start is called, etc.). Again, this wouldn't be the biggest deal because there are likely serious kubernetes communication problems whenever this comes up, but leaking tasks nobody's waiting for is generally something to avoid, which I think is why asyncio added trio-style TaskGroups so folks can write some code that does "start some async things; but no matter what happens, when I exit this context, none of those tasks are running".

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

@minrk ❤️ 🎉 thank you for resolving this, helping me learn with great PR comments, and providing inline notes to help us update this in the future to use TaskGroup!!

@consideRatio consideRatio merged commit 9b83e8c into jupyterhub:main Oct 19, 2023
9 checks passed
@minrk minrk deleted the actually-start-futures branch October 19, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object has no attribute 'cancel' - a secondary error when handling a timeout error
2 participants