Add async support for Connect() function?

Connecting to an SSH server is quite time consuming with all the key exchanges etc. I did not find an async version of that function, so was wondering if it was possible to add an async version of that (old school async is fine, as we can then wrap it for task based async). Or is there some reason that function is not async?

For the moment when I need to open multiple connections I do them in separate task threads, so I can run them in parallel.

Async support was just added for SFTP, SSH part didn't get the love yet. :)
There is no particular technical reason, it just hasn't been done yet.

Ok good to know

Just had another look and ConnectAsync(CancellationToken cancellationToken) is implemented on the BaseClient from which SshClient is derived.
So if you only need this, it is already available on the development branch. No NuGet package yet, so you'll have to build SSH.NET yourself.

That is good to know. I can wait for the next NuGet build though, as I already solved our problem simply by running the connection code in a separate task. Once this is public, I can remove that functionality and use the async versions.

Hopefully we can get task based async stuff added at the same time so we can avoid using a third party library like this one:

https://github.com/JohnTheGr8/Renci.SshNet.Async

Maybe I should take a look at the latest code and do a PR to integrate it.

That would be great. Please have a look at how it is implemented in SftpClient. The above mentioned extension library uses TAP over APM approach. Unfortunately the current APM implementation in SSH.NET uses ThreadPool to queue "async" requests, which creates even more pressure that running caller code on ThreadPool. Ideally in the future we could just remove the APM or replace it with APM over TAP.

I expect that at least these should be added at this time:

SshCommand.ExecuteAsync()
SshClient.RunCommandAsync()
Shell.StartAsync()
Shell.StopAsync()

Do you see anything else?

ShellStream is problematic in "many, many wonderfully ways", please see #926. I think it should be refactored before adding any async functionality.

That looks like about it for me. I don't use ShellStream (need to research what that is - could be useful!) but yes, checking that problem reports for that I suggest we leave that alone for the moment.

What is the process of getting all the tests to run. Do I need to set up a test SSH server someone and setup the credentials to access it?

That would be great. Please have a look at how it is implemented in SftpClient. The above mentioned extension library uses TAP over APM approach. Unfortunately the current APM implementation in SSH.NET uses ThreadPool to queue "async" requests, which creates even more pressure that running caller code on ThreadPool. Ideally in the future we could just remove the APM or replace it with APM over TAP.

Ok, so implementing SshCommand.ExecuteAsync() using TAP is not going to be easy without either wrapping it as TAP over APM, or as complete re-write of the internals to be TAP and then implementing the APM on top of the TAP version. I am not experienced enough in the internals of the SSH channel operation to being to figure out how to re-write that to use TAP since it revolves around waiting on a wait handle, and then having event handlers that execute when bytes come over the wire and stuffing them into the appropriate streams.

For now I think the simple solution is to implement this as TAP over APM, since at least then the API surface area exists and will work, and longer term someone more familiar than me with the internals to re-write it to be TAP internally, and then implement the legacy async support with APM on top of that.

Question: for SshCommand.Result and SshCommand.Error, it reads the values there from OutputStream and ExtendedOutputStream, which are both instances of PipeStream. Clearly there are ReadToEndAsync() functions we can use for reading the streams, and I have implemented that. However I am not sure if that is even useful? Will reading from those streams actually block at all? I think it can, but I am not entirely clearly on that. If it can, then we do want GetResultAsync() and GetErrorAsync() functions like I have implemented.

Ok, work in progress is here. I need to figure out how to run the unit tests so I can run the ones I have built, and also add more for the rest of the functions. You can see the change here:

kendallb@4034b14

As mentioned above, most of the functionality is not easy to convert to TAP so I am using the TAP over APM approach of this library for those functions I could not natively convert:

https://github.com/JohnTheGr8/Renci.SshNet.Async

The core of it is stored in this new file:

https://github.com/kendallb/SSH.NET/blob/feature/ssh-tap-async/src/Renci.SshNet/AsyncExtensions.cs

Still needs to be tested, but it adds support for cancellation tokens as well as doing TAP over APM. I am relying on the ability to call into the AsyncResult.SetComplete() function if the cancellation token fires to terminate the thread running the APM async code, but I think that will work. Have not tested it yet as I need to run the tests :)

