-
-
Notifications
You must be signed in to change notification settings - Fork 34.8k
bpo-32250: Implement loop.current_task() and loop.all_tasks() #4766
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,6 +248,8 @@ def __init__(self): | |
| self.slow_callback_duration = 0.1 | ||
| self._current_handle = None | ||
| self._task_factory = None | ||
| self._tasks = weakref.WeakSet() | ||
| self._current_task = None | ||
| self._coroutine_wrapper_set = False | ||
|
|
||
| if hasattr(sys, 'get_asyncgen_hooks'): | ||
|
|
@@ -302,6 +304,28 @@ def get_task_factory(self): | |
| """Return a task factory, or None if the default one is in use.""" | ||
| return self._task_factory | ||
|
|
||
| def current_task(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the new methods need documentation. Do they operate only on instances of 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my mind a task is hashable object with We can invite a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They will work with anything that behaves like 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 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?
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:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it would be great to have both
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it sounds like introducing an AbstractTask is the way to go, just to define what exactly is necessary (for example,
Hmm, I hadn't even considered registering tornado's
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 @asvetlov Let's create a separate issue to add the discussed ABCs. For this one -- let's make
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't follow what is proposed implementation?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I'm suggesting.
Let me list here some thoughts I have about this:
All in all, I do believe now, that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
| return self._current_task | ||
|
|
||
| def all_tasks(self): | ||
| return set(self._tasks) | ||
|
|
||
| def _register_task(self, task): | ||
| self._tasks.add(task) | ||
|
|
||
| def _enter_task(self, task): | ||
| if self._current_task is not None: | ||
| raise RuntimeError("Entering into task {!r} " | ||
| "when other task {!r} is executed." | ||
| .format(task, self._current_task)) | ||
| self._current_task = task | ||
|
|
||
| def _leave_task(self, task): | ||
| if self._current_task is not task: | ||
| raise RuntimeError("Leaving task {!r} is not current {!r}." | ||
| .format(task, self._current_task)) | ||
| self._current_task = None | ||
|
|
||
| def _make_socket_transport(self, sock, protocol, waiter=None, *, | ||
| extra=None, server=None): | ||
| """Create socket transport.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,19 +51,39 @@ def current_task(cls, loop=None): | |
|
|
||
| None is returned when called not in the context of a Task. | ||
| """ | ||
| warnings.warn("Task.current_task() is deprecated, " | ||
| "use loop.current_task() instead.", | ||
| PendingDeprecationWarning, | ||
| stacklevel=2) | ||
| if loop is None: | ||
| loop = events.get_event_loop() | ||
| return cls._current_tasks.get(loop) | ||
| try: | ||
| return loop.current_task() | ||
| except (AttributeError, NotImplementedError): | ||
| # This code is needed to thrird-party event loops that don't | ||
| # support loop introspection API yet. | ||
| # The fallback will be removed in 3.8. | ||
| return cls._current_tasks.get(loop) | ||
|
|
||
| @classmethod | ||
| def all_tasks(cls, loop=None): | ||
| """Return a set of all tasks for an event loop. | ||
|
|
||
| By default all tasks for the current event loop are returned. | ||
| """ | ||
| warnings.warn("Task.all_tasks() is deprecated, " | ||
| "use loop.all_tasks() instead.", | ||
| PendingDeprecationWarning, | ||
| stacklevel=2) | ||
| if loop is None: | ||
| loop = events.get_event_loop() | ||
| return {t for t in cls._all_tasks if t._loop is loop} | ||
| try: | ||
| return loop.all_tasks() | ||
| except (AttributeError, NotImplementedError): | ||
| # This code is needed to thrird-party event loops that don't | ||
| # support loop introspection API yet. | ||
| # The fallback will be removed in 3.8. | ||
| return {t for t in cls._all_tasks if t._loop is loop} | ||
|
|
||
| def __init__(self, coro, *, loop=None): | ||
| assert coroutines.iscoroutine(coro), repr(coro) | ||
|
|
@@ -74,7 +94,10 @@ def __init__(self, coro, *, loop=None): | |
| self._fut_waiter = None | ||
| self._must_cancel = False | ||
| self._loop.call_soon(self._step) | ||
| self.__class__._all_tasks.add(self) | ||
| try: | ||
| self._loop._register_task(self) | ||
| except (AttributeError, NotImplementedError): | ||
| self.__class__._all_tasks.add(self) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These attributes are present for sake of backward compatibility.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| def __del__(self): | ||
| if self._state == futures._PENDING and self._log_destroy_pending: | ||
|
|
@@ -167,7 +190,10 @@ def _step(self, exc=None): | |
| coro = self._coro | ||
| self._fut_waiter = None | ||
|
|
||
| self.__class__._current_tasks[self._loop] = self | ||
| try: | ||
| self._loop._enter_task(self) | ||
| except (AttributeError, NotImplementedError): | ||
| self.__class__._current_tasks[self._loop] = self | ||
| # Call either coro.throw(exc) or coro.send(None). | ||
| try: | ||
| if exc is None: | ||
|
|
@@ -238,7 +264,10 @@ def _step(self, exc=None): | |
| RuntimeError( | ||
| 'Task got bad yield: {!r}'.format(result))) | ||
| finally: | ||
| self.__class__._current_tasks.pop(self._loop) | ||
| try: | ||
| self._loop._leave_task(self) | ||
| except (AttributeError, NotImplementedError): | ||
| self.__class__._current_tasks.pop(self._loop) | ||
| self = None # Needed to break cycles when an exception occurs. | ||
|
|
||
| def _wakeup(self, future): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofall_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea.
asyncio.Taskcan have a__del__method which would callself.loop.unregister_task(self).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()andall_tasks()are public methods but_register_task()and family should be never called by user code, that's why I've added underscore prefix.