Add IAsyncEnumerable streaming directory listing (Issue #45)#54
Add IAsyncEnumerable streaming directory listing (Issue #45)#54
Conversation
- Add ListAllEnumerableAsync, ListFilesEnumerableAsync, ListDirectoriesEnumerableAsync - Streaming methods yield return parsed entries as they arrive from the server - Support CancellationToken for early termination of large listings - Add Microsoft.Bcl.AsyncInterfaces polyfill for netstandard2.0/net462 - Set LangVersion to latest for C# 8+ async stream support on net462 - Existing buffered methods remain unchanged for backward compatibility
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b0a4c5b9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/CoreFtp/FtpClient.cs
Outdated
| await foreach ( var node in directoryProvider.ListAllEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); |
There was a problem hiding this comment.
The new async-stream listing methods only call ControlStream.GetResponseAsync() after the await foreach completes normally, so if callers cancel the token or break out early (a common streaming use case), this line is never reached. In that case the final LIST/MLSD control reply (e.g., 226/426) remains unread, and subsequent FTP commands can consume that stale reply and fail or desynchronize the session; the same pattern appears in all three *EnumerableAsync methods.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces IAsyncEnumerable for streaming directory listings, which is an excellent enhancement for performance and memory efficiency. However, a critical vulnerability exists in FtpClient.cs due to a lack of proper exception handling for the FTP control connection. If an exception occurs during streaming, the client fails to read the completion response, leading to protocol desynchronization and potential misinterpretation of subsequent FTP commands, which could result in incorrect program logic or security-relevant state errors. Additionally, there are opportunities for code simplification and consistency.
| public async IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| EnsureLoggedIn(); | ||
| Logger?.LogDebug( $"[FtpClient] Streaming all nodes in {WorkingDirectory}" ); | ||
|
|
||
| await foreach ( var node in directoryProvider.ListAllEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); | ||
| } |
There was a problem hiding this comment.
The newly introduced streaming methods (ListAllEnumerableAsync, ListFilesEnumerableAsync, and ListDirectoriesEnumerableAsync) are missing a try-finally block to ensure the FTP control connection remains synchronized.
In the FTP protocol, a directory listing command expects a completion response (e.g., 226 Transfer complete) on the control channel after the data transfer is finished. Currently, await ControlStream.GetResponseAsync() is only called if the await foreach loop completes successfully. If an exception occurs during iteration (due to network issues, parsing errors, or cancellation), this call is skipped, leaving a stale response on the control socket.
Subsequent commands on the same FtpClient instance will read this stale response instead of their own, which can lead to 'Response Smuggling' where a command appears to succeed (because it read a 226 success code) even if it failed or was never processed.
To fix this, wrap the await foreach in a try-finally block and move the GetResponseAsync() call into the finally block, similar to the implementation in the non-streaming ListAllAsync method.
| public async IAsyncEnumerable<FtpNodeInformation> ListFilesEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| EnsureLoggedIn(); | ||
| Logger?.LogDebug( $"[FtpClient] Streaming files in {WorkingDirectory}" ); | ||
|
|
||
| await foreach ( var node in directoryProvider.ListFilesEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); | ||
| } |
There was a problem hiding this comment.
Similar to ListAllEnumerableAsync, this method is missing a try-finally block to ensure ControlStream.GetResponseAsync() is called even if an exception occurs during streaming. This is necessary to maintain FTP protocol synchronization.
| public async IAsyncEnumerable<FtpNodeInformation> ListDirectoriesEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| EnsureLoggedIn(); | ||
| Logger?.LogDebug( $"[FtpClient] Streaming directories in {WorkingDirectory}" ); | ||
|
|
||
| await foreach ( var node in directoryProvider.ListDirectoriesEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); | ||
| } |
There was a problem hiding this comment.
Similar to ListAllEnumerableAsync, this method is missing a try-finally block to ensure ControlStream.GetResponseAsync() is called even if an exception occurs during streaming. This is necessary to maintain FTP protocol synchronization.
| await ftpClient.dataSocketSemaphore.WaitAsync( cancellationToken ); | ||
| try | ||
| { | ||
| stream = await ftpClient.ConnectDataStreamAsync(); |
There was a problem hiding this comment.
The MlsdDirectoryProvider checks if the stream returned from ConnectDataStreamAsync is null. For consistency and robustness, it's a good idea to add a similar null check here to handle cases where a data connection could not be established.
stream = await ftpClient.ConnectDataStreamAsync();
if ( stream == null )
throw new FtpException( "Could not establish a data connection" );| public override async IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| await foreach ( var node in ListNodesEnumerableAsync( null, cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
| } |
There was a problem hiding this comment.
This async iterator method unnecessarily re-wraps the IAsyncEnumerable returned by ListNodesEnumerableAsync. This creates an extra state machine with a slight performance overhead. You can simplify this by directly returning the result of ListNodesEnumerableAsync. This feedback also applies to ListFilesEnumerableAsync and ListDirectoriesEnumerableAsync.
public override IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default )
{
return ListNodesEnumerableAsync( null, cancellationToken );
}| } | ||
| finally | ||
| { | ||
| stream?.Dispose(); |
There was a problem hiding this comment.
For consistency with MlsdDirectoryProvider and to prevent accidental use of a disposed object, it's good practice to set the stream field to null after it has been disposed.
stream?.Dispose();
stream = null;| public override async IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| await foreach ( var node in ListNodeTypeEnumerableAsync( null, cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
| } |
There was a problem hiding this comment.
Similar to the ListDirectoryProvider, this async iterator method re-wraps the IAsyncEnumerable returned by ListNodeTypeEnumerableAsync, creating an unnecessary state machine. You can simplify this by directly returning the enumerable. This feedback also applies to ListFilesEnumerableAsync and ListDirectoriesEnumerableAsync.
public override IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default )
{
return ListNodeTypeEnumerableAsync( null, cancellationToken );
}There was a problem hiding this comment.
Pull request overview
Adds async-streaming (IAsyncEnumerable<FtpNodeInformation>) directory listing APIs so callers can process entries as they arrive instead of buffering full listings in memory, addressing Issue #45.
Changes:
- Added new public
FtpClientstreaming listing methods:ListAllEnumerableAsync,ListFilesEnumerableAsync,ListDirectoriesEnumerableAsync. - Added streaming counterparts throughout directory listing providers (
IDirectoryProvider,DirectoryProviderBase,ListDirectoryProvider,MlsdDirectoryProvider). - Updated build settings to support async streams on older TFMs (LangVersion + conditional
Microsoft.Bcl.AsyncInterfaces).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CoreFtp/FtpClient.cs | Introduces public async-stream listing APIs that delegate to providers and then read the final control-stream response. |
| src/CoreFtp/CoreFtp.csproj | Enables newer C# language features and adds async-interfaces polyfill conditionally for older target frameworks. |
| src/CoreFtp/Components/DirectoryListing/IDirectoryProvider.cs | Extends provider abstraction with streaming listing methods. |
| src/CoreFtp/Components/DirectoryListing/DirectoryProviderBase.cs | Adds virtual streaming method stubs to the base provider class. |
| src/CoreFtp/Components/DirectoryListing/ListDirectoryProvider.cs | Implements streaming parsing over LIST responses with cancellation support and semaphore handling. |
| src/CoreFtp/Components/DirectoryListing/MlsdDirectoryProvider.cs | Implements streaming parsing over MLSD responses with cancellation support and semaphore handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| EnsureLoggedIn(); | ||
| Logger?.LogDebug( $"[FtpClient] Streaming all nodes in {WorkingDirectory}" ); | ||
|
|
||
| await foreach ( var node in directoryProvider.ListAllEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); | ||
| } |
There was a problem hiding this comment.
These async-iterator methods only call ControlStream.GetResponseAsync() after the await foreach completes. If the consumer stops enumeration early (break/exception/cancellation), that code will not run, leaving the control connection with an unread final response (e.g., 226/426) and potentially breaking subsequent commands. Wrap the enumeration in a try/finally so the final response is drained whenever the iterator is disposed (and consider whether to pass the same CancellationToken).
| public async IAsyncEnumerable<FtpNodeInformation> ListAllEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { | ||
| EnsureLoggedIn(); | ||
| Logger?.LogDebug( $"[FtpClient] Streaming all nodes in {WorkingDirectory}" ); | ||
|
|
||
| await foreach ( var node in directoryProvider.ListAllEnumerableAsync( cancellationToken ).ConfigureAwait( false ) ) | ||
| yield return node; | ||
|
|
||
| await ControlStream.GetResponseAsync(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Streams all files in the current working directory as they are parsed from the server | ||
| /// </summary> | ||
| /// <param name="cancellationToken"></param> | ||
| /// <returns></returns> | ||
| public async IAsyncEnumerable<FtpNodeInformation> ListFilesEnumerableAsync( [EnumeratorCancellation] CancellationToken cancellationToken = default ) | ||
| { |
There was a problem hiding this comment.
New streaming listing methods were added as public methods on FtpClient, but IFtpClient (which FtpClient implements) does not expose them. If consumers depend on IFtpClient (e.g., DI/mocking), they won't be able to use the new API; consider adding these methods to IFtpClient (and any related abstractions) to keep the public surface consistent.
| <AssemblyTitle>CoreFTP</AssemblyTitle> | ||
|
|
||
| <Authors>Nick Briscoe</Authors> | ||
| <LangVersion>latest</LangVersion> |
There was a problem hiding this comment.
Using <LangVersion>latest</LangVersion> makes builds non-deterministic across SDK upgrades (language version can change and introduce new warnings/errors/semantics). Consider pinning to the minimum required (C# 8.0 for async streams) or latestMajor, or applying it conditionally only where needed (e.g., net462) to reduce upgrade risk.
| <LangVersion>latest</LangVersion> | |
| <LangVersion>latestMajor</LangVersion> |
| using System.Collections.Generic; | ||
| using System.Collections.ObjectModel; | ||
| using System.IO; | ||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
using System.Runtime.CompilerServices; appears unused in this file (no references to types from this namespace). Consider removing it to avoid unnecessary usings / analyzer warnings.
| using System.Runtime.CompilerServices; |
…pers, add null guards
|
Addressed all review feedback in
|
Summary
Adds streaming
IAsyncEnumerable<FtpNodeInformation>directory listing methods that yield parsed entries as they arrive from the server, without buffering the entire listing in memory first.Closes #45
New Public API
Changes
New Methods
FtpClient.ListAllEnumerableAsync(CancellationToken)FtpClient.ListFilesEnumerableAsync(CancellationToken)FtpClient.ListDirectoriesEnumerableAsync(CancellationToken)Internal Changes
IAsyncEnumerablestreaming methods toIDirectoryProvider,DirectoryProviderBase,ListDirectoryProvider, andMlsdDirectoryProviderdataSocketSemaphoreand dispose the data stream infinallyblocksCancellationTokenvia[EnumeratorCancellation]attributeBuild Infrastructure
Microsoft.Bcl.AsyncInterfacespolyfill (conditional fornetstandard2.0/net462only)<LangVersion>latest</LangVersion>to enable C# 8+ async stream syntax onnet462Backward Compatibility
ListAllAsync(),ListFilesAsync(),ListDirectoriesAsync()remain unchanged