Specifying AbstractEventLoop.run_in_executor as a coroutine conflicts with implementation/intent

[chrahunt]

BPO	35792
Nosy	@asvetlov, @1st1, @chrahunt

^{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 = None
created_at = <Date 2019-01-21.02:59:18.644>
labels = ['type-bug', '3.7', 'expert-asyncio']
title = 'Specifying AbstractEventLoop.run_in_executor as a coroutine conflicts with implementation/intent'
updated_at = <Date 2019-04-20.14:18:31.754>
user = 'https://github.com/chrahunt'

bugs.python.org fields:

activity = <Date 2019-04-20.14:18:31.754>
actor = 'chrahunt'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2019-01-21.02:59:18.644>
creator = 'chrahunt'
dependencies = []
files = []
hgrepos = []
issue_num = 35792
keywords = []
message_count = 4.0
messages = ['334109', '340533', '340545', '340574']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'chrahunt']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35792'
versions = ['Python 3.7']

[chrahunt]

Currently AbstractEventLoop.run_in_executor is specified as a coroutine, while BaseEventLoop.run_in_executor is actually a non-coroutine that returns a Future object. The behavior of BaseEventLoop.run_in_executor would be significantly different if changed to align with the interface . If run_in_executor is a coroutine then the provided func will not actually be scheduled until the coroutine is awaited, which conflicts with the statement in PEP-3156 that it "is equivalent to wrap_future(executor.submit(callback, *args))".

There has already been an attempt in bpo-32327 to convert this function to a coroutine. We should change the interface specified in AbstractEventLoop to indicate that run_in_executor is not a coroutine, which should help ensure it does not get changed in the future without full consideration of the impacts.

[asvetlov]

I would rather change the implementation by converting it into async function.
It can break some code, sure -- but in a very explicit way (coroutine run_in_executor is never awaited error).
Making existing third-party code forward-compatible is trivial: just push await before the call.

[chrahunt]

My use case is scheduling work against an executor but waiting on the results later (on demand).

If converting BaseEventLoop.run_in_executor(executor, func, *args) to a coroutine function, I believe there are two possible approaches (the discussion that started this here only considers [impl.1]):

impl.1) BaseEventLoop.run_in_executor still returns a future, but we must await the coroutine object in order to get it (very breaking change), or
impl.2) BaseEventLoop.run_in_executor awaits on the result of func itself and returns the result directly

In both cases the provided func will only be dispatched to executor when the coroutine object is scheduled with the event loop.

For [impl.1], from the linked discussion, there is an example of user code required to get the behavior of schedule immediately and return future while still using BaseEventLoop.run_in_executor:

    async def run_now(f, *args):
        loop = asyncio.get_event_loop()
        started = asyncio.Event()
        def wrapped_f():
            loop.call_soon_threadsafe(started.set)
            return f(*args)
        fut = loop.run_in_executor(None, wrapped_f)
        await started.wait()
        return fut

however this wrapper would only be possible to use in an async function and assumes the executor is running in the same process - synchronous functions (e.g. an implementation of Protocol.data_received) would need to use an alternative my_run_in_executor:

    def my_run_in_executor(executor, f, *args, loop=asyncio.get_running_loop()):
        return asyncio.wrap_future(executor.submit(f, *args), loop=loop)

either of these would need to be discovered by users and live in their code base.

Having to use my_run_in_executor would be most unfortunate, given the purpose of run_in_executor per the PEP is to be a shorthand for this exact function.

For [impl.2], we are fine if the use case allows submitting and awaiting the completion of func in the same location, and no methods of asyncio.Future (e.g. add_done_callback, cancel) are used. If not then we still need to either:

soln.1) use my_run_in_executor, or
soln.2) wrap the BaseEventLoop.run_in_executor coroutine object/asyncio.Future with asyncio.ensure_future

[soln.1] is bad for the reason stated above: this is the function we are trying to avoid users having to write.

[soln.2] uses the low-level function asyncio.ensure_future because both of the suggested alternatives (per the docs) asyncio.create_task and BaseEventLoop.create_task throw a TypeError when provided an asyncio.Future as returned by the current implementation of BaseEventLoop.run_in_executor. This will have to be discovered by users and exist in their code base.

[chrahunt]

For impl.1:

(very breaking change)

should be

(very breaking change, mitigated some by the fact that the implementation will warn about the unawaited future)

[kumaraditya303]

This will be backwards incompatible change to change it to a coroutine and so far this doesn't seem to be a problem. I am -1 on changing this.

[gvanrossum]

In GH-21852, AbstractEventLoop.run_in_executor was changed to a plain function (still raising NotImplementedError). So this was fixed already.