bpo-34270: added the ability to name asyncio tasks by agronholm · Pull Request #8547 · python/cpython

agronholm · 2018-07-29T11:41:28Z

The ability to name individual tasks helps developers debug complex
asyncio applications.

Co-authored-by: Antti Haapala [email protected]

https://bugs.python.org/issue34270

The ability to name individual tasks helps developers debug complex asyncio applications. Co-authored-by: Antti Haapala <[email protected]>

the-knights-who-say-ni · 2018-07-29T11:41:31Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

You can check yourself
to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

agronholm · 2018-07-29T11:44:00Z

I've updated my account to include my github name, so the CLA label should be cleared.

1st1 · 2018-07-29T11:51:17Z

I'll review this tomorrow

ztane · 2018-07-29T12:21:27Z

Co-author here (CLA signed)

zooba

Wait for Yury, but it looks good to me. One question about the need to convert names to str on assignment, but not a big deal.

zooba · 2018-07-29T12:50:27Z

+
+    @name.setter
+    def name(self, value):
+        self._name = str(value)


Is forcing this to a str really important enough to need a property? The name is basically only ever used for printing anyway, which will do the conversion when necessary.

This is consistent with the behavior of threading.Thread. Some code might also access the name property directly, expecting a str.

As I mentioned above I don't think we need a setter. A Task.get_name() function is enough for now.

You can set names of threads and processes too, why not tasks as well? Where's the harm in that? I can think of a few use cases for this, like displaying the status of a task as it progresses.
I could convert this to java-style setters and getters if you want, despite how much I despise them (not to mention the fact that threading also switched away from this behavior). But your call.

I can think of a few use cases for this, like displaying the status of a task as it progresses.

Hm, interesting. OK, that's at least one valid use-case :)

I could convert this to java-style setters and getters if you want, despite how much I despise them (not to mention the fact that threading also switched away from this behavior). But your call.

It's not about liking Java-style getters or not, it's about consistency. If asyncio's APIs were using @property it'd be equally happy about that, but now we better follow the overall style and use get_name() and set_name().

elprans · 2018-07-30T14:27:45Z

 from .coroutines import coroutine

+# Helper to generate new task names
+_counter = itertools.count(1).__next__


Using itertools.count() seems like a bit of an overkill to me. I'd simply do an explicit increment. Also, _task_name_counter would be a clearer name.

Indeed this seems like an overkill. There's no reason to make Task creation any slower than it has to be; keep in mind that PyPy uses the pure Python version. Just replace this with a simple int increment.

This is what the threading module does, and it uses itertools.count() because _counter += 1 is not thread safe. The same applies to asyncio if there are more than one thread running an event loop (since the counter is global).

_counter += 1 is not thread safe.

It is thread safe in Python and will likely always be with or without the GIL.

It is thread safe in Python and will likely always be with or without the GIL.

What output does this script give you then?

Hmmm... thank you for pointing this out! ;)

To my point: += is actually atomic, but storing the updated number in globals is another operation and we have a race between them. This actually deserves a comment in the code explaining why we use counter() and not +=.

1st1 · 2018-07-30T14:31:51Z

 -----

-.. method:: AbstractEventLoop.create_task(coro)
+.. method:: AbstractEventLoop.create_task(coro, \*, name=None)


We also need to update the documentation of set_task_factory

Good point.

1st1 · 2018-07-30T14:33:31Z

      get_stack().  The file argument is an I/O stream to which the output
      is written; by default output is written to sys.stderr.

+   .. attribute:: name


I care way more about consistency of asyncio APIs than about maintaining any kind of artificial similarity between Task and Thread. And in asyncio we don't use properties that much. Please replace the name property with a Task.get_name() method. There's no use case for adding set_name() though, so let's not add it at least in 3.8.

1st1 · 2018-07-30T14:36:03Z


    if task._fut_waiter is not None:
-        info.insert(2, f'wait_for={task._fut_waiter!r}')
+        info.insert(3, f'wait_for={task._fut_waiter!r}')


