PYTHON-4981 - Create workaround for asyncio.Task.cancelling support in older Python versions #2009
Conversation
|
As discussed in person, we need to workaround the fast that task.cancelling() was only added in Python 3.11. |
| def __init__(self, coro: Coroutine[Any, Any, Any], *, name: Optional[str] = None) -> None: | ||
| super().__init__(coro, name=name) | ||
| self._cancelled: bool = False | ||
| asyncio._register_task(self) |
There was a problem hiding this comment.
We can't use private asyncio apis. Can we do this pattern instead?:
_Task(asyncio.create_task(...))There was a problem hiding this comment.
Oh I see the problem. That wouldn't work because asyncio.all_tasks() would return the non-wrapper class.
There was a problem hiding this comment.
The problem still remains though. If we can't do this using only public apis then we can't add this workaround at all.
There was a problem hiding this comment.
Any implementation needs to support our _Task getting cancelled by external components like unittest or pytest. The approach you outline, where _Task is just a wrapper around the actual task, doesn't work in this case: the loop will only interact with the task it owns, rather than the _Task itself. When our testing framework goes to cancel all remaining tasks, it only has access to the loop, which won't have a reference to any _Task instances.
Subclassing asyncio.Task and overriding the cancel method to support pre-3.11 Python versions solves this issue.
There was a problem hiding this comment.
asyncio._register_task is documented in the asyncio docs here: https://docs.python.org/3/library/asyncio-extending.html as the only way to extend Task functionality.
There was a problem hiding this comment.
The above passes all tests for me locally and doesn't use any private asyncio methods or import hacking, only MethodType tricks.
There was a problem hiding this comment.
If we own the task and its cancellation, can't we keep a mapping of tasks to cancelled state ourselves?
There was a problem hiding this comment.
We don't own the cancellation when the test runner cancels the task at the end of the test that created it. We could write our own teardown method that uses such a mapping to run before the runner's teardown, but that seems messy.
There was a problem hiding this comment.
Okay I relent for now since we plan remove this code anyway once we figure out the real issue causing the hangs.
There was a problem hiding this comment.
I'll add a TODO for us to come back and revisit this at that time.
NoahStapp
changed the title
|
|
||
|
|
||
| def create_task(coro: Coroutine[Any, Any, Any], *, name: Optional[str] = None) -> _Task: | ||
| return _Task(coro, name=name) |
There was a problem hiding this comment.
Okay if we're going down this route, can we at least only use this workaround on <=3.10?
Like:
if version >= 3.11:
return asyncio.create_task(...)
return _Task(coro, name=name)
There was a problem hiding this comment.
If we're fine with _Task.cancelling and asyncio.Task.cancelling having the same name but different actual purposes, then yes. asyncio.Task.cancelling technically returns the number of cancelled requests for a task instead of a boolean. I could also just implement a counter instead of a boolean to make it match.
There was a problem hiding this comment.
How would they differ? Oh I see, the stdlib returns a count of the cancellation requests as an integer. Is that what you mean?
Also I see this:
This method is used by asyncio’s internals and isn’t expected to be used by end-user code. See uncancel() for more details.
So we are going against their guidance in multiple ways.
There was a problem hiding this comment.
Yes let's implement the cancel/uncancel counter pattern. It's simple enough.
|
@NoahStapp Could you show a patch build for this? I'm a little skeptical this will resolve the hangs. |
|
No description provided.