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

Jump to bottom

Add cancel() to RunningTask and deprecate kill() #2679

Open
marlonjames opened this issue Aug 19, 2021 · 6 comments · May be fixed by #3874
Open

Add cancel() to RunningTask and deprecate kill() #2679

marlonjames opened this issue Aug 19, 2021 · 6 comments · May be fixed by #3874
Labels
type:deprecation API that should warn and eventually be removed
Milestone
2.0

Comments

@marlonjames
Copy link
Contributor

marlonjames commented Aug 19, 2021 •
edited

Continuing the theme of providing the new interfaces before 2.0.
For now we could keep it the same, without using CancelledError.

Potentially, kill() could be made private and used by the scheduler, whereby the task is cancelled even if it tries to suppress the exception.
@marlonjames marlonjames changed the title Add cancel() to `RunningTask and deprecate kill() Add cancel() to RunningTask and deprecate kill() Aug 19, 2021
@marlonjames marlonjames added the type:deprecation API that should warn and eventually be removed label Aug 19, 2021
@ktbarrett
Copy link
Member

So the point is just the name change? I think if we are going to introduce "another way" to kill a task it should behave at least slightly differently.

@marlonjames
Copy link
Contributor Author

Yes the point is to change the name and refer to cancelling a task. Brings the language in line with asyncio and trio.

@ktbarrett
Copy link
Member

Are there problems with making cancel throw CancelledError right away? Is that a lot of work? If it is, perhaps we should emit a FutureWarning about it throwing CancelledError in 2.0?

@marlonjames
Copy link
Contributor Author

marlonjames commented Sep 3, 2021 •
edited

It would take some work, as kill just calls Scheduler._unschedule, which doesn't run the Task's coroutine again.

@marlonjames
Copy link
Contributor Author

cancel() was added in #2877.
Deprecating kill() is to-be-decided.

@ktbarrett
Copy link
Member

I think kill() should be deprecated and I think we should also work to support throwing CancelledErrors.

@marlonjames marlonjames added this to the 2.0 milestone Oct 12, 2023
@ktbarrett ktbarrett mentioned this issue Feb 28, 2024
Closed
@ktbarrett ktbarrett linked a pull request May 1, 2024 that will close this issue
Draft
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type:deprecation API that should warn and eventually be removed
Projects
None yet
Milestone
2.0
Development

Successfully merging a pull request may close this issue.

Throw CancelledError into cancelled tasks ktbarrett/cocotb
2 participants
@ktbarrett @marlonjames