Add IAsyncEnumerable<SftpFile> to enumerate remote files #907
Conversation
|
I did not change ISftpClient yet, so this PR is only a draft at this time... |
|
Depending on what targets you want to support. |
|
I think there is no point in introducing new features into SSH.NET that target non-supported runtimes. This basically limits our discussion to net472. Also, I think this should not be a discussion of whether we CAN support IAsyncEnumerable on net472, but rather a discussion of whether we SHOULD support it. The only real life difference I see between Currently SSH.NET has no external dependencies (SshNet.Security.Cryptography is a sibling and under the same governance) when targeting net472 or netstandard2.0/2.1. I think we should keep it this way, at least it terms of the default package. If someone wants to do a private build, this is of course entirely up to him/her. We can even introduce a feature flag if it helps someone, but I really really would hope to stay dependency free if possible. |
|
I also prefer to also keep the number of dependencies to an absolute minimum. |
|
Should we add IAsyncDisposable implementation while we're at it? |
src/Renci.SshNet/ISftpClient.cs
Outdated
| #if NETSTANDARD2_1_OR_GREATER | ||
| [Obsolete("Use EnumerateDirectoryAsync()")] | ||
| [System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)] | ||
| #endif | ||
| Task<IEnumerable<SftpFile>> ListDirectoryAsync(string path, CancellationToken cancellationToken); |
There was a problem hiding this comment.
I'd prefer to only expose the IAsyncEnumerable version (with ListDirectoryAsync as name), even if that means it's .NET Standard 2.1 (or higher) only.
Is the NETSTANDARD2_1_OR_GREATER symbol also defined for .NET 5.0 or 6.0?
There was a problem hiding this comment.
If we reuse the same name with a different signature, this means a breaking change when the caller changes its target framework. If we do a "soft" transition with Obsoletes and a new name then we give callers time to transition to the new method at their own pace. But we do end up with a new name...
The symbol NETSTANDARD* is unfortunately not defined when targeting net5.0 or net6.0, so this check would need to be changed to #if NETSTANDARD2_1_OR_GREATER || NET5_0_OR_GREATER.
| <SignAssembly>true</SignAssembly> | ||
| <TargetFrameworks>net35;net40;net472;netstandard1.3;netstandard2.0</TargetFrameworks> | ||
| <TargetFrameworks>net35;net40;net472;netstandard1.3;netstandard2.1</TargetFrameworks> |
There was a problem hiding this comment.
Do we want to keep these (and other) changes to have a .NET Standard 2.1 flavor in this PR, or separate them out?
This PR would of course rely on that one being merged first.
There was a problem hiding this comment.
IMO it would be more clean if we first remove old targets and move to .NET Standard 2.0. We should probably also add .NET Standard 2.1 target in that PR, even without using 2.1 features. Later we can implement new 2.1 features in separate PRs (like this one) one feature at a time.
src/Renci.SshNet/ISftpClient.cs
Outdated
| /// <exception cref="SftpPermissionDeniedException">Permission to list the contents of the directory was denied by the remote host. <para>-or-</para> A SSH command was denied by the server.</exception> | ||
| /// <exception cref="SshException">A SSH error where <see cref="Exception.Message" /> is the message from the remote host.</exception> | ||
| /// <exception cref="ObjectDisposedException">The method was called after the client was disposed.</exception> | ||
| IAsyncEnumerable<SftpFile> EnumerateDirectoryAsync(string path, CancellationToken cancellationToken); |
There was a problem hiding this comment.
This should return ISftpFile instead of the concrete SftpFile.
src/Renci.SshNet/SftpClient.cs
Outdated
| throw new ArgumentNullException(nameof(path)); | ||
| if (_sftpSession == null) | ||
| throw new SshConnectionException("Client not connected."); | ||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
I suppose we perform this check in all async methods that we invoke here. Do we want this check here as well?
|
I'm closing this PR because I've already removed old .NET frameworks and IAsyncEnumerable has been moved to #1126 |
@drieseng, this is my proposal on how to gradually implement
IAsyncEnumerablefor asynchronously enumerating remote files.First, the prerequisites:
The latter means SSH.NET would no longer compile with VS 2017 or earlier. Is this an issue?
I propose to have two distinct methods for enumerating remote files:
Task<IEnumerable<SftpFile>> ListDirectoryAsync(string path, CancellationToken cancellationToken):This method works asynchronously, but will only complete the task after all the files have been read from the remote.
IAsyncEnumerable<SftpFile> EnumerateDirectoryAsync(string path, [EnumeratorCancellation]CancellationToken cancellationToken):This method works asynchronously too, but will return remote files in chunks as the are received by a single
RequestReadDirAsynccall. This allows the caller to abort enumeration early if they satisfied their goal before reaching the end of enumeration.Assuming the consumer is currently targeting net472, they would only have access to ListDirectoryAsync. If the consumer later changes its target to netstandard2.1 (or later, i.e. net5.0 or net6.0), they would still be able to call ListDirectoryAsync, but it will light up as obsolete. In addition, they would now be able to call EnumerateDirectoryAsync. This allows for gradual migration and does not present a breaking change for the caller when raising their target.
Any consumer of EnumerateDirectoryAsync would need to have access to
async foreach, which is a C# 8.0 feature, meaning the consumer can target netstandard2.1 or later. So I don't see any realistic scenario for using IAsyncEnumerable in pre-netstandard2.1 world.