BTW, feel free to make another pull request to refactor this code. I don't like us modifying some list object with insert() calls.

How would you prefer that we display the name of the task in repr() then?

It would be great to refactor the code that formats things (tasks, callbacks, reprs) etc somehow. Right now it feels hacky and it's one of the areas of asyncio code that I'd like to be rewritten to be clearer. Discussing that is slightly out of the scope of this PR though, but if you have any ideas how to avoid mutating lists and having a non-linear formatting logic I'd be happy to accept a new PR. For this PR, your current code is totally fine!

1st1 · 2018-07-30T14:36:53Z

 from .coroutines import coroutine

+# Helper to generate new task names
+_counter = itertools.count(1).__next__


Indeed this seems like an overkill. There's no reason to make Task creation any slower than it has to be; keep in mind that PyPy uses the pure Python version. Just replace this with a simple int increment.

1st1 · 2018-07-30T14:37:53Z

            self._log_destroy_pending = False
            raise TypeError(f"a coroutine was expected, got {coro!r}")

+        self._name = str(name) if name is not None else 'Task-%s' % _counter()


This is a bit too hard to read. I'd replace it with an if statement.

1st1 · 2018-07-30T14:38:28Z

+
+    @name.setter
+    def name(self, value):
+        self._name = str(value)


As I mentioned above I don't think we need a setter. A Task.get_name() function is enough for now.

1st1 · 2018-07-30T14:39:07Z

 static volatile uint64_t cached_running_holder_tsid;

+/* Counter for autogenerated Task names */
+static unsigned long long task_name_counter = 0;


Please use uint64_t.

I had that at first but @ztane pointed out that the format string explicitly requires an unsigned long long and uint64_t may or may not be the same size. Are you sure this is ok?

Got it. AFAIK unsigned long long is guaranteed to be large enough to store uint64_t. Using the latter makes the code more consistent, so I'd go for it anyways.

You can use this format specifier to format uint64_t specifically:

printf("... %" PRIu64 "...", (uint64_t)1);

1st1 · 2018-07-30T14:41:58Z

+    } else {
+        name = PyObject_Str(name);
+    }
+


Please remove this new line to make it easier for the reader to see that we handle the case of name==NULL

bedevere-bot · 2018-07-30T14:42:33Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

1st1 · 2018-07-30T16:41:30Z

BTW, please also add a What's New entry for 3.8.

asvetlov · 2018-07-30T16:48:43Z

I like the idea!

* Removed the public *name* property * Added the *Task.get_name()* method * Modified the documentation of *set_task_factory()* to include the *name* parameter * Whitespace changes

agronholm · 2018-07-30T20:36:49Z

I have made the requested changes; please review again.

bedevere-bot · 2018-07-30T20:36:51Z

Thanks for making the requested changes!

@zooba, @1st1: please review the changes made to this pull request.

agronholm · 2018-08-08T19:27:31Z

I have made the requested changes; please review again.

bedevere-bot · 2018-08-08T19:27:33Z

Thanks for making the requested changes!

@zooba, @1st1: please review the changes made to this pull request.

1st1 · 2018-08-08T19:30:11Z

            raise TypeError(f"a coroutine was expected, got {coro!r}")

+        if name is None:
+            self._name = 'Task-%s' % _task_name_counter()


nit: I've recently updated asyncio to use f-strings. Please change this one to either an f-string or to a .format() call (just for maintaining consistency of the codebase).

1st1 · 2018-08-08T19:31:48Z

        task._log_destroy_pending = False
        coro.close()

+    def test_create_named_task_with_custom_factory(self):


Please also add test_create_named_task_with_default_factory. That would test a bug we fixed in this PR when the default task factory didn't pass the name=name to Task().

1st1 · 2018-08-08T19:32:18Z

Almost there. Please fix the two remaining nit comments and I'll merge this.

1st1

Just spotted an error, a PyObject* function returns -1. Please also check that the C compiler generates 0 warnings for your patch in _asynciomodule.c.

