-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Recursive list directory for FluentStorage.SFTP #56
Recursive list directory for FluentStorage.SFTP #56
Conversation
Can you extract the async method as a new method, because I don't like the pattern of methods being declared inside other methods. |
async Task ListDirectoryAsync(string folderToList) { | ||
IEnumerable<SftpFile> directoryContents = await client.ListDirectoryAsync(folderToList); | ||
var tempBlobCollection = directoryContents | ||
.Where(dc => (options.FilePrefix == null || dc.Name.StartsWith(options.FilePrefix)) | ||
&& (dc.IsDirectory || dc.IsRegularFile || dc.OwnerCanRead) | ||
&& !cancellationToken.IsCancellationRequested | ||
&& dc.Name != "." | ||
&& dc.Name != "..") | ||
.Take(options.MaxResults.Value) | ||
.Select(ConvertSftpFileToBlob) | ||
.Where(options.BrowseFilter).ToList(); | ||
|
||
lock (lockObject) { | ||
blobCollection.AddRange(tempBlobCollection); | ||
} | ||
|
||
if (options.Recurse == true) { | ||
IEnumerable<string> subFoldersToList = tempBlobCollection | ||
.Where(x => x.IsFolder == true) | ||
.Select(x => x.FullPath); | ||
#if NET6_0 | ||
await Parallel.ForEachAsync(subFoldersToList, async (subFolder, token) => { | ||
await ListDirectoryAsync(subFolder); | ||
}); | ||
#else | ||
foreach (string subFolder in subFoldersToList) { | ||
await ListDirectoryAsync(subFolder); | ||
} | ||
#endif | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract this to a new method please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do this please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just finsihed for the day. I'll make this my first task tomorrow morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the method out, bumped SSH.NET to 2023.0.1, and removed Renci.SshNet.Async. With that changed I had to change SftpClient to use ListDirectoryAsync which is an IAsyncEnumerable. I am unsure if I am using it correctly, or if it can be made more efficient.
Compared to those other test results,
ParallelForEach = 3.0691516 sec
foreach = 17.7257746 sec
IAsyncEnumerable = 1.9855958 sec
This changed involved re-writing SftpClient to use ListDirectoryAsync which is an IAsyncEnumerable.
…of the class Also marked as readonly to make static analysis happy.
Thanks a lot! |
Fixes
Issue #55
Description
Not sure the preferred way to handle this. I created a local function so I could share the lock object rather than adding it as a class variable. If you are using .NET 6+ it allows the use of
Parallel.ForEachAsync
. Because of usingParallel.ForEachAsync
I also added a lock for updating the main list. Unsure if there are complications of allowing this to query a server multiple times at once (also with no upper bound set).I am also unsure if this should be littered with
.ConfigureAwait(false)
for all those tasks it is using.I have not run any of the tests to confirm these changes work, but this is copy/pasted where I am using it for an internal tool I am testing out.