Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Comments

bpo-32250: Implement asyncio.current_task() and asyncio.all_tasks()#4799

Merged
asvetlov merged 28 commits intopython:masterfrom
asvetlov:current_task2
Dec 16, 2017
Merged

bpo-32250: Implement asyncio.current_task() and asyncio.all_tasks()#4799
asvetlov merged 28 commits intopython:masterfrom
asvetlov:current_task2

Conversation

@asvetlov
Copy link
Contributor

@asvetlovasvetlov commentedDec 11, 2017•
edited by bedevere-bot
Loading

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

@1st1@bdarnell please take a look

https://bugs.python.org/issue32250


from . import base_futures
from . import coroutines
from .events import get_running_loop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Import the module, not the function.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

asyncio.all_tasks() should useget_running_loop().

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

@1st1
Copy link
Member

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
Copy link
ContributorAuthor

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.



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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Whyweakref.WeakKeyDictionary and notweakref.WeakValueDictionary?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A task should be weak, on task garbage collection an item from_all_tasks should disappear.asyncio on master branch usesWeakSet of tasks.
Buttask._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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Alright. This deserves to be put in a comment.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

ok

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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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}.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Leaving task {task!r} does not matchthe current task {current_task!r}.

from . import events
from . import futures
from .coroutines import coroutine
from .base_tasks import (all_tasks, current_task, _register_task,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please import the module, not functions from it.

Copy link
ContributorAuthor

@asvetlovasvetlovDec 12, 2017•
edited by 1st1
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

In this casebase_tasks should have__all__ = ('all_tasks', 'current_task') to export names intoasyncio namespace._register_task cannot be exposed bybase_tasks, see comment below.

"use asyncio.current_task() instead",
PendingDeprecationWarning,
stacklevel=2)
return current_task(loop)
Copy link
Member

@1st11st1Dec 12, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So we want to useasyncio.get_running_loop() inasyncio.current_task(). And we want to keep usingasyncio.get_event_loop() inTask.current_task() to maintain backwards compat.

@classmethoddefcurrent_task(cls,loop=None):ifloopisNone:loop=events.get_event_loop()warnings.warn(..)returncurrent_task(loop)


def current_task(loop=None):
if loop is None:
loop = events.get_event_loop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Useevents.get_running_loop() here -- we want to use the new API here. It doesn't make sense to callasyncio.current_task() and accidentally create a new event loop becauseget_event_loop() thinks so.

_PyTask = Task
_py_register_task = _register_task
_py_enter_task = _enter_task
_py_leave_task = _leave_task
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You should do this logic inbase_tasks.py.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

That's why I've decided to have both_py_register_task and_c_register_task in the sameasyncio.tasks module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Then define all these functions (and_all_tasks and_current_tasks mappings) in_asynciomodule.c and try to import them intasks.py. If that fails, define them in Python. Let's not touchbase_tasks.py at all in this PR.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If we define_all_tasks in_asynciomodule.c -- C Accelerator becomes mandatory.
Let's define_all_tasks and_current_tasks inbase_tasks.py but move_register_task and family intotasks.py.

Copy link
Member

@1st11st1Dec 12, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

C Accelerator becomes mandatory.

No, why?

_asynciomodule.c:

// defines _all_tasks, _current_tasks

asyncio/tasks.py:

try:from_asyncioimport_all_tasks,_current_tasksexceptImportError:_all_tasks= ..._current_tasks= ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I actually wanted to make a separate PR

I can make that PR myself now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

working on it.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 inbase_tasks.py. I'm not blocking you from working on this PR, right?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, you are not blocking.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You should do this logic inbase_tasks.py.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I prefer not to use_impl functions in C code. Better to implementint enter_task(loop, task) function and use it inTask implementation directly. Then you call that function in_asyncio__enter_task_impl and returnNULL/None based on its return value. This way you also don't need to decrefNone inTask code, and the code is overall cleaner.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok

@1st1
Copy link
Member

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

Absolutely, a separate PR is the way to go.

@asvetlov
Copy link
ContributorAuthor

Do you agree with usingget_event_loop() inasyncio.all_tasks()?asyncio.current_task() will be switched toget_running_loop().

@1st1
Copy link
Member

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.pyindex 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())


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

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

@1st11st1Dec 15, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

gettung -> getting

Please change to "Ifloop isNone, :func:get_running_loop is used to get the current loop."

.. 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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
Copy link
Member

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

enter_task(PyObject *loop, PyObject *task)
{
PyObject *item;
item = PyDict_GetItem(current_tasks, loop);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe we can further optimize this by using_PyDict_GetItem_KnownHash and_PyDict_SetItem_KnownHash?

@asvetlov
Copy link
ContributorAuthor

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

@1st1
Copy link
Member

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.

{
PyObject *item;
item = PyDict_GetItem(current_tasks, loop);
long hash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Py_hash_t

{
PyObject *item;
item = PyDict_GetItem(current_tasks, loop);
long hash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Py_hash_t

item = PyDict_GetItem(current_tasks, loop);
long hash;
hash = PyObject_Hash(loop);
if (hash == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

== -1

item = PyDict_GetItem(current_tasks, loop);
long hash;
hash = PyObject_Hash(loop);
if (hash == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

== -1

@1st1
Copy link
Member

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

@asvetlovasvetlov merged commit44d1a59 intopython:masterDec 16, 2017
@asvetlovasvetlov deleted the current_task2 branchDecember 16, 2017 19:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@1st11st11st1 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@asvetlov@1st1@the-knights-who-say-ni@bedevere-bot
[8]ページ先頭
©2009-2026 Movatter.jp