1st1 · 2018-08-08T19:52:54Z

+{
+    PyObject *name = PyObject_Str(value);
+    if (name == NULL) {
+        return -1;


Oups, need to fix this to return NULL!

With make clean; CFLAGS=-Wno-cast-function-type make, compilation of the _asyncio extension produced no warnings after the patch.

bedevere-bot · 2018-08-08T19:54:16Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

agronholm · 2018-08-08T20:52:43Z

I have made the requested changes; please review again.

bedevere-bot · 2018-08-08T20:52:46Z

Thanks for making the requested changes!

@zooba, @1st1: please review the changes made to this pull request.

1st1 · 2018-08-08T21:07:58Z

Merged. Thanks for much for pushing this through, Alex!

bpo-34270: added the "name" attribute to asyncio tasks

19204e1

The ability to name individual tasks helps developers debug complex asyncio applications. Co-authored-by: Antti Haapala <[email protected]>

agronholm requested review from 1st1 and asvetlov as code owners July 29, 2018 11:41

the-knights-who-say-ni added the CLA not signed label Jul 29, 2018

bedevere-bot added the awaiting review label Jul 29, 2018

zooba approved these changes Jul 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 29, 2018

elprans reviewed Jul 30, 2018

View reviewed changes

1st1 reviewed Jul 30, 2018

View reviewed changes

1st1 requested changes Jul 30, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jul 30, 2018

Mariatta removed the CLA not signed label Jul 30, 2018

the-knights-who-say-ni added the CLA signed label Jul 30, 2018

agronholm added 4 commits July 30, 2018 19:01

Made changes requested by the reviewers

25939b9

* Removed the public *name* property * Added the *Task.get_name()* method * Modified the documentation of *set_task_factory()* to include the *name* parameter * Whitespace changes

Added whatsnew entry

369eba9

Changed unsigned long long to uint64_t

ce8e014

Added explanation for the use of itertools.count() in asyncio.tasks

11258ed

bedevere-bot removed the awaiting changes label Jul 30, 2018

bedevere-bot added the awaiting change review label Jul 30, 2018

agronholm added 3 commits August 8, 2018 21:33

Made the changes requested by the reviewer

dd4d4a5

Fixed segfault if "name" is already a string

860d67a

Fixed the new test to use a custom factory

2cca5e7

1st1 reviewed Aug 8, 2018

View reviewed changes

Fixed last two nits

a1be2e0

1st1 approved these changes Aug 8, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 8, 2018

1st1 requested changes Aug 8, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Aug 8, 2018

Fixed incompatible return value from set_name()

2ee3972

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 8, 2018

1st1 approved these changes Aug 8, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 8, 2018

1st1 merged commit cca4eec into python:master Aug 8, 2018

bedevere-bot removed the awaiting merge label Aug 8, 2018

agronholm deleted the tasknames branch August 9, 2018 08:27

graingert mentioned this pull request Jul 24, 2022

AttributeError: 'Task' object has no attribute 'uncancel'. Did you mean: 'cancel'? when the asyncio loop.set_task_factory factory returns a asyncio.Future-compatible object #95097

Closed

Uh oh!

Conversation

agronholm commented Jul 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jul 29, 2018

Uh oh!

agronholm commented Jul 29, 2018

Uh oh!

1st1 commented Jul 29, 2018

Uh oh!

ztane commented Jul 29, 2018

Uh oh!

zooba left a comment

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!

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!

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!

bedevere-bot commented Jul 30, 2018

Uh oh!

1st1 commented Jul 30, 2018

Uh oh!

asvetlov commented Jul 30, 2018

Uh oh!

agronholm commented Jul 30, 2018

Uh oh!

bedevere-bot commented Jul 30, 2018

Uh oh!

agronholm commented Aug 8, 2018

Uh oh!

bedevere-bot commented Aug 8, 2018

Uh oh!

agronholm commented Jul 29, 2018 •

edited by bedevere-bot

Loading