Add async support to SftpClient and SftpFileStream

[IgorMilavec]

Core async support requested by #153.

Added async support to BaseClient: (update 2021-05-11)

• ConnectAsync

Added async support to SftpClient:

• ListDirectoryAsync
• GetStatusAsync
• OpenAsync
• DeleteFileAsync
• RenameFileAsync
• DownloadFileAsync
• UploadFileAsync

Added async support to SftpFileStream:

• OpenAsync
• ReadAsync
• WriteAsync
• FlushAsync

Added net46 target to use async in .NET Framework.

This PR is intended to resolve #153.
This PR is intended to supersede #300 and #661.

[vanillajonathan]

Should the generic class Exception be caught here? Or something more specific?

[IgorMilavec]

Possibly, but I only reused the logic of sync versions of the functions at this time. When/if this is released, practice will show if any changes need to be made.

[drieseng]

When FileMode is Create, I think we should just use Flags.CreateNewOrOpen | Flags.Truncate.
Let me run some checks later.

[drieseng]

I can confirm that it's ok to just add Flags.CreateNewOrOpen | Flags.Truncate when mode is FileMode.Create.

[vanillajonathan]

Specify second parameter to ArgumentException, nameof(buffer).

[IgorMilavec]

I'm not sure about that, any of the three could be wrong, depending on the caller's intention. Anyhow, this is the same as the sync version, so we either need to change both or none...

[vanillajonathan]

Consider returning ICollection or IReadOnlyCollection rather than IEnumerable as per the Microsoft Guidelines for Collections.

DO use Collection or a subclass of Collection for properties or return values representing read/write collections.

[drieseng]

This should be IAsyncEnumerable<SftpFile>. It's only supported on .NET 4.6.1 or higher.
For .NET 4.6.1 and .NET Standard 2.0, you need to add a reference to Microsoft.Bcl.AsyncInterfaces.

[IgorMilavec]

Then we need to remove FEATURE_TAP from netstandard1.3. Also, we take two "external" dependencies (AsyncInterfaces and Task.Extensions).
What if we leave this one as it is and add IAsyncEnumerable<SftpFile> EnumerateDirectoryAsync(...) for netstandard2.1+ ?

[drieseng]

I won't let this block this merge request, but I still strongly consider changing this method to return IAsyncEnumerable<SftpFile>. I also don't like bringing in additional dependencies, but the alternative is not better.

[schaveyt]

Any chance this could also target .net5 or .net6 as well

[IgorMilavec]

The library already targets netstandard2.0, so you can reference it from net5 or net6 projects.
It would only be beneficial for the library to target net5+ if it would use any net5+ features, like Spans etc. But this is out of scope of this issue and this PR. IMO we should remove legacy targets (please see #665) before adding any modern code.

[schaveyt]

I did not connect those dots. Thank you for clarifying 👍

[paulmaybee]

This addition looks like it has a great deal of value. Is there a timetable or estimate for when the PR will be completed?

[IgorMilavec]

This PR if functionally complete IMO. It should also be stable, I have been running it in production since July. Now we need @drieseng to review and merge it.

[drieseng]

@IgorMilavec Thanks, I'll try to find time in the coming days.

[IgorMilavec]

Figured this is the last chance to modify UploadFileAsync signature before this becomes a breaking change... :)

[drieseng]

@IgorMilavec I finally had time to do a high-level review of this PR. Overall it's looking good, but of course (unsurprisingly) still incomplete. Thanks for taking this first (but important) step!!

I hope you're not discouraged by the number of comments.
A lot of them are due to (unintended?) formatting changes. These make the PR harder to review, pollute history and are not inline with the code style that we use.

I propose we discuss the comments in the PR, but - if useful/necessary - we can organize a (voice?) chat.

[IgorMilavec]

No worries, @drieseng, thanks for the review.
Let me first clear trivial items off the table and then we'll discuss further, OK? I think voice chat would be great at that point.

[IgorMilavec]

I believe I have addressed all the trivial items. @drieseng can you please re-review the code and my comments?

[drieseng]

@IgorMilavec, is it ok for you to target .NET Framework 4.7.2 (or 4.8) instead?

[drieseng]

I can confirm that it's ok to just add Flags.CreateNewOrOpen | Flags.Truncate when mode is FileMode.Create.

[drieseng]

@IgorMilavec Thanks for hanging on. I'll do my best to finish the review some time this week.

[drieseng]

We're still lacking (unit & integration) test coverage, but I'm ok with adding these as a follow PR.
I'd also like to discuss further how to proceed with ISftpClient.ListDirectoryAsync(...). In its current state, this is not a true async method.

