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

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

Co-authored-by: Antti Haapala antti.haapala@anttipatterns.com

https://bugs.python.org/issue34270

The ability to name individual tasks helps developers debug complex asyncio applications. Co-authored-by: Antti Haapala <antti.haapala@anttipatterns.com>

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!

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

I'll review this tomorrow

Co-author here (CLA signed)

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.

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

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 +=.

We also need to update the documentation of set_task_factory

Good point.

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.

Got it.

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!

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 a bit too hard to read. I'd replace it with an if statement.

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

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);

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

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.

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

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

I have made the requested changes; please review again.

Thanks for making the requested changes!

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

I have made the requested changes; please review again.

Thanks for making the requested changes!

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

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

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

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

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.

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.

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.

I have made the requested changes; please review again.

Thanks for making the requested changes!

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

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 <antti.haapala@anttipatterns.com>

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

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

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

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

                     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

                   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}')

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

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

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

                           raise TypeError(f"a coroutine was expected, got {coro!r}")
+                      if name is None:
+                          self._name = 'Task-%s' % _task_name_counter()

                       task._log_destroy_pending = False
                       coro.close()
+                  def test_create_named_task_with_custom_factory(self):

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

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

Uh oh!

Uh oh!

bpo-34270: added the ability to name asyncio tasks - #8547

bpo-34270: added the ability to name asyncio tasks#85471st1 merged 22 commits intopython:masterpython/cpython:masterfrom agronholm:tasknamesagronholm/cpython:tasknamesCopy head branch name to clipboard

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!

bpo-34270: added the ability to name asyncio tasks#8547
1st1 merged 22 commits into
python:masterfrom
agronholm:tasknames

agronholm commented Jul 29, 2018 •
edited by bedevere-bot
Loading