Also Looking at Shell.Start(), I am not entirely sure if there is a whole lot of value in converting that to TAP Async? It does not expose the normal APM Begin/End functions I think since it really starts a background thread to process reading from the stream from the shell until the shell terminates. I am not sure that even makes sense to convert to TAP? Converting the thread internally to TAP is a bit pointless also, since it's still going to be a separate thread, so easily to leave it as is I think?

Let me know what you think.

I'm sorry, but I think this approach is not getting the users of SSH.NET any value.

Both TAP and APM are an attempt to conserve threads. Current implementation of most APM APIs in SSH.NET consume the very resource they are supposed to conserve: they schedule all the work on the thread pool. One such example is SftpClient.BeginSynchronizeDirectories. If user code would call BeginSynchronizeDirectories and then wait for the AsyncResult, it would actually consume TWO threads (user code's thread and thread pool thread) instead of ONE, making the situation even worse.

In my opinion, the problem with Renci.SshNet.Async extension library is not that it is a separate library. That is one of the reasons extensions were invented in the first place. The problem is that it uses improperly implemented APM APIs to (by extension improperly) implement TAP API. I really see no point in bringing Renci.SshNet.Async code into SSH.NET.

One more consideration: SshCommand.BeginExecute is actually synchronous in terms of local processing: it synchronously creates a channel, synchronously opens it and synchronously sends the request. Only after that it becomes asynchronous in terms of waiting for the remote response.

As a user of this library I would rather have a smaller API surface that is properly implemented that having a large API surface with half-bad implementation.

Yes, I agree. I don't think it is the right approach after having spent time on it. The correct approach is a proper TAP approach, but I do not know enough at the moment to be able to start such an implementation. ConnectAsync() is already implemented so when that gets published I can switch over to that in our code. The rest of it though such as ExecuteAsync(), it's not clear to me yet how a proper TAP implementation would work. I would be happy to spend a bit more time on it with some guidance on how that might be implemented.

The biggest issue I see is how to implement the ability to consume the two streams coming back from the command in an async fashion when using TAP? Same goes for the Shell support. For instance below is the current function I use to run commands over SSH using TAP wrapping the APM calls:

    // Internal function to read from the stream and report on the results
    async Task CheckForProgress(
        IAsyncResult asyncResult,
        StreamReader streamReader,
        StringBuilder output,
        char[] charBuffer)
    {
        // Keep going until the command is either completed, or we still have stuff to read
        while (!asyncResult.IsCompleted || !streamReader.EndOfStream) {
            var bytesRead = await streamReader.ReadAsync(charBuffer, 0, 1).ConfigureAwait(false);
            if (bytesRead > 0) {
                var ch = charBuffer[0];
                progress.Invoke(ch);
                output.Append(ch);
            } else {
                // Pause for a bit to give up some time while we wait for the next character
                await Task.Delay(10).ConfigureAwait(false);
            }
        }
    }

    using var sshCommand = node.SshClient.CreateCommand($"{command} {arguments}");
    var asyncResult = sshCommand.BeginExecute();
    using var streamReader = new StreamReader(sshCommand.OutputStream);
    var result = new StringBuilder();
    var charBuffer = new char[1];
    await CheckForProgress(asyncResult, streamReader, result, charBuffer);
    sshCommand.EndExecute(asyncResult);
    return new SshResult {
        Result = result.ToString(),
        Error = sshCommand.Error,
        ExitStatus = sshCommand.ExitStatus,
    };

So we call BeginExecute() to start the call, then set up async reading of the progress streams and continually check if there are results in the output stream or if the ascncResult is completed, delaying for a bit each time. In this case we specifically need to get one character at time back from the SSH command (as they are long running commands and we can't wait for newlines to report on progress). We don't care about the error stream while it's running, so we only need to grab that if something went wrong but theoretically you could read that as well and output it's progress. It works great.

BUT, I am not clear how this would be structured if we had a TAP based ExecuteAsync? If you call ExecuteAsync() and the result is the standard output as a string, it won't return until the command has finished and then return the result. Which is probably find if you just want to execute a command and block/wait on the result. But how you would implement similar functionality to actually read the standard output and standard error streams? We would need some kind of TAP async support for reading the contents of the two streams but I am not clear on how we would do that.

@darkoperator

It should be as simple as:

    using var sshCommand = node.SshClient.CreateCommand($"{command} {arguments}");
    await sshCommand.ExecuteAsync(cancellationToken).ConfigureAwait(false);
    
    using var outputReader = new StreamReader(sshCommand.OutputStream);
    using var errorReader = new StreamReader(sshCommand.ExtendedOutputStream);
    return new SshResult {
        Result = await outputReader.ReadToEndAsync(cancellationToken).ConfigureAwait(false),
        Error = await errorReader.ReadToEndAsync(cancellationToken).ConfigureAwait(false),
        ExitStatus = sshCommand.ExitStatus,
    };

@darkoperator, you have more experience with SSH, does this look good to you?

There are a couple of obstacles to be able to use the code above:

PipeStream returns 0 instead of blocking when running a command with intermittent output #12 makes StreamReader.ReadToEnd terminate too early. Fix PipeStream.Read returning 0 prematurely #924 (not been merged yet) should fix this.
We need to implement SshCommand.ExecuteAsync()
The StreamReader.ReadToEndAsync() is using async/sync until we implement PipeStream.ReadAsync(byte[], int, int, CancellationToken)

Ad 1: This is hopefully taken care of.
Ad 2: @kendallb, for SshCommand.ExecuteAsync specifically, TAP over APM would work OK. Can you implement it using TaskFactory<string>.FromAsync()?
Ad 3: PipeStream is quite inefficient performance-wise (we have #374 pending), so let's cross that bridge when we get to it...

Just noted that the above sample would not work, as EndExecute consumes the output stream, thereby making this API very fragile. So the proper sample would be:

    using var sshCommand = node.SshClient.CreateCommand($"{command} {arguments}");
    var output = await sshCommand.ExecuteAsync(cancellationToken).ConfigureAwait(false);
    
    using var errorReader = new StreamReader(sshCommand.ExtendedOutputStream);
    return new SshResult {
        Result = output,
        Error = await errorReader.ReadToEndAsync(cancellationToken).ConfigureAwait(false),
        ExitStatus = sshCommand.ExitStatus,
    };

@IgorMilavec

@IgorMilavec right clearly 1) is the reason I had to implement my async code the way I did, as I did find it was terminating too early if there was nothing to ready. So if PipeStream is fixed so it would block until either the connection is closed or there is data, it would make it support ReadToEndAsync() and block until the command is finished.

However that still would not solve the problem I have, in that I need to interactively report on each charactet as it comes over the wire (in this case we are simply rendering to the console since we need to see what the remote command is actually doing as it runs). But I suspect if PipeStream is fixed, then I can still read one byte at a time with ReadAsync(charBuffer, 0, 1) and would not have to worry about it returning an early EOF like it does now. So that makes sense.

For 2), yes, I already have a version of TAP over APM in my code tree that does that, using TaskFactory.FromAsync(). So that should work fine for the interim.

@IgorMilavec

@IgorMilavec Maybe we need a version of EndExecute that does not consume the output stream for TAP, and require the user to get the resulting output or error streams using the new Async functions I wrote:

https://github.com/kendallb/SSH.NET/blob/4034b1455a65b60b4f12b78e06116ef64558ccac/src/Renci.SshNet/SshCommand.cs#L99

but that still requires PipeStream to be fixed as I see now my code as it is today would not work.

@drieseng

I need to interactively report on each charactet

Sure, you can do the read loop yourself in this case.

I already have a version of TAP over APM in my code tree that does that

I think you're doing "too much too early" in kendallb@4034b14. Can you just do the ExecuteAsync() first, so we can discuss it?

Maybe we need a version of EndExecute that does not consume the output stream for TAP

I too would rather see ExecuteAsync return Task (with exit code) and leave the output and error streams intact for the caller to use. Right now we expose OutputStream, Result and return the value from Execute/EndExecute. Result consumes the unconsumed part of the OutputStream, so I guess the caller should accumulate it somehow? For me the clean interface would be:

Expose OutputStream
Remove Result (mark it obsolete first, ...)
Implement SshCommand.ExecuteAsync() to return status code and leave the OutputStream intact.

@drieseng what is your position regarding this?

I think you're doing "too much too early" in kendallb@4034b14. Can you just do the ExecuteAsync() first, so we can discuss it?

Yes, I will try to get that done shortly and leave out all the other bits.

Maybe we need a version of EndExecute that does not consume the output stream for TAP

I too would rather see ExecuteAsync return Task (with exit code) and leave the output and error streams intact for the caller to use. Right now we expose OutputStream, Result and return the value from Execute/EndExecute. Result consumes the unconsumed part of the OutputStream, so I guess the caller should accumulate it somehow? For me the clean interface would be:

Expose OutputStream

Remove Result (mark it obsolete first, ...)

Implement SshCommand.ExecuteAsync() to return status code and leave the OutputStream intact.

I don't think we need to get rid of the Result property, as that is quite useful in the situation where someone wants to run the command Async and then get back the results from it? For an Async call you can make the Execute call which returns no result, and then use GetResultAsync() to read the result from the stream in an async fashion, or directly process the stream yourself like I do to get a character at a time in real time from it. Or call .Result() which would read the stream in a blocking fashion?

Although I suppose you could argue that perhaps .Result can be removed and the sync version returns the result as a string, but if you run something async you would need to process the result stream yourself?

@IgorMilavec

I'm also inclined to follow @IgorMilavec's proposal (except expose an InputStream; we're already exposing OutputStream and ExtendedOutputStream).

I'd indeed remove (mark obsolete first) the Result and Error properties.

I didn't read all the comments, so I may not have the complete picture.

Well the counter argument to removing at least the Error property is that for someone that just wants to run a simple command and get the results back as strings, the error properly would be useful? We can have the non async RunCommand run the same as before and return the output as a string but it would be a lot of extra code all time to process the extended output steam to get the error results.

I understand the urge for simplicity. However, having two different access method for the same content is bound to surprise some users. It is not clear from the interface that accessing the Error property will change the state of the object returned by ExtendedOutputStream property (by consuming it). By design guidelines, accessing a property should not change the state of the object. Also, the semantics of such a property are not clear: should it return the content buffered until now (as it actually does) or should it block until the end of ExtendedOutputStream? Arguably, the latter would be correct for me, but my point is that we avoid a lot of uncertainty by just removing this property. A Stream on the other hand has clear and well understood interface.
Writing new StreamReader(shell.ExtendedOutputStream).ReadToEnd() in user code is clear about semantics and side effects and in my opinion not that much code to write.

Yep, all good points. So I would agree.

Ok I think this implements what we discussed. Although now there are a ton of warnings due to the obsolete Result and Error properties :). So we really need to decide if that becomes obsolete and we fix all the tests, or leave it as is.

#937

I have not fully tested this code as I am not entirely sure on how to run the unit tests? How do you guys run the tests? Do you set up a local SSH server and tweak the settings in the config files to point to it? I can probably either set up WinSSHD on my local box (not sure there is a free version though as we pay for it at work), or probably just use the SSH setup in WSL (I use that for testing some other stuff we do for Percona).

Ok I worked out how to get some tests running with my WSL install, and I fixed the issues with task cancellation and added a test. So the code works nicely now.

Navigation Menu

Repository navigation

Add async support for Connect() function? #931

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add async support for Connect() function? #931

Description

Activity

IgorMilavec commented on Mar 11, 2022

kendallb commented on Mar 11, 2022

IgorMilavec commented on Mar 12, 2022

kendallb commented on Mar 12, 2022

kendallb commented on Mar 12, 2022

IgorMilavec commented on Mar 12, 2022

kendallb commented on Mar 12, 2022

kendallb commented on Mar 12, 2022

kendallb commented on Mar 12, 2022

kendallb commented on Mar 13, 2022

IgorMilavec commented on Mar 13, 2022

kendallb commented on Mar 13, 2022

IgorMilavec commented on Mar 13, 2022

IgorMilavec commented on Mar 13, 2022

kendallb commented on Mar 13, 2022

kendallb commented on Mar 13, 2022

IgorMilavec commented on Mar 13, 2022

kendallb commented on Mar 16, 2022

drieseng commented on Mar 16, 2022

kendallb commented on Mar 16, 2022

IgorMilavec commented on Mar 16, 2022

kendallb commented on Mar 16, 2022

kendallb commented on Mar 19, 2022

kendallb commented on Mar 20, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions