|
@1st1 if you like the concept I'll add full test coverage. |
| if self._current_task is not None: | ||
| raise RuntimeError("Entring into task {!r} " | ||
| "when other task {!r} is executed. " | ||
| "The loop invariants are broken" |
There was a problem hiding this comment.
No need for "The loop invariants are broken". I think it's pretty obvious when an event loop raises a RuntimeError.
| try: | ||
| return loop.current_task() | ||
| except (AttributeError, NotImplementedError): | ||
| return cls._current_tasks.get(loop) |
There was a problem hiding this comment.
Let's rewrite as:
try:
current_task_meth = loop.current_task
except AttributeError:
pass
else:
try:
return current_task_meth()
except NotImplementedError:
passThis way we won't accidentally trap an AttributeError from a badly behaving loop.current_task().
There was a problem hiding this comment.
BTW, don't forget to update _asynciomodule.c to use new loop methods too.
| try: | ||
| self._loop._register_task(self) | ||
| except (AttributeError, NotImplementedError): | ||
| self.__class__._all_tasks.add(self) |
There was a problem hiding this comment.
Let's remove Task._all_tasks and Task._current_tasks. They were never public attributes anyways.
There was a problem hiding this comment.
These attributes are present for sake of backward compatibility.
Let's imagine a loop without task introspection support (not implementing _register_task, _enter_task and _leave_task).
The proposed PR will keep working by all_tasks/_current_task fallbacks.
There was a problem hiding this comment.
Got it. Let's add a comment that this code is needed to thrird-party event loops end that it will be removed in 3.8.
| return cls._current_tasks.get(loop) | ||
|
|
||
| @classmethod | ||
| def all_tasks(cls, loop=None): |
There was a problem hiding this comment.
Let's raise PendingDeprecationWarning in Task.all_tasks() and Task.current_task(), suggesting to use the new event loop methods and that Task methods will be removed in 3.9.
|
Overall it looks good. @bdarnell do you have any comments re proposed task introspection API for asyncio? |
| self.slow_callback_duration = 0.1 | ||
| self._current_handle = None | ||
| self._task_factory = None | ||
| self._tasks = weakref.WeakSet() |
There was a problem hiding this comment.
I think there should be an unregister_task() method instead of relying on GC and WeakSet. It's weird if holding on to a copy of all_tasks() prevents any of those tasks from leaving the set even if they're finished. (The WeakSet implementation was fine for an internal-only debugging tool, but if it's going to be public I think it should have more well-defined semantics.)
There was a problem hiding this comment.
I think it's a good idea. asyncio.Task can have a __del__ method which would call self.loop.unregister_task(self).
There was a problem hiding this comment.
Unfortunately it doesn't work: if tasks are stored in strong set() inside a loop the __del__ is never called -- refcount never became zero.
I could call _unregister_task() from _step() on exit from coroutine.
Note about naming: current_task() and all_tasks() are public methods but _register_task() and family should be never called by user code, that's why I've added underscore prefix.
| """Return a task factory, or None if the default one is in use.""" | ||
| return self._task_factory | ||
|
|
||
| def current_task(self): |
There was a problem hiding this comment.
All of the new methods need documentation. Do they operate only on instances of asyncio.Task or is there a "task" interface that could have other implementations?
For Tornado I originally wanted to be able to use a tornado.gen.Runner instead of asyncio.Task, but I've given up on that. If anyone else is still pursuing alternative coroutine runners you may want to get their feedback on this interface too.
If it's only asyncio.Task, then I think it probably makes more sense to just do everything with public classmethods of Task instead of putting it on the event loop. Every event loop is just going to copy these five methods more or less verbatim; there's nothing here that requires anything of the event loop besides a place to store a couple of attributes. The only reason I see to move it from Task to the event loop is if we're aiming to abstract away the Task class itself.
There was a problem hiding this comment.
In my mind a task is hashable object with .cancel() and .cancelled().
With the patch task could be and coroutine runner (sorry, only in Python 3.7+).
We can invite a AbstractTask if needed.
There was a problem hiding this comment.
All of the new methods need documentation. Do they operate only on instances of asyncio.Task or is there a "task" interface that could have other implementations?
They will work with anything that behaves like asyncio.Task, although I don't think we need to add any type checks to enforce that.
Originally this PR was motivated by PyO3/tokio event loop, which is written in Rust. The idea is that tokio would have its own Task implementation that can be used in both Python and Rust environments simultaneously—think of it as a bridge between async Python and async Rust code.
There aren't a lot of requirements for a Task-like object: it must implement asyncio.Future APIs (add_done_callback(), cancel(), etc), and must be able to wrap a coroutine object. tokio event loop provides just that. There should be nothing wrong to have a program that simultaneously uses a few different Task implementations.
I guess you have a similar need in Tornado. You likely need a custom Task class that supports both generator-based coroutines and async/await, and maybe some Tornado-specific functionality, right?
If it's only asyncio.Task, then I think it probably makes more sense to just do everything with public classmethods of Task instead of putting it on the event loop. Every event loop is just going to copy these five methods more or less verbatim; [..]
You're right here. I now think that instead of having those as methods on the event loop, they should be just top-level asyncio functions: asyncio.all_tasks(), asyncio.current_task(), etc.
There was a problem hiding this comment.
We can invite a AbstractTask if needed.
I think it would be great to have both AbstractFuture and AsbtractTask ABCs just like we have events.AbstractEventLoop. Just as with event loops it won't be required for Future-like and Task-like objects to actually extend those classes.
There was a problem hiding this comment.
Yeah, it sounds like introducing an AbstractTask is the way to go, just to define what exactly is necessary (for example, set_result and set_exception probably don't belong here)
You likely need a custom Task class that supports both generator-based coroutines and async/await, and maybe some Tornado-specific functionality, right?
Hmm, I hadn't even considered registering tornado's yield-based coroutines with asyncio like this. That could be useful (although it would require us to implement cancellation). I don't actually have any tornado-specific functionality that is compatible with async/await, which is why I'm moving to just use asyncio.Task in that case.
Just as with event loops it won't be required for Future-like and Task-like objects to actually extend those classes.
That would be great! I thought I had lost that argument and I've spent a lot of time shaving yaks to just move to the asyncio classes, since they weren't amenable to external implementations.
There was a problem hiding this comment.
That would be great! I thought I had lost that argument and I've spent a lot of time shaving yaks to just move to the asyncio classes, since they weren't amenable to external implementations.
BTW, I'm not sure if you know about this as it's a rather obscure thing: since at least 3.6, any object that has a _asyncio_future_blocking attribute is recognized as a Future-like object by asyncio. This is something that we could highlight in the AbstractFuture ABC.
@asvetlov Let's create a separate issue to add the discussed ABCs. For this one -- let's make all_tasks(), current_task(), _register_task(), etc top-level asyncio functions. Ben's right, there's nothing loop-specific about them.
There was a problem hiding this comment.
I don't follow what is proposed implementation?
Just move Task._current_tasks and Task._all_tasks into module namespace?
In my mind a loop is a container for own tasks.
asyncio.current_task() and asyncio.all_tasks() make sense as user-level API but implementation should sit in a loop, isn't it?
There was a problem hiding this comment.
I don't follow what is proposed implementation?
Just move Task._current_tasks and Task._all_tasks into module namespace?
Yes, that's what I'm suggesting.
In my mind a loop is a container for own tasks.
asyncio.current_task() and asyncio.all_tasks() make sense as user-level API but implementation should sit in a loop, isn't it?
Let me list here some thoughts I have about this:
All event loops will have to copy the implementation from asyncio. The API and its implementation is very simple and will likely be identical between, say, uvloop and asyncio/base_events.py. This duplication of code in different loop implementations is the main argument against adding this API to the event loop.
We actually want to minimize the need of getting the current event loop. We also want the API to become straightforward to use. Compare the following:
loop = asyncio.get_event_loop()
loop.current_task()
# vs
asyncio.Task.current_task()
# vs
asyncio.current_task()asyncio.Task.current_task() isn't just cumbersome to use. It also requires us to have two implementations of it in asyncio. One in tasks.py, and one in _asynciomodule.c.
asyncio.Task.current_task() is tied to asyncio.Task. It's weird to expect it to point to a different implementation of a Task (say tokio's Task objects).
Currently, the loop argument is optional in asyncio.Task.current_task(). If the loop wasn't passed to it explicitly, it uses asyncio.get_event_loop() to get the current event loop. The caveat here is that asyncio.get_event_loop() can actually create a new event loop, or do something else, depending on the current policy.
I want the new current_task() implementation to use asyncio._get_running_loop(), and raise an error if there's no currently running event loop.
Implementing current_task() as a top-level asyncio function means that the existing event loops don't need to be updated at all to stay compatible with the new APIs we want to add. This means we can drop asyncio.Task._current_tasks and asyncio.Task._all_tasks attributes. We can't drop them with the current approach.
Maintaining a separate event loop implementation (like uvloop) is already hard. I'd like to minimize the number of APIs we need to implement on the loop. In this case it's easy to do so.
All in all, I do believe now, that asyncio.current_task() and asyncio.all_tasks() is the way to go. Decoupling them from asyncio.Task and the event loop has simply too many advantages and no downsides.
There was a problem hiding this comment.
Makes sense.
Thanks for comprehensive explanation.
I'll rewrite the PR tomorrow.
|
Closing this in favour of #4799 |
https://bugs.python.org/issue32250