Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for a Task based async API #153

Closed
TrevorPilley opened this issue Jan 17, 2017 · 15 comments · Fixed by #819
Closed

Add support for a Task based async API #153

TrevorPilley opened this issue Jan 17, 2017 · 15 comments · Fixed by #819

Comments

@TrevorPilley
Copy link

It'll make the library much easier to use with async/await and fit the modern .NET async friendly way of working.

I'd suggest at minimum the following methods:

  • public Task<Stream> DownloadFileAsync(string path)
  • public Task<IEnumerable<SftpFile>> ListDirectoryAsync(string path)
  • public Task UploadFileAsync(string path, Stream stream, bool overwrite = false)

I've managed to wrap the old IAsyncResult style API you already have for DownloadFile as follows:

await Task.Factory.FromAsync(this.sftpClient.BeginDownloadFile(remoteUri, stream), this.sftpClient.EndDownloadFile) but I'm struggling to figure out the way to do the same for ListDirectory...

@omidkrad
Copy link

omidkrad commented Mar 30, 2017

I think if SshCommand.EndExecute returned the command, instead of string, as in:

public SshCommand EndExecute(IAsyncResult asyncResult)

then we could do:

public static Task<SshCommand> RunCommandAsync(this SshClient sshClient, string command)
{
    if (sshClient == null)
        throw new ArgumentNullException(nameof(sshClient));

    var sshCmd = sshClient.CreateCommand(command);
    return Task<SshCommand>.Factory.FromAsync(sshCmd.BeginExecute, sshCmd.EndExecute, null);
}

and then await on it.

@omidkrad
Copy link

omidkrad commented Mar 30, 2017

In the extension method I posted above, if you replace Task<SshCommand> with Task<string> it will work, however, since return type isn't SshCommand we can't examine if the command was a success or failure.

My workaround right now is the following:

public static async Task<SshCommand> RunCommandAsync(this SshClient sshClient, string commandText)
{
    if (sshClient == null)
        throw new ArgumentNullException(nameof(sshClient));

    return await Task.Run(() => sshClient.RunCommand(commandText)).ConfigureAwait(false);
}

Bartizan added a commit to Bartizan/SSH.NET that referenced this issue Sep 15, 2017
…existing async API for SftpClient. Added into a new project targeting .net 4.5.
@Bartizan
Copy link

Bartizan commented Sep 15, 2017

Maybe the library will get the TAP support. If it is not, there is also a nuget package Rehci.SshNet.Async.

@JohnTheGr8
Copy link

I have updated the aforementioned Rehci.SshNet.Async package to also target .NET Standard 1.3 and UAP.

Hopefully it is useful to some of you...

@niemyjski
Copy link

It would be really great to only support async going forward for all operations like getting meta data, listing directories, uploading, downloading and seeing if a file or folder exists.

Bartizan added a commit to Bartizan/SSH.NET that referenced this issue Apr 22, 2020
…existing async API for SftpClient. (Not supported by .net 3.5)
@wplong11
Copy link

wplong11 commented May 4, 2020

The Connect method must also support asynchronous APIs.

@vanillajonathan
Copy link

The proposed ListDirectoryAsync method could make use of IAsyncEnumerable which is new in C# 8.0, but it requires .NET Standard 2.1 or later.

@andriysavin
Copy link

The proposed ListDirectoryAsync method could make use of IAsyncEnumerable which is new in C# 8.0, but it requires .NET Standard 2.1 or later.

The need of targeting .NET Standard 2.1 is not the biggest impediment for these APIs (cross-compiling could be used for that). The whole library seems to be written without async in mind and requires significant refactoring. All current async methods and wrappers are based on the old Begin/End async pattern, implementation of which turns out to be not really asynchronous and just calls synchronous methods on thread pool threads. This is a well-known antipattern and leads to many problems.

@IgorMilavec
Copy link
Collaborator

@andriysavin, I agree regarding SftpClient.
Hovever, SftpSession's BeginX/EndX implementation looks OK to me on first sight.

@IgorMilavec
Copy link
Collaborator

IgorMilavec commented Mar 11, 2021

@drieseng I have committed my attempt at implementing DownloadFileAsync in IgorMilavec/SSH.NET@f510968. Can you please have a look and let me know if this is something you would consider merging into SSH.NET, before I push on with other functions as suggested in this issue?
The implementation is async all the way down, no threads were harmed during implementation of this feature. :)

@kdcllc
Copy link

kdcllc commented May 10, 2021

@drieseng, we are running into the issues with iasyncresult and can use this feature. Is the PR @IgorMilavec completed?

@IgorMilavec
Copy link
Collaborator

I just added ConnectAsync to the PR. I believe the functionality should now be functionally complete for most uses.

@IgorMilavec
Copy link
Collaborator

@drieseng can you please have a look at #819? Is it possible to merge this PR and build a beta package? I'm running this code in production on a couple of servers for a month now and did not see any regressions. By building a beta package we would make this more accessible to interested parties and get feedback on code quality and supported usage cases.

@IgorMilavec
Copy link
Collaborator

@drieseng pinging you again in case you missed the above proposal

@drieseng
Copy link
Member

@IgorMilavec Sorry man, I've been very busy the last months (or is it years). I'll try to find more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet