Skip to content
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

Add names to asyncio tasks #78451

Closed
agronholm mannequin opened this issue Jul 29, 2018 · 21 comments
Closed

Add names to asyncio tasks #78451

agronholm mannequin opened this issue Jul 29, 2018 · 21 comments
Labels
3.8 only security fixes topic-asyncio type-feature A feature request or enhancement

Comments

@agronholm
Copy link
Mannequin

agronholm mannequin commented Jul 29, 2018

BPO 34270
Nosy @benjaminp, @asvetlov, @agronholm, @ericsnowcurrently, @1st1
PRs
  • bpo-34270: added the ability to name asyncio tasks #8547
  • bpo-34270: Fixed inconsistency in string handling in the Task C implementation #8717
  • Make regular expressions in test_tasks.py raw strings. #8759
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-08-08.21:08:30.228>
    created_at = <Date 2018-07-29.09:25:37.510>
    labels = ['type-feature', '3.8', 'expert-asyncio']
    title = 'Add names to asyncio tasks'
    updated_at = <Date 2018-08-14.04:32:34.460>
    user = 'https://github.com/agronholm'

    bugs.python.org fields:

    activity = <Date 2018-08-14.04:32:34.460>
    actor = 'benjamin.peterson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-08-08.21:08:30.228>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2018-07-29.09:25:37.510>
    creator = 'alex.gronholm'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34270
    keywords = ['patch']
    message_count = 21.0
    messages = ['322620', '323302', '323303', '323324', '323325', '323326', '323327', '323328', '323329', '323330', '323334', '323337', '323338', '323339', '323340', '323341', '323342', '323343', '323344', '323345', '323497']
    nosy_count = 5.0
    nosy_names = ['benjamin.peterson', 'asvetlov', 'alex.gronholm', 'eric.snow', 'yselivanov']
    pr_nums = ['8547', '8717', '8759']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34270'
    versions = ['Python 3.8']

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Jul 29, 2018

    Having names on tasks helps tremendously when something goes wrong in a complex asyncio application. Threads have names and even trio has the ability to name its tasks. This would also greatly benefit PyCharm's concurrency visualization: https://www.jetbrains.com/help/pycharm/thread-concurrency-visualization.html#asyncio

    @agronholm agronholm mannequin added 3.8 only security fixes topic-asyncio type-feature A feature request or enhancement labels Jul 29, 2018
    @1st1
    Copy link
    Member

    1st1 commented Aug 8, 2018

    New changeset cca4eec by Yury Selivanov (Alex Grönholm) in branch 'master':
    bpo-34270: Make it possible to name asyncio tasks (GH-8547)
    cca4eec

    @1st1
    Copy link
    Member

    1st1 commented Aug 8, 2018

    Thank you for the contribution!

    @1st1 1st1 closed this as completed Aug 8, 2018
    @ericsnowcurrently
    Copy link
    Member

    FWIW, the C implementation of Task.__init__ is not exactly equivalent to the Python implementation (nor to both the C and Python implementation of Task.set_name). In the C impl of Task.__init__ the provided name is used as-is if it's an instance of str:

    (_asyncio_Task___init___impl() in Modules/_asynciomodule.c)

       if (name == Py_None) {
           name = PyUnicode_FromFormat("Task-%" PRIu64, ++task_name_counter);
       } else if (!PyUnicode_Check(name)) {
           name = PyObject_Str(name);
       } else {
           Py_INCREF(name);
       }

    One of the following should happen, right?

    1. fix the Python implementation of Task.__init__() and both impl of Task.set_name()
    2. change the check to PyUnicode_CheckExact()
    3. remove the special-case (i.e. change the C impl to match the Python impl)

    p.s. Sorry I did not notice this before it got committed. :/

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    1. change the check to PyUnicode_CheckExact()

    I'd be OK with this, but why is this important?

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    As a side note, Alex, what do you think about appending coroutine's name to Task's name if the latter is autogenerated?

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    I also couldn't figure out yet why PyUnicode_Check() was necessary in the first place. Doesn't PyObject_Str() just increment the refcount if the argument is already a string?

    Eric, please explain why these changes should be done.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    Yury, I have no objections. Furthermore, it would be nice to expose the coroutine object publicly, like curio and trio do. It would make life simpler for me in some cases.

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    I also couldn't figure out yet why PyUnicode_Check() was necessary in the first place. Doesn't PyObject_Str() just increment the refcount if the argument is already a string?

    str() returns its argument if it's exactly a builtins.str instance. If it's a subclass of str, it will construct a builtins.str out of it.

    >>> class mystr(str):
    ...     pass
    >>> a = mystr('aaa')
    >>> str(a) is a
    False

    So Eric is right, there's a small discrepancy between Python and C version.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    Ok, I understand. But is the conversion a bad thing then?

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    Ok, I understand. But is the conversion a bad thing then?

    It's not a bad thing, it's just that we don't do it in C Task and we do it in pure Python Task. Eric wants us to synchronize them so that in a very unlikely scenario where someone uses subclasses of str for names they will have exact same behaviour under both Tasks implementations.

    I'd say let's just fix the C version to use PyUnicode_CheckExact. Even though it's highly unlikely somebody ever hits this, there's no reason to keep Python and C implementations even slightly out of sync w.r.t. behaviour.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    It's not a bad thing, it's just that we don't do it in C Task and we do it in pure Python Task. Eric wants us to synchronize them so that in a very unlikely scenario where someone uses subclasses of str for names they will have exact same behaviour under both Tasks implementations.

    Should a new issue be created for this so I can make a PR against it?

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    Let's just reuse this issue, it's just a small fix.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    Which way do we want to change this? Do we want to convert to pure strings or retain the original object? In the latter case both the C and Python implementations (including set_name()) have to be changed.

    @ericsnowcurrently
    Copy link
    Member

    I'm not too invested in any changes happening at this point, actually. :) Mostly I happened to be reading through the commit and noticed the inconsistency. If I had reviewed the PR then I would have asked that it be fixed. So I figured I'd mention it.

    FWIW, I don't expect it would cause any problems. It could result in a different (between the two implementations) Task repr if the name's type (a str subclass) implements __repr__. There's also the possibility of side-effects (from the implementation of the name's type). Neither is a big deal (especially the latter since it's *not* a common use case). On the other had, the matter is made moot by using PyUnicode_CheckExact(). :)

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    On the other had, the matter is made moot by using PyUnicode_CheckExact()

    Then, in order to keep the pure Python implementation in sync, we'd have to change it to something like this:

    if name is None:
       self._name = f'Task-{_task_name_counter()}'
    elif isinstance(name, str):
       self._name = name
    else:
       self._name = str(name)

    I don't know about you, but it looks pretty awkward to me.

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    Please just change PyUnicode_Check to PyUnicode_CheckExact in C Task.__init__ and use the same if check in C Task.set_name.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    Please just change PyUnicode_Check to PyUnicode_CheckExact in C Task.__init__ and use the same if check in C Task.set_name.

    I'll do that if you say so, but I'm just saying that the C and Python implementations will still remain different in semantics then.

    @1st1
    Copy link
    Member

    1st1 commented Aug 9, 2018

    I'll do that if you say so, but I'm just saying that the C and Python implementations will still remain different in semantics then.

    Probably I'm missing something here. How would they be different?

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Aug 9, 2018

    I'll do that if you say so, but I'm just saying that the C and Python implementations will still remain different in semantics then.

    Never mind, that was a brain fart. I keep ignoring the "!" in my mind.

    @benjaminp
    Copy link
    Contributor

    New changeset aa4e4a4 by Benjamin Peterson in branch 'master':
    Make regular expressions in test_tasks.py raw strings. (GH-8759)
    aa4e4a4

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants