Fix #395 : SftpClient Enumerates Rather Than Accumulates Directory Items

[znamenap]

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

[Alexinio97]

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

[znamenap]

This PR is ready to review and approve potentially.

[Rob-Hague]

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]

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<SftpFile> 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]

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.