asyncssh icon indicating copy to clipboard operation
asyncssh copied to clipboard

asyncio.run(asyncssh.connect()) raises ValueError: a coroutine was expected, got <asyncssh.misc._ACMWrapper object ..>

Open fillest opened this issue 1 year ago • 4 comments

I have found out that asyncio.get_event_loop() is deprecated, upgraded to Python 3.12 (asyncssh==2.14.2) and switched to the asyncio.Runner. asyncio.run roughly just calls asyncio.Runner.

When I run e.g. asyncio.run(asyncssh.connect(host)), it raises:

    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))
E           ValueError: a coroutine was expected, got <asyncssh.misc._ACMWrapper object at 0x...>

We can see in the code that it basically checks if coro is an instance of (types.CoroutineType, collections.abc.Coroutine). Maybe you can inherit _ACMWrapper from it?

fillest avatar Jun 08 '24 13:06 fillest

I'm not sure it makes sense to do what you're trying to do here. If you use asyncio.run() on a call to asyncssh.connect(), you wouldn't be able to use the SSHClientConnection object you get back, because the event loop created to handle asyncio.run() would have ended, and you can't use any AsyncSSH objects after the event loop that created them has been closed.

Whatever you event loop used to call asyncssh.connect() needs to stay running for as long as you want to use the AsyncSSH objects it creates.

The closest I can see getting to this would be something like:

loop = asyncio.new_event_loop()
conn = loop.run_until_complete(asyncssh.connect('localhost'))
result = loop.run_until_complete(conn.run('echo hello'))
print(result.stdout, end='')
loop.close()

This still works fine in Python 3.12, and keeps the loop active after getting back "conn" so that you can use it in other calls scheduled on that same event loop. However, I think this would be written more cleanly as:

async def run_client():
    async with asyncssh.connect('localhost') as conn:
        result = await conn.run('echo hello')
        print(result.stdout, end='')

asyncio.run(run_client())

ronf avatar Jun 08 '24 15:06 ronf

the event loop created to handle asyncio.run() would have ended

Yes, I actually wanted to use asyncio.Runner (not asyncio.run()) - I mentioned asyncio.run() just to simplify the example (sorry for making it more confusing :D).

I can't use full async because I need the higher-level code to be synchronous - I hide and run the async calls only inside the wrappers.

I thought to try to avoid using "low-level" new_event_loop() + run_until_complete() directly because the asyncio.Runner performs some extra logic that I probably will have to reproduce anyway, e.g. signal handling. There is some more logic there that I'm not sure I understand clearly, but it probably is useful and handles some corner cases.

So I came up with an idea that _ACMWrapper could just inherit also from collections.abc.Coroutine, but I'm not sure, as I'm not familiar with the types being used there.. Just an idea :)

fillest avatar Jun 08 '24 21:06 fillest

the event loop created to handle asyncio.run() would have ended Yes, I actually wanted to use asyncio.Runner (not asyncio.run()) - I mentioned asyncio.run() just to simplify the example (sorry for making it more confusing :D).

I see. I hadn’t run across that class before, but I see it appears to be doing something similar to the run_until_complete example I gave, where it can keep the event loop open.

I’m actually somewhat surprised that it is making an explicit check like this here, rather than relying on duck-typing. If it did want to make an explicit check, I would have expected it to align with loop.run_until_complete() in what it accepts, which is basically an asyncio.Future.

I’ve just filed issue https://github.com/python/cpython/issues/120284 about this on GitHub, to see whether this is something which can be changed (though that’ll still leave some versions out there which wouldn’t support it). I’m curious what the asyncio devs think of this, though.

I can't use full async because I need the higher-level code to be synchronous - I hide and run the async calls only inside the wrappers.

I thought to try to avoid using "low-level" new_event_loop() + run_until_complete() directly because the asyncio.Runner performs some extra logic that I probably will have to reproduce anyway, e.g. signal handling. There is some more logic there that I'm not sure I understand clearly, but it probably is useful and handles some corner cases.

So I came up with an idea that _ACMWrapper could just inherit also from collections.abc.Coroutine, but I'm not sure, as I'm not familiar with the types being used there.. Just an idea :)

It looks like collections.abc.Coroutine would requite an object to not only be awaitable but also to implement send(), throw(), and close(), which I wouldn’t want to do here. There is a collections.abc.Awaitable which is more appropriate, but that probably wouldn’t help make asyncio.Runner happy. As for using types.coroutine, that is a decorator, not a type, and it requires that its argument is a Python generator. There is types.CoroutineType, but I tried it just see what would happen and it looks like that can't be used as a base type:

    class _ACMWrapper(Generic[_ACM], types.CoroutineType):
TypeError: type 'coroutine' is not an acceptable base type

ronf avatar Jun 09 '24 01:06 ronf

Yes, thank you for reporting upstream, I agree that the logic of the type check inside asyncio.Runner looks problematic actually.

fillest avatar Jun 09 '24 08:06 fillest

The change to broaden the types for asyncio.Runner.run() (and asyncio.run()) is now checked into cpython (see commit 1229cb8).

ronf avatar Sep 28 '24 00:09 ronf

Thank you. I haven't tested it yet (will wait for the next full Python release) but it looks working, so I'm closing the issue.

fillest avatar Sep 30 '24 18:09 fillest