gh-120284: Enhance asyncio.Runner's run() method to accept more object types

[ronf]

This commit makes asyncio.Runner's run() method accept the same object types as asyncio.run_until_complete(). Instead of accepting only a coroutine, run() will now accept an awaitable, coroutine, asyncio.Future, or asyncio.Task.

• Issue: asyncio.Runner explicitly checks for coroutine #120284

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

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.

[bedevere-app]

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.

[ronf]

Let me know if you'd like me to create a NEWS entry for this.

[gvanrossum]

Yes please

[kumaraditya303]

You replace most of this with call to ensure_future.

[ronf]

That's what I did originally, but it caused problems with some of the unit tests. In particular, ensure_future() had a call to get_event_loop() in it which caused a unit test to fail. Also, in my original change, I needed to modify ensure_future() to take a context argument and that changed a public API, which caused other associated unit test issues. More importantly, ensure_future() also had code which called close() on the coro or future, which isn't wanted here.

After trying out various alternatives, I settled on just taking the few lines from ensure_future() that were needed and modifying them to include only what this new code required. It was a smaller change, and in my opinion lower risk and more readable.

[kumaraditya303]

Maybe extract it to a new function? I really don't want to repeat the logic of awaitable trick here.

[ronf]

I'll take another look at this tonight.

[ronf]

Ok - I figured out a way to leverage the code in ensure_future() for this - see commit 4ed7357.

One thing I'm not thrilled about here is that this adds a new optional keyword argument to ensure_future(), which is a public API. I could move the entire logic of ensure_future() into a new function instead to avoid changing its signature, but that's kind of ugly in a different way.

One other thing to note is that this now raises a TypeError instead of a ValueError when a caller passes in a coro which is not one of the expected types, since that's what ensure_future() raised.

[willingc]

Thanks for the PR.

[Jason-Y-Z]

Thanks for the PR! Overall logic makes sense to me, left a small comment

[picnixz]

Some nitpicks (feel free to argue).

[picnixz]

Maybe mention that the current context is used if none is specified (I think the other functions/methods that support contextvars do it).

[ronf]

From what I can tell, ensure_future() sets no context at all if you pass in None to it. It calls create_task() on the current event loop with no context argument in that case. What happens from there would depend on the event loop being used.

I do see code in the Task() constructor which calls contextvars.copy_context() if you pass None in as a context and that's what event loops usually call. I'm hesitant to claim that behavior at the ensure_future() level, though, given the multiple layers in between and the possibility for the event loops to do something different.

Looking at Runner.run(), it passes in the runner's default context if no context is specified when Runner.run() is called, and it is documented as such. That default context seems to be set to a copy of the context active at the time the Runner object is first used (during its lazy init). That then gets passed explicitly to ensure_future() if no context is passed into Runner.run().

Note that the runner's default context may not always be the same as the context which is active at the time Runner.run() is called.

[picnixz]

You're entirely right. I incorrectly assumed that it would be like Runner.run() but that's not what the code actually does. Maybe we could expand the docs by saying:

-custom :class:`contextvars.Context` to use when creating a new task.
+custom :class:`contextvars.Context` to use when creating a new task
+via :meth:`loop.create_task <asyncio.loop.create_task>`.

[ronf]

I don't think the loop version of the create_task() method is a public function. There's only asyncio.create_task() (and more recently TaskGroup.create_task), but those are not used by Runner.

[picnixz]

Then I really wonder how we can teach the None argument in that case. I don't have any good suggestion but we can also decide to be deliberately imprecise... I'll leave it to another reviewer :/

[ronf]

I looked this over again, and it turns out that the same indirection happening with ensure_future() calling into the internal create_task() on the event loop also happens in asyncio.create_task(), and yet it documents context as:

   An optional keyword-only *context* argument allows specifying a
   custom :class:`contextvars.Context` for the *coro* to run in.
   The current context copy is created when no *context* is provided.

Given that both of these are likely to call the constructor for Task which does the actual copy and documents that:

   An optional keyword-only *context* argument allows specifying a
   custom :class:`contextvars.Context` for the *coro* to run in.
   If no *context* is provided, the Task copies the current context
   and later runs its coroutine in the copied context.

So, perhaps it's ok after all to document context in ensure_future() as something like:

   An optional keyword-only *context* argument allows specifying a
   custom :class:`contextvars.Context` to use when creating a new task.
   A copy of the current context is used when no *context* is provided.

There's a chance that a custom event loop could fail to pass the context through properly and change this behavior but that's fairly unlikely (and might in fact be considered a bug).

[1st1]

Suggested change

	.. versionchanged:: 3.12
	.. versionchanged:: 3.13

[1st1]

The proposed implementation makes ensure_future unpredictable so I suggest taking a different route.

[1st1]

Sadly this change is a no-go.

If coro_or_future is a future or task, the context argument would be ignored, breaking the future user assumption that the context they pass would actually be applied.

I suggest a different approach to this PR:

Runner.run(o) should check if o is a coroutine -- if it is, keep things as is. If not, it should wrap it in an ad-hoc "proxy" coroutine async def proxy(o=o): return await o and pass proxy to create_task().

[ronf]

I have made the requested changes; please review again.

[ronf]

(apologies Yury for misspelling your first name in the last commit!)

[bedevere-app]

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.

[kumaraditya303]

Looks fine but the docs also need to be updated for this.

[ronf]

I've updated the documentation in asyncio-runner.rst for Runner.run() and asyncio.run(). In the process, I removed documentation I had previously added in runners.py (before realizing the docs weren't inline).

resolved

[ronf]

Thanks very much!

[1st1]

@kumaraditya303 Kumar, please don't "resolve" my requests like this. If I review something I usually want to see the final thing too.