Skip to content

gh-112622: Pass name to loop create_task method #112623

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

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

ordinary-jamie
Copy link
Contributor

@ordinary-jamie ordinary-jamie commented Dec 2, 2023

Issue: #112622

The name parameter is not being passed to the event loop and is instead being set by the asyncio.create_task function. This means that third party implementations of the event loop will never receive the name parameter. This commit fixes this.

Note Python 3.7 (now end-of-life) is the last version which does not expect a name parameter for asyncio.create_task.

@ghost
Copy link

ghost commented Dec 2, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Dec 2, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Shouldn't you also do this for the C accelerator module _asyncio?

Also, create_task() exists mainly so that e.g. uvloop can override it. Are you sure that uvloop's create_task() takes a name keyword arg?

@ordinary-jamie
Copy link
Contributor Author

Shouldn't you also do this for the C accelerator module _asyncio?

Also, create_task() exists mainly so that e.g. uvloop can override it. Are you sure that uvloop's create_task() takes a name keyword arg?

Shouldn't you also do this for the C accelerator module _asyncio?

Also, create_task() exists mainly so that e.g. uvloop can override it. Are you sure that uvloop's create_task() takes a name keyword arg?

Thanks for the review @gvanrossum! uvloop does indeed accept a name keyword arugment.

I wasn't able to find an equivalent create_task function in the _asynciomodule.c sorry. From what I can tell, there is only the equivalent constructor for asyncio.Task: _asyncio_Task__init__impl which is not affected by this issue.

Please let me know if there is something I missed!

If you're adding the name to the create_task() call, shouldn't you delete this line? It would seem to be redundant.

Good point! I was initially worried that custom event loop implementations may be affected if they have never been setting the name initially. Would it be prudent to check if a name is set after calling the loop method and then applying a name (if the kwarg is not None)? E.g.

if name and task.get_name() is None:
    task.set_name(name)

@gvanrossum
Copy link
Member

Thanks for checking uvloop. And I was fortunately mistaken about _asynciomodule.c.

This leaves the issue of whether we still need the redundant set_name() call.

Unfortunately I also found another place that uses the same pattern: In taskgroups.py there is a create_task() method that uses the same pattern. It can be fixed in the same way and brings up the same question about set_name().

It's also unfortunate that BaseEventLoop.create_task() calls _task_factory() if it is not None, and that is defined as taking exactly (loop, coro) as arguments. This is not easily fixed: we'd have to add a new API set_task_factory_new(), or something like that (naming is hard :-).

Okay, I've got a gut feeling here: let's keep the redundant set_name() but also fix the pattern in taskgroups.py. What do you think?

@ordinary-jamie
Copy link
Contributor Author

Good spot!

To confirm, we won't need set_name() in taskgroups.py::TaskGroup after fixing the pattern there since the loop method should set the name (at least BaseEventLoop does albeit the task factory issue), correct?

@@ -0,0 +1,2 @@
Ensure ``name`` parameter is passed to event loop in
:func:`asyncio.create_task`
Copy link
Member

Choose a reason for hiding this comment

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

Trailing period.

The name parameter is not being passed to the event loop and is instead
being set by the asyncio.create_task function. This means that third
party implementations of the event loop will never receive the name
parameter. This commit fixes this. Note that python3.7 (eol) if the
last version of python that does not support the name parameter.
`TaskGroup.create_task` follows the same pattern as
`asyncio.create_task`, also including a redundant call to
`Task.set_name`. Note that `BaseEventLoop.create_task` uses the
`task_factory` if it is set, in which does not accept a `name` kwarg.
Hence, while we can remove the redundant call in `TaskGroup` we did not
remove it in the `asyncio.create_task`
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yup, let's drop the redundant set_name() calls. I'll merge! Thanks for working through this carefully.

@hugovk
Copy link
Member

hugovk commented Dec 16, 2023

Does this need backporting to 3.12? If not, let's close the issue: #112622.

@gvanrossum
Copy link
Member

No backport, it is a slight API change.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
This affects task creation through either `asyncio.create_task()` or `TaskGroup.create_task()` -- the redundant call to `task.set_name()` is skipped. We still call `set_name()` when a task factory is involved, because the task factory call signature (unfortunately) doesn't take a `name` argument.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
This affects task creation through either `asyncio.create_task()` or `TaskGroup.create_task()` -- the redundant call to `task.set_name()` is skipped. We still call `set_name()` when a task factory is involved, because the task factory call signature (unfortunately) doesn't take a `name` argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants