ronf / asyncssh

AsyncSSH is a Python package which provides an asynchronous client and server implementation of the SSHv2 protocol on top of the Python asyncio framework.
Eclipse Public License 2.0
1.53k stars 150 forks source link

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

Open fillest opened 2 months ago

fillest commented 2 months ago

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?

ronf commented 2 months ago

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())
fillest commented 2 months ago

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 :)

ronf commented 2 months ago

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
fillest commented 2 months ago

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