That said: Thx !!!!!!!!

[Dimencia]

Working on a large project at work and just ran into this being a problem, that's awesome that hours before I (realized I) needed it, there was already a PR merged

I'll be pulling this down and trying it tomorrow, nice work, and thanks

[IgorMilavec]

Thanks @drieseng !
I'll start working on unit tests. After these are complete I can tackle the message pump and make it Task based as we already discussed.
Regarding ISftpClient.ListDirectoryAsync(...): the current version is already async, but it is not "streaming". For directories with small amount of files (up to the amount that SftpReadDirRequest returns) there is no difference. For a directory with large amount of files, the current implementation only completes (asynchronously) when all the file infos are read. The IAsyncEnumerable implementation would allow the caller to process file names in chunks (of the amount returned by one SftpReadDirRequest) even before all the file infos are read back. Let me implement another function with IAsyncEnumerable and then we can discuss further, OK?

[drieseng]

@IgorMilavec Thanks for taking this up. I'm ok with having a second method for ISftpClient.ListDirectoryAsync(...) as POC.

[noahmehl]

When we review this PR, it looks like these methods are not actually implemented:

• DownloadFileAsync
• UploadFileAsync

Are we just mis-understanding the PR itself?

[IgorMilavec]

These functions were initially implemented as part of this PR, but during the code review we decided that these functions are not really necessary, as they can be easily implemented in the consuming code with a couple of lines of code.

If you have no special requirements, you can use this extension class to get these two functions:

namespace Renci.SshNet
{
    public static class SftpClientExtensions
    {
        public static async Task DownloadFileAsync(this SftpClient sftpClient, string path, Stream output, CancellationToken cancellationToken)
        {
            using (Stream remoteStream = await sftpClient.OpenAsync(path, FileMode.Open, FileAccess.Read, cancellationToken).ConfigureAwait(false))
            {
                await remoteStream.CopyToAsync(output, 81920, cancellationToken).ConfigureAwait(false);
            }
        }

        public static async Task UploadFileAsync(this SftpClient sftpClient, Stream input, string path, FileMode createMode, CancellationToken cancellationToken)
        {
            using (Stream remoteStream = await sftpClient.OpenAsync(path, createMode, FileAccess.Write, cancellationToken).ConfigureAwait(false))
            {
                await input.CopyToAsync(remoteStream, 81920, cancellationToken).ConfigureAwait(false);
            }
        }
    }
}

[noahmehl]

@IgorMilavec Thank you so much for your quick and thoughtful reply. We will take a look at this asap.

[jjxtra]

Are these methods in official nuget? Not seeing any async methods on the sftpclient...

[vanillajonathan]

I think the DownloadFileAsync and UploadFileAsync methods ought to be included, because I think this should be a async-first library where async should be the default behavior users use, and it should be easy to do so.

To be honest, I would like to see the non-async methods get removed as well as the legacy Begin* and End* async functions.

[WojciechNagorski]

Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0

[snargledorf]

Something that just bit me is that the OpenAsync method expects an absolute path while the Begin/End async methods for Upload/Download operate relative to the working directory.

Not sure what the best way to handle this would be, but it might be less likely to make a mistake if there were replacement methods on the SftpClient (DownloadAsync/UploadAsync) that behaved in the same way as the Begin/End methods (Relative to working directory), while OpenAsync remains absolute as to not break existing code.

[MarkEnverus]

namespace Renci.SshNet
{
    public static class SftpClientExtensions
    {
        public static async Task DownloadFileAsync(this SftpClient sftpClient, string path, Stream output, CancellationToken cancellationToken)
        {
            using (Stream remoteStream = await sftpClient.OpenAsync(path, FileMode.Open, FileAccess.Read, cancellationToken).ConfigureAwait(false))
            {
                await remoteStream.CopyToAsync(output, 81920, cancellationToken).ConfigureAwait(false);
            }
        }

        public static async Task UploadFileAsync(this SftpClient sftpClient, Stream input, string path, FileMode createMode, CancellationToken cancellationToken)
        {
            using (Stream remoteStream = await sftpClient.OpenAsync(path, createMode, FileAccess.Write, cancellationToken).ConfigureAwait(false))
            {
                await input.CopyToAsync(remoteStream, 81920, cancellationToken).ConfigureAwait(false);
            }
        }
    }
}

@IgorMilavec the issue here is that we cannot moq this code since we can't create a SftpFileStream (as it's not an interface and it's private). Whereas we can moq UploadFile (void return) unless OpenAsync returned a generic Stream instead of a wrapped Stream