client_factory as an optional parameter to create_connection

[Nicholas Chammas]

Does it make sense to make client_factory an optional parameter to create_connection()?

The “streams” API from the examples, which IMO is nicer to work with, passes in client_factory as None.

​

[Nicholas Chammas]

Put differently, should client_factory have a default value of None?

[Ron Frederick]

This is an interesting question.

I originally made it a required argument to follow the pattern established by the asyncio library’s calls to functions like BaseEventLoop.create_connection(). There, the protocol_factory argument is required. Later, I allowed a caller to pass in None as a signal to use the base SSHClient class as the factory, but about the only time that makes sense is when you are using the streams API. Otherwise, you wouldn’t end up implementing any callbacks, and so you’d never see the results of using the connection.

Another challenge here is that ‘host’ is a second positional argument after client_factory which is currently required, and so it’d be difficult to make client_factory have a default without either re-ordering it or making both arguments optional. In BaseEventLoop.create_connection(), host is optional, but only because there’s an option to pass in an already open socket instead. If both “host” and “sock” are left out, the create fails. AsyncSSH doesn’t currently allow you to pass in an already open socket, so for now it doesn’t make sense to allow “host” to be optional.

The “streams” API from the examples, which IMO is nicer to work with, passes in client_factory as None.=

-- 

Ron Frederick

ro...@timeheart.net

[Nicholas Chammas]

OK that makes sense. Thanks for explaining the background behind the decision.

[Ron Frederick]

Nicholas,

I was thinking about this question a bit more tonight and I realized what I said below wasn’t quite right. There is a problem with the order of the arguments and wanting to keep ‘host’ as a required parameter, but it does make sense for many simple clients to not need a customized SSHClient class. My comment about the streams API was actually more applicable to the ‘session_factory’ argument to SSHClientConnection.create_session(), but not as much to ‘client_factory’ here.

Given that, I wonder if it might make sense to offer a wrapper function called something like asyncssh.connect() which took the same arguments as create_connection() except for client_factory, passing in the default SSHClient class as the factory, and which returned just the SSHClientConnection instance and not the SSHClient which gets created. So, the simple_client example would look something like:

class MySSHClientSession(asyncssh.SSHClientSession):
    def data_received(self, data, datatype):
        print(data, end='')

    def connection_lost(self, exc):
        if exc:
            print('SSH session error: ' + str(exc), file=sys.stderr)

@asyncio.coroutine
def run_client():
    conn = yield from asyncssh.connect('localhost')
    chan, session = yield from conn.create_session(MySSHClientSession, 'ls abc')
    yield from chan.wait_closed()
    conn.close()

try:
    asyncio.get_event_loop().run_until_complete(run_client())
except (OSError, asyncssh.Error) as exc:
    sys.exit('SSH connection failed: ' + str(exc))

What do you think? It’s not a huge improvement, but it could make things a bit cleaner.

[Nicholas Chammas]

Totally agreed. It’s a small but nice improvement.

I can’t comment on how adding this connect() helper method would affect the overall coherence of the AsyncSSH API, but I can say that a) it fits my use case more naturally and b) it can be mapped more directly to Paramiko’s API (which is where I’m coming from).

So to add to the example you provided, I would reach straight for the streams API and do something like this:

@asyncio.coroutine
def run_client():
    conn, client = yield from asyncssh.connect(host='localhost')
    stdin, stdout, stderr = yield from conn.open_session('ls')
    output = yield from stdout.read().strip()
    print(output)
    exit_status = stdout.channel.get_exit_status()

In Paramiko, this would roughly look like:

def run_client():
    with paramiko.client.SSHClient() as client:
        client.connect(hostname='localhost')
        stdin, stdout, stderr = client.exec_command('ls')
        output = stdout.read().strip()
        print(output)
        exit_status = stdout.channel.recv_exit_status()

In both cases there is a minimum of code tangential to what the user is interested in. Whenever possible, sensible defaults are chosen so the user doesn’t have to think about additional details until they become relevant to their problem.

So thumbs up from me.

Nick

[Ron Frederick]

Thanks, Nick! This change is now available in both the master & develop branches on Github. Here’s the documentation for the new function:

connect

asyncssh.connect(host, port=22, **kwargs)[source]

Make an SSH client connection

This function is a coroutine wrapper around create_connection() which can be used when a custom SSHClient instance is not needed. It takes all the same arguments as create_connection() except for client_factory and returns only the SSHClientConnection object rather than a tuple of an SSHClientConnection and SSHClient.

When using this call, the following restrictions apply:

1. No callbacks are called when the connection is successfully opened, when it is closed, or when authentication completes.
2. Any authentication information must be provided as arguments to this call, as any authentication callbacks will deny other authentication attempts. Also, authentication banner information will be ignored.
3. Any debug messages sent by the server will be ignored.

