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

AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Future #84959

Closed
jamesba mannequin opened this issue May 26, 2020 · 8 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@jamesba
Copy link
Mannequin

jamesba mannequin commented May 26, 2020

BPO 40782
Nosy @gvanrossum, @asvetlov, @1st1, @miss-islington, @aeros, @jamesba
PRs
  • bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine #21852
  • [3.8] bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine (GH-21852) #21903
  • [3.9] bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine (GH-21852) #21904
  • 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 2020-08-17.14:41:46.469>
    created_at = <Date 2020-05-26.15:45:38.680>
    labels = ['type-bug', '3.8', '3.9', '3.10', '3.7', 'library', 'expert-asyncio']
    title = 'AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Future'
    updated_at = <Date 2020-08-17.14:41:46.468>
    user = 'https://github.com/jamesba'

    bugs.python.org fields:

    activity = <Date 2020-08-17.14:41:46.468>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-08-17.14:41:46.469>
    closer = 'gvanrossum'
    components = ['Library (Lib)', 'asyncio']
    creation = <Date 2020-05-26.15:45:38.680>
    creator = 'jamesba'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40782
    keywords = ['patch']
    message_count = 8.0
    messages = ['370005', '370045', '372735', '375278', '375375', '375548', '375551', '375552']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'asvetlov', 'yselivanov', 'miss-islington', 'aeros', 'jamesba']
    pr_nums = ['21852', '21903', '21904']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue40782'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

    @jamesba
    Copy link
    Mannequin Author

    jamesba mannequin commented May 26, 2020

    As discussed in < python/typeshed#3999 (comment) > the type of AbstractEventLoop.run_in_executor is defined at < https://github.com/python/cpython/blob/master/Lib/asyncio/events.py#L286 > as follows:

    async def run_in_executor(self, executor, func, *args):
            raise NotImplementedError
    

    However all concrete implementations of this method are actually not async methods but rather synchronous methods which return a Future object.

    Logically this appears to make sense: at base run_in_executor is not a coroutine, since it doesn't create an object representing code which will be executed when the object is awaited, rather it returns an object representing code which is running asynchronously elsewhere (on another thread) and which can be awaited to wait for that other thread to complete its task. Which seems to be a perfect match to what a Future object is supposed to be.

    As such it seems that the current definition of the method as a coroutine is possibly a mistake.

    Alternatively if some feel that it is important to allow concrete implementations to implement it as a coroutine if they need to then perhaps it could be specified to be a method returning an Awaitable, since that would cover both options?

    @jamesba jamesba mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 26, 2020
    @aeros
    Copy link
    Contributor

    aeros commented May 27, 2020

    From looking at the commit history of AbstactEventLoop.run_in_executor(), it seems that it was previously be a non-coroutine method prior to the conversion from the @asyncio.coroutine decorator to async def (PR-4753). See https://github.com/python/cpython/blame/ede157331b4f9e550334900b3b4de1c8590688de/Lib/asyncio/events.py#L305.

    The only context for the change I can find is the following conversation between Andrew and Yury: #4753 (comment). However, the example provided of connect_read_pipe() had already been a coroutine at the time for the BaseEventLoop implementation, which makes sense in that case. So, it's not clear to me as to why run_in_executor() was also converted to "async def" when its main implementation is not a coroutine. Furthermore, it's documented as an awaitable rather than a coroutine (https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor).

    @andrew do you have any additional context to provide that I'm potentially missing?

    @jamesba
    Copy link
    Mannequin Author

    jamesba mannequin commented Jul 1, 2020

    Is there any further movement on this?

    @gvanrossum
    Copy link
    Member

    I think it makes sense to remove the async from the definition in AbstractEventLoop.

    If you want to help, you can submit a PR to do it.

    @serhiy-storchaka serhiy-storchaka changed the title AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Futrue AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Future Aug 13, 2020
    @serhiy-storchaka serhiy-storchaka changed the title AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Futrue AbstactEventLoop.run_in_executor is listed as an async method, but should actually return a Future Aug 13, 2020
    @jamesba
    Copy link
    Mannequin Author

    jamesba mannequin commented Aug 14, 2020

    #21852

    @miss-islington
    Copy link
    Contributor

    New changeset 29f8429 by James Weaver in branch 'master':
    bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine (GH-21852)
    29f8429

    @miss-islington
    Copy link
    Contributor

    New changeset 1baa8b1 by Miss Islington (bot) in branch '3.8':
    bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine (GH-21852)
    1baa8b1

    @miss-islington
    Copy link
    Contributor

    New changeset d6bdf6d by Miss Islington (bot) in branch '3.9':
    bpo-40782: Change asyncio.AbstractEventLoop.run_in_executor to be a method not a coroutine (GH-21852)
    d6bdf6d

    @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.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants