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

Fix #395 : SftpClient Enumerates Rather Than Accumulates Directory Items #396

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

znamenap
Copy link

@znamenap znamenap commented Mar 4, 2018

Hi @drieseng ,

Please, could you check this pull request for #395 ?

The enumerating directory approach has better time results on very huge directorries (a lot of files meaning 80000). In many situations happened the call timed out while using the actual method ListDirectory.

I've also tested in my test environment and the same tests as like in ListDirectory.

Thanks.
Pavel

znamenap added a commit to znamenap/SSH.NET that referenced this pull request Mar 4, 2018
@znamenap znamenap force-pushed the develop branch 2 times, most recently from dc7c792 to 57adbee Compare September 1, 2020 21:27
znamenap added a commit to znamenap/SSH.NET that referenced this pull request Sep 1, 2020
@Alexinio97
Copy link

Hi,
I'm also interested in these changes, is there a chance for this to get merged soon?

@znamenap
Copy link
Author

This PR is ready to review and approve potentially.

@Rob-Hague
Copy link
Collaborator

I like the change, but I don't think we should add a new method for it. Instead, we should just change ListDirectory to yield. We now have ListDirectoryAsync which yields.

I also think we should remove the listCallback parameter from ListDirectory: yielding will make it less useful, but also executing an arbitrary callback on the threadpool during enumeration is unnecessary complication and not very canonical of an IEnumerable method (it seems to be a leftover of the Begin/End APM methods)

What do you think @WojciechNagorski ?

@znamenap
Copy link
Author

znamenap commented Sep 24, 2023

My two cents two the discussion regarding changing the internal implementation of the ListDirectory, changing it to use the yield return will certainly break the existing consumer's business logic with the unwritten behavior of returing a complete list in fact. Many of the consumers had already implemented logic such way they are in fact expecting the full list. Therefore, I'd rather suggest changing ListDirectory to return IList interface as part of the major breaking changes and do not introduce the yield return at all. If consumers would like to change the behavior, they can simply change the call from ListDirectory to EnumerateDirectory.
The open-ended enumeration is a contract to the runtime of the consumer's application context created with the SftpConnection, people may be reporting issues because of reaching the out of the scope for the connection and request enumerating/listing the directory. We should consider this option also.

@znamenap
Copy link
Author

Regarding the callback, I second that idea, the thread pool may be in excess. I presume the callback was added in order to report the progress and hence, I'm suggesting this may be replaced with IProgress<T>. Plus, I think, this change should be out-scoped to this PR and reported as another issue to solve later.

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

Successfully merging this pull request may close these issues.

3 participants