Also, I expanded the first client example to actually show the full create_connection() pattern but updated all of the others to now use this simpler API. For instance, here’s the new math client example:

@asyncio.coroutine
def run_client():
    conn = yield from asyncssh.connect('localhost')

    stdin, stdout, stderr = yield from conn.open_session('bc')

    for op in ['2+2', '1*2*3*4', '2^32']:
        stdin.write(op + '\n')
        result = yield from stdout.readline()
        print(op, '=', result, end='')

    conn.close()

try:
    asyncio.get_event_loop().run_until_complete(run_client())
except (OSError, asyncssh.Error) as exc:
    sys.exit('SSH connection failed: ' + str(exc))

I’m not going to post a new PyPI release right away just for this one change, but this will get rolled into the next version I post there.

[Nicholas Chammas]

Looks good to me!

[Nicholas Chammas]

As I mentioned in the other thread, this new API might be well suited to Python’s concept of context managers.

For example, the example above could be rewritten slightly as:

@asyncio.coroutine
def run_client():
    with yield from asyncssh.connect('localhost') as conn:
        stdin, stdout, stderr = yield from conn.open_session('bc')

        

for op in ['2+2', '1*2*3*4', '2^32']:
            stdin.write(op + '\n')
            result = yield from stdout.readline()
            print(op, '=', result, end='')

I’m not sure if context managers play nicely with asyncio, but this approach would be nicely Pythonic and would offer all the usual benefits of context managers.

Nick

​

[Ron Frederick]

Hi Nick,

On Apr 19, 2015, at 12:22 PM, Nicholas Chammas <nicholas...@gmail.com> wrote:

As I mentioned in the other thread, this new API might be well suited to Python’s concept of context managers.

For example, the example above could be rewritten slightly as:

@asyncio.coroutine
def run_client():
    with yield from asyncssh.connect('localhost') as conn:
        stdin, stdout, stderr = yield from conn.open_session('bc')

        for op in ['2+2', '1*2*3*4', '2^32']:
            stdin.write(op + '\n')
            result = yield from stdout.readline()
            print(op, '=', result, end='')

I’m not sure if context managers play nicely with asyncio, but this approach would be nicely Pythonic and would offer all the usual benefits of context managers.

It looks like this is possible, but the syntax isn’t quite as clean as what you propose here. There are a couple of different options. You can either do:

@asyncio.coroutine
def run_client():
    conn = yield from asyncssh.connect('localhost'

)
    with conn:
        stdin, stdout, stderr = yield from conn.open_session('bc')

        

for op in ['2+2', '1*2*3*4', '2^32']:
            stdin.write(op + '\n')
            result = yield from stdout.readline()
            print(op, '=', result, end='')

or:

@asyncio.coroutine
def run_client():
    with (yield from asyncssh.connect('localhost’)) as conn:
        stdin, stdout, stderr = yield from conn.open_session('bc')

        

for op in ['2+2', '1*2*3*4', '2^32']:
            stdin.write(op + '\n')
            result = yield from stdout.readline()
            print(op, '=', result, end='')

Basically, you can’t use the “yield from” syntax as part of another statement without the extra parentheses except in the special case of a simple assignment.

The good news here is that it is trivial to add. The change to SSHConnection is basically just:

@@ -167,6 +167,16 @@ class SSHConnection(SSHPacketHandler):

         self._cmp_algs = _select_algs('compression', compression_algs,

                                       get_compression_algs(), b'none')

 

+    def __enter__(self):

+        """Allow SSHConnection to be used as a context manager"""

+

+        return self

+

+    def __exit__(self, *exc_info):

+        """Automatically close the connection when used as a context manager"""

+

+        self.close()

+

     def _cleanup(self, exc=None):

         if self._channels:

             for chan in list(self._channels.values()):

Also, by splitting the assignment and “with” into separate statements you could potentially take advantage of the context manager even when using the create_connection() version of the API:

@asyncio.coroutine
def run_client():
    conn, client = yield from asyncssh.create_connection(MySSHClient, 'localhost’)
    with conn:

        chan, session = yield from conn.create_session(MySSHClientSession, 'ls abc')
        yield from chan.wait_closed()

-- 

Ron Frederick

ro...@timeheart.net

[Nicholas Chammas]

Ah, the syntax makes sense.

And that’s neat—having the context manager available on SSHConnection lets it be used with both create_connection() and the upcoming connect(). Nice!

I didn’t even know you could call with obj: like that.

Nick

​

[Ron Frederick]

Yeah - I think the “with <obj>:” version of the syntax is common for something like grabbing & releasing a lock, where you might create the lock once but then want to grab and release it multiple times after that. That sort of usage wouldn’t be applicable here since close() can only be called once, but the syntax of separating the creation from the instantiation of the context can still be applied.