Add async support for Connect() function?

[kendallb]

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.

[IgorMilavec]

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.

[kendallb]

Ok good to know

[IgorMilavec]

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.

[kendallb]

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.

[kendallb]

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.

[IgorMilavec]

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.

[kendallb]

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.

[kendallb]

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?

[kendallb]

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.

[kendallb]

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.

[IgorMilavec]

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.

[kendallb]

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.

[IgorMilavec]

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:

1. 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.
2. We need to implement SshCommand.ExecuteAsync()
3. 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...

[IgorMilavec]

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,
    };