bpo-32250: Implement asyncio.current_task() and asyncio.all_tasks() by asvetlov · Pull Request #4799 · python/cpython

asvetlov · 2017-12-11T18:42:42Z

Second version.
Proof of concept, no tests, C Accelerator is not updated.

https://bugs.python.org/issue32250

1st1 · 2017-12-11T19:10:26Z


 from . import base_futures
 from . import coroutines
+from .events import get_running_loop


Import the module, not the function.

1st1 · 2017-12-11T19:12:52Z

        By default all tasks for the current event loop are returned.
        """
-        if loop is None:
-            loop = events.get_event_loop()


You should keep get_event_loop() here for backwards compatibility. So that code that calls Task.all_tasks() before a loop is created continues to work in 3.7 (although it will raise a warning).

asyncio.all_tasks() should use get_running_loop().

Not sure about asyncio.all_tasks() -- calling the function outside of coroutine context might make sense.
RuntimeError from asyncio.current_task() is more reasonable but I not sure again.

Not sure about asyncio.all_tasks() -- calling the function outside of coroutine context might make sense.

Then pass a loop object explicitly. The problem with get_event_loop() is that it can create a new loop object implicitly. Or it may fail at creating it -- that function is weird.

1st1 · 2017-12-11T19:12:56Z

Overall it looks good. I think we should keep using a weak-mapping for _all_tasks.

_unregister_task exists for the purpose of forcefully removing the task object from _all_tasks for task implementations that need it, but I don't want to make it mandatory to be called.

asvetlov · 2017-12-12T14:53:45Z

Implementation is finished (I hope), test coverage added.
Please review again.

If everything is ok -- I'll start documenting the feature.

P.S.
AbstractTask/AbstractFuture are not implemented, let's do it in separate PR.
This one is large enough already.

1st1 · 2017-12-12T17:39:48Z

+
+
+# WeakKeyDictionary of {Task: EventLoop} containing all tasks alive.
+_all_tasks = weakref.WeakKeyDictionary()


Why weakref.WeakKeyDictionary and not weakref.WeakValueDictionary?

A task should be weak, on task garbage collection an item from _all_tasks should disappear. asyncio on master branch uses WeakSet of tasks.
But task._loop is a private property, custom task implementation can not expose it. That's why storing a loop along with task itself is required.

Mapping like {loop: WeakSet[task]} is not an option: there is no signal for loop deletion and _all_tasks is the singleton. In case of many loops creations/deletions (unit tests) the singleton will blow up with closed loops. I want to avoid it.

Alright. This deserves to be put in a comment.

1st1 · 2017-12-12T17:42:30Z

+    current_task = _current_tasks.get(loop)
+    if current_task is not None:
+        raise RuntimeError(f"Entering into task {task!r} "
+                           f"when other task {current_task!r} is executed.")


Cannot enter into task {task!r} while another task {current_task!r} is being executed.

1st1 · 2017-12-12T17:43:38Z

+    current_task = _current_tasks.get(loop)
+    if current_task is not task:
+        raise RuntimeError(f"Leaving task {task!r} "
+                           f"is not current {current_task!r}.")


Leaving task {task!r} does not match the current task {current_task!r}.

1st1 · 2017-12-12T17:44:30Z

 from . import events
 from . import futures
 from .coroutines import coroutine
+from .base_tasks import (all_tasks, current_task, _register_task,


Please import the module, not functions from it.

In this case base_tasks should have __all__ = ('all_tasks', 'current_task') to export names into asyncio namespace. _register_task cannot be exposed by base_tasks, see comment below.

1st1 · 2017-12-12T17:45:47Z

+                      "use asyncio.current_task() instead",
+                      PendingDeprecationWarning,
+                      stacklevel=2)
+        return current_task(loop)


So we want to use asyncio.get_running_loop() in asyncio.current_task(). And we want to keep using asyncio.get_event_loop() in Task.current_task() to maintain backwards compat.

@classmethod def current_task(cls, loop=None): if loop is None: loop = events.get_event_loop() warnings.warn(..) return current_task(loop)

1st1 · 2017-12-12T17:46:55Z

+
+def current_task(loop=None):
+    if loop is None:
+        loop = events.get_event_loop()


Use events.get_running_loop() here -- we want to use the new API here. It doesn't make sense to call asyncio.current_task() and accidentally create a new event loop because get_event_loop() thinks so.

1st1 · 2017-12-12T17:49:35Z

 _PyTask = Task
+_py_register_task = _register_task
+_py_enter_task = _enter_task
+_py_leave_task = _leave_task


You should do this logic in base_tasks.py.

Not so easy: asyncio._asyncio imports asyncio.base_tasks. C Accelerators becomes available only in asyncio.tasks, pushing them into base_tasks leads to circular imports.

That's why I've decided to have both _py_register_task and _c_register_task in the same asyncio.tasks module.

Then define all these functions (and _all_tasks and _current_tasks mappings) in _asynciomodule.c and try to import them in tasks.py. If that fails, define them in Python. Let's not touch base_tasks.py at all in this PR.

If we define _all_tasks in _asynciomodule.c -- C Accelerator becomes mandatory.
Let's define _all_tasks and _current_tasks in base_tasks.py but move _register_task and family into tasks.py.

C Accelerator becomes mandatory.

No, why?

_asynciomodule.c:

// defines _all_tasks, _current_tasks

asyncio/tasks.py:

try: from _asyncio import _all_tasks, _current_tasks except ImportError: _all_tasks = ... _current_tasks = ...

I actually wanted to make a separate PR

I can make that PR myself now.

working on it.

Please do.
I'm going to sleep soon anyway.

Please do.

Almost there.

Thus asyncio.current_loop() should be in base_tasks.py.

Yeah... I get what you're saying now. OK, let it be in base_tasks.py. I'm not blocking you from working on this PR, right?

Yes, you are not blocking.

1st1 · 2017-12-12T17:49:40Z

    Task = _CTask = _asyncio.Task
+    _register_task = _c_register_task = _asyncio._register_task
+    _enter_task = _c_enter_task = _asyncio._enter_task
+    _leave_task = _c_leave_task = _asyncio._leave_task


You should do this logic in base_tasks.py.

1st1 · 2017-12-12T17:53:03Z

+
+static PyObject *
+_asyncio__enter_task_impl(PyObject *module, PyObject *loop, PyObject *task)
+/*[clinic end generated code: output=a22611c858035b73 input=de1b06dca70d8737]*/


I prefer not to use _impl functions in C code. Better to implement int enter_task(loop, task) function and use it in Task implementation directly. Then you call that function in _asyncio__enter_task_impl and return NULL/None based on its return value. This way you also don't need to decref None in Task code, and the code is overall cleaner.

1st1 · 2017-12-12T17:53:42Z

AbstractTask/AbstractFuture are not implemented, let's do it in separate PR.

Absolutely, a separate PR is the way to go.

asvetlov · 2017-12-12T18:49:42Z

Do you agree with using get_event_loop() in asyncio.all_tasks()? asyncio.current_task() will be switched to get_running_loop().

1st1 · 2017-12-14T17:29:56Z

overall LGTM.

Please apply this diff to fix tests in refleak mode:

diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py
index 31d4ed649c..f6a1ac6f31 100644
--- a/Lib/test/test_asyncio/test_tasks.py
+++ b/Lib/test/test_asyncio/test_tasks.py
@@ -2202,6 +2202,8 @@ class BaseTaskIntrospectionTests:
         self.assertEqual(asyncio.all_tasks(loop), set())
         self._register_task(loop, task)
         self.assertEqual(asyncio.all_tasks(loop), {task})
+        self._unregister_task(loop, task)
+        self.assertEqual(asyncio.all_tasks(loop), set())

     def test__enter_task(self):
         task = mock.Mock()
@@ -2209,6 +2211,7 @@ class BaseTaskIntrospectionTests:
         self.assertIsNone(asyncio.current_task(loop))
         self._enter_task(loop, task)
         self.assertIs(asyncio.current_task(loop), task)
+        self._leave_task(loop, task)

     def test__enter_task_failure(self):
         task1 = mock.Mock()
@@ -2218,6 +2221,7 @@ class BaseTaskIntrospectionTests:
         with self.assertRaises(RuntimeError):
             self._enter_task(loop, task2)
         self.assertIs(asyncio.current_task(loop), task1)
+        self._leave_task(loop, task1)

     def test__leave_task(self):
         task = mock.Mock()
@@ -2234,6 +2238,7 @@ class BaseTaskIntrospectionTests:
         with self.assertRaises(RuntimeError):
             self._leave_task(loop, task2)
         self.assertIs(asyncio.current_task(loop), task1)
+        self._leave_task(loop, task1)

     def test__leave_task_failure2(self):
         task = mock.Mock()
@@ -2246,13 +2251,13 @@ class BaseTaskIntrospectionTests:
         task = mock.Mock()
         loop = mock.Mock()
         self._register_task(loop, task)
-        self._unregister_task(loop, task)
+        asyncio._unregister_task(loop, task)
         self.assertEqual(asyncio.all_tasks(loop), set())

     def test__unregister_task_not_registered(self):
         task = mock.Mock()
         loop = mock.Mock()
-        self._unregister_task(loop, task)
+        asyncio._unregister_task(loop, task)
         self.assertEqual(asyncio.all_tasks(loop), set())

1st1 · 2017-12-15T21:42:10Z

+
+.. function:: current_task(loop=None):
+
+   Return a currently executed task or ``None`` if no task is running.


Please change to "Return the current running task or None, if no task is running."

1st1 · 2017-12-15T21:43:26Z

+   Return a currently executed task or ``None`` if no task is running.
+
+   If *loop* is ``None`` :func:`get_running_loop` is used for gettung
+   current loop.


gettung -> getting

Please change to "If loop is None, :func:get_running_loop is used to get the current loop."

1st1 · 2017-12-15T21:45:06Z

+.. function:: all_tasks(loop=None):
+
+   Return a set of tasks created for the *loop* (the set can be empty
+   if there is no task exists).


Change to: "Return a set of :class:Task objects created for the loop."

I don't think "the set can be empty if there is no task exists" is needed, I think people will figure it out.

1st1 · 2017-12-15T21:45:42Z

This is ready to be merged. Just please fix two doc nits.

1st1 · 2017-12-16T03:25:17Z

+enter_task(PyObject *loop, PyObject *task)
+{
+    PyObject *item;
+    item = PyDict_GetItem(current_tasks, loop);


Maybe we can further optimize this by using _PyDict_GetItem_KnownHash and _PyDict_SetItem_KnownHash?

asvetlov · 2017-12-16T09:56:08Z

Notes are fixed.
The only thing bothers me: current_task(loop) accepts a loop as regular parameter but asyncio usually accepts it as keyword-only.
Should I change it?

1st1 · 2017-12-16T13:06:35Z

I think it's ok for loop to be a positional parameter for our new functions. The reason it was a keyword-only parameter was that those functions were methods on Task and that's the convention for methods.

1st1 · 2017-12-16T13:27:20Z

 {
    PyObject *item;
-    item = PyDict_GetItem(current_tasks, loop);
+    long hash;


1st1 · 2017-12-16T13:27:27Z

 {
    PyObject *item;
-    item = PyDict_GetItem(current_tasks, loop);
+    long hash;


1st1 · 2017-12-16T13:28:56Z

-    item = PyDict_GetItem(current_tasks, loop);
+    long hash;
+    hash = PyObject_Hash(loop);
+    if (hash == NULL) {


1st1 · 2017-12-16T13:29:11Z

-    item = PyDict_GetItem(current_tasks, loop);
+    long hash;
+    hash = PyObject_Hash(loop);
+    if (hash == NULL) {


1st1 · 2017-12-16T18:25:00Z

Looks like there's some compile/sphinx issue in the updated documentation. Otherwise LGTM. Let's merge this.

Proof of concept

71c327e

asvetlov requested a review from 1st1 as a code owner December 11, 2017 18:42

the-knights-who-say-ni added the CLA signed label Dec 11, 2017

bedevere-bot added the awaiting merge label Dec 11, 2017

Move functions into base_tasks.py

a384a51

1st1 reviewed Dec 11, 2017

View reviewed changes

asvetlov added 8 commits December 11, 2017 22:47

Use WeakKeyDictionary for _all_tasks

c9647a7

Polish implementation

9888405

Merge remote-tracking branch 'upstream/master' into current_task2

8897fd8

Fix C Acceleration

a4d01b8

Add tests for deprecation

8bf63f3

Make C Accelerators for _register_task, _enter_task and _leave_task

9d205dd

Move pythod fallbacks back to base_tasks.py

96ba687

Add tests

dda83b1

asvetlov added 2 commits December 12, 2017 16:58

Add NEWS entry

5a8e598

Fix test name duplication.

c3b1794

1st1 reviewed Dec 12, 2017

View reviewed changes

asvetlov added 4 commits December 14, 2017 16:21

Improve tests

998f316

Merge remote-tracking branch 'upstream/master' into current_task2

a36ca8a

Reorganize imports

5b1cfc2

Refactor code

4cdfd0a

asvetlov added 2 commits December 15, 2017 08:33

Merge remote-tracking branch 'upstream/master' into current_task2

ff5cf49

Add missing decref

1c7f5c2

1st1 mentioned this pull request Dec 15, 2017

bpo-32250: Implement loop.current_task() and loop.all_tasks() #4766

Closed

asvetlov added 3 commits December 15, 2017 10:03

Reorganize code

9abbb89

Add docs/docstrings

8430283

Merge remote-tracking branch 'upstream/master' into current_task2

5fdc96a

1st1 reviewed Dec 15, 2017

View reviewed changes

1st1 reviewed Dec 16, 2017

View reviewed changes

asvetlov added 2 commits December 16, 2017 10:38

Merge remote-tracking branch 'upstream/master' into current_task2

ee26922

Fix notes

21cbb06

1st1 reviewed Dec 16, 2017

View reviewed changes

Fix hash calculation errors

85059e5

Fix markup

06a57d8

asvetlov merged commit 44d1a59 into python:master Dec 16, 2017

bedevere-bot removed the awaiting merge label Dec 16, 2017

asvetlov deleted the current_task2 branch December 16, 2017 19:58



		# WeakKeyDictionary of {Task: EventLoop} containing all tasks alive.
		_all_tasks = weakref.WeakKeyDictionary()


		.. function:: current_task(loop=None):

		Return a currently executed task or ``None`` if no task is running.

Uh oh!

Conversation

asvetlov commented Dec 11, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 11, 2017

Uh oh!

asvetlov commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 12, 2017 • edited by 1st1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 12, 2017

Uh oh!

asvetlov commented Dec 12, 2017

Uh oh!

1st1 commented Dec 14, 2017

Uh oh!

1st1 Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

asvetlov commented Dec 11, 2017 •

edited by bedevere-bot

Loading

asvetlov Dec 12, 2017 •

edited by 1st1

Loading

1st1 Dec 12, 2017 •

edited

Loading

1st1 Dec 12, 2017 •

edited

Loading

1st1 Dec 15, 2017 •

edited

Loading