asyncio.Runner explicitly checks for coroutine

[ronf]

The class asyncio.Runner currently checks explicitly that its coro argument is a coroutine:

cpython/Lib/asyncio/runners.py

Lines 87 to 89 in 46e69f8

	"""Run a coroutine inside the embedded event loop."""
	if not coroutines.iscoroutine(coro):
	raise ValueError("a coroutine was expected, got {!r}".format(coro))

However, shouldn't this allow any awaitable, checked via inspect.isawaitable(), or perhaps aligned with run_until_complete(), which currently accepts a Future object? Is there a specific reason why only coroutines are allowed here?

I've got a class which implements a custom __await__() method that I'd like to be able to pass into asyncio.Runner, but the check above is preventing this. There are workaround for this, like wrapping the await on the object in a regular async function, but I'd prefer not to require that.

Linked PRs

• gh-120284: Enable asyncio.runners.Runner to run an awaitable object. #120306
• gh-120284: Enhance asyncio.Runner's run() method to accept more object types #120566

[gvanrossum]

I believe the coroutine check exists because this is one of the first APIs that people encounter when learning to use asyncio, and good diagnostics for incorrect usage are important.

As a compromise, maybe something that has an __await__ method should also be acceptable (it's hard to see how a beginner could accidentally construct something like that -- if they pass a Future or Task it should just work, right?).

Are you interested in submitting a PR?

[ronf]

Thanks for the quick response! I agree that there's value in doing an explicit check here to catch errors in usage early and print a better error message. However, I'd also like to see asyncio.Runner accept the same set of types as asyncio.run_until_complete() does, as long as all of them can be made to work properly.

From some testing I did here, an async def function passes the asyncio.iscoroutine() check. However, neither an asyncio.Future nor an asyncio.Task is considered a coroutine, and as such they are not currently accepted by the run() function in asyncio.Runner. As noted above, instances of a class with an __await__() method are also not accepted.

I tested all four of these cases against loop.run_until_complete() and all four cases ran just fine. However, with asyncio.Runner as it stands right now, only the first of these options is supported.

Unfortunately, just replacing the call to asyncio.iscoroutine() with inspect.isawaitable() isn't quite enough to fix this, as there is a later call to loop.create_task() which has its own test to see if the coro passed into it is a coroutine or not. So, I'll need to do a little more digging into loop.run_until_complete() to see how it handles all of these cases. My initial guess is that it may be related to a call it makes to ensure_future(), but I need to study the code more closely.

I'd be happy to try and put together a PR for this.

[gvanrossum]

That all makes sense. Ping us here when you have a PR or questions.

[pygeek]

Thanks for the quick response! I agree that there's value in doing an explicit check here to catch errors in usage early and print a better error message. However, I'd also like to see asyncio.Runner accept the same set of types as asyncio.run_until_complete() does, as long as all of them can be made to work properly.

From some testing I did here, an async def function passes the asyncio.iscoroutine() check. However, neither an asyncio.Future nor an asyncio.Task is considered a coroutine, and as such they are not currently accepted by the run() function in asyncio.Runner. As noted above, instances of a class with an __await__() method are also not accepted.

I tested all four of these cases against loop.run_until_complete() and all four cases ran just fine. However, with asyncio.Runner as it stands right now, only the first of these options is supported.

Unfortunately, just replacing the call to asyncio.iscoroutine() with inspect.isawaitable() isn't quite enough to fix this, as there is a later call to loop.create_task() which has its own test to see if the coro passed into it is a coroutine or not. So, I'll need to do a little more digging into loop.run_until_complete() to see how it handles all of these cases. My initial guess is that it may be related to a call it makes to ensure_future(), but I need to study the code more closely.

I'd be happy to try and put together a PR for this.

I was making changes and realized this quickly. I made changes to inspect.isawaitable (temporarily moved it to asyncio.coroutines to debug changes I made in c asyncmodule), and updated Runner and create_task. I got a bit stuck when it came to making updates to the _asynciomodule.c, and clinic. I pushed what I had so far (minus the _asynciomodule.c changes).

PS: I didn't realize you were going to pick it up—so I'd be willing to close the PR. Regardless, you're more knowledgable in this domain, so I'd appreciate any feedback. @ronf

[ronf]

I got a chance to look at this more tonight. After looking at run_until_complete(), I took a somewhat different approach than #120306. I can turn this into a proper PR if you think this is a good way to go, but in the meantime here are the proposed changes (they're fairly small):

--- tasks.py.orig	2024-04-12 10:16:56
+++ tasks.py	2024-06-12 20:19:56
@@ -667,7 +667,7 @@
         h.cancel()


-def ensure_future(coro_or_future, *, loop=None):
+def ensure_future(coro_or_future, *, loop=None, context=None):
     """Wrap a coroutine or an awaitable in a future.

     If the argument is a Future, it is returned directly.
@@ -692,7 +692,7 @@
     if loop is None:
         loop = events.get_event_loop()
     try:
-        return loop.create_task(coro_or_future)
+        return loop.create_task(coro_or_future, context=context)
     except RuntimeError:
         if should_close:
             coro_or_future.close()

--- runners.py.orig	2024-04-12 10:16:56
+++ runners.py	2024-06-12 20:21:37
@@ -85,9 +85,15 @@

     def run(self, coro, *, context=None):
         """Run a coroutine inside the embedded event loop."""
-        if not coroutines.iscoroutine(coro):
-            raise ValueError("a coroutine was expected, got {!r}".format(coro))

+        if context is None:
+            context = self._context
+
+        if coroutines.iscoroutine(coro):
+            task = coro
+        else:
+            task = tasks.ensure_future(coro, context=context)
+
         if events._get_running_loop() is not None:
             # fail fast with short traceback
             raise RuntimeError(
@@ -95,10 +101,6 @@

         self._lazy_init()

-        if context is None:
-            context = self._context
-        task = self._loop.create_task(coro, context=context)
-
         if (threading.current_thread() is threading.main_thread()
             and signal.getsignal(signal.SIGINT) is signal.default_int_handler
         ):

The ValueError exception here is removed because that's already covered in ensure_future(), raising the following error if the argument is not supported:

TypeError: An asyncio.Future, a coroutine or an awaitable is required

I also added an argument to ensure_future() to allow the context to be set when it creates a task. If the caller passes in something which is already a Future, this context won't be set (since there is no task in this case), but I think that should be ok.

One thing to consider is whether the argument to run() should be changed from "coro" to "future" here to match what is used in run_until_complete(), but I avoided that for now to keep the diff more readable.

What do you think?

I tested this on all four of the object types I described above, and everything looks good so far.

[pygeek]

I prefer your changes. I'll close my PR.