Use span-based read helper for control stream

[sparkeh9]

Motivation

• Modernize the control-stream byte-by-byte read path to avoid a heap-allocated new byte[1] allocation and reduce per-byte GC pressure.
• Centralize single-byte reads into a small Span<byte>-aware helper to keep EOF and cancellation semantics consistent across the loop.
• Ensure Span<T> support on the targeted frameworks by adding the required package reference where needed.

Description

• Add a private helper int Read(Span<byte> buffer) that uses ReadByte() to populate a single-byte Span and returns 1 for a read byte or 0 for EOF.
• Replace the previous var buffer = new byte[1] usage in ReadLine with Span<byte> buffer = stackalloc byte[1] and switch the loop to call Read(buffer) to read one byte at a time.
• Update src/CoreFtp/CoreFtp.csproj to include a PackageReference to System.Memory to provide Span<T> support on the targeted frameworks.

Testing

• No automated tests were run for this change.

---

Codex Task

---

Note

Low Risk
Low risk perf refactor confined to FtpControlStream line-reading and a new dependency; main risk is subtle EOF/byte-read behavior differences across target frameworks.

Overview
Reduces allocations in the FTP control-stream line reader. FtpControlStream.ReadLine now reads bytes via a Span<byte> (stackalloc) path instead of allocating new byte[1] per loop iteration, using a new Read(Span<byte>) helper backed by an overridden ReadByte() and a cached 1-byte buffer.

Adds a System.Memory package reference in CoreFtp.csproj to ensure Span<T> support on targeted frameworks.

^{Written by Cursor Bugbot for commit 1220b8b. This will update automatically on new commits. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 964e7d8b3e

ℹ️ 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".

[chatgpt-codex-connector]

Avoid per-byte allocation by not using Stream.ReadByte

The new helper calls ReadByte(), but Stream.ReadByte() allocates a new one-byte array on each call before delegating to Read(byte[], int, int). In this loop, that means a heap allocation per byte read, which is a regression from the previous single-buffer reuse and defeats the stated GC/perf motivation. This will be most visible when reading long control responses. Consider calling the existing Read(byte[], int, int)/NetworkStream.Read with the provided buffer instead of ReadByte() to keep the stackalloc benefit.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

This PR attempts to modernize the FTP control stream's byte-reading implementation by introducing Span<byte> to reduce heap allocations during line-by-line reading of FTP responses. However, the implementation does not achieve the stated performance goals.

Changes:

• Added a new private Read(Span<byte>) helper method that calls ReadByte() to populate a span
• Modified ReadLine to use stackalloc byte[1] instead of new byte[1]
• Added System.Memory package reference (v4.5.5) to support Span<T> on target frameworks

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs	Added span-based Read helper and converted ReadLine to use stackalloc, but the optimization is ineffective due to ReadByte() implementation
src/CoreFtp/CoreFtp.csproj	Added System.Memory package reference to enable Span support

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

While this change adds the System.Memory package reference which is necessary for Span support, the PR description's claim about "modernizing to avoid a heap-allocated new byte[1] allocation and reduce per-byte GC pressure" is not actually achieved by the current implementation. The base Stream.ReadByte() still creates a heap allocation internally. Consider updating the PR description to reflect that additional work is needed to achieve the stated performance goals, or fix the implementation as noted in the other comment.

[Copilot]

The optimization to avoid heap allocation is ineffective because ReadByte() is not overridden in this class. The base Stream.ReadByte() implementation allocates a temporary byte array and calls Read(byte[], int, int), which defeats the purpose of using Span.

To achieve the intended optimization, either:

1. Override ReadByte() to directly read from NetworkStream, or
2. Implement Read(Span) to directly read from NetworkStream instead of calling ReadByte()

The current implementation still results in a heap-allocated byte[1] array on each call.

Suggested change

	if ( buffer.IsEmpty )
	return 0;
	var value = ReadByte();
	if ( value == -1 )
	return 0;
	buffer[ 0 ] = (byte) value;
	return 1;
	if ( NetworkStream == null )
	{
	return 0;
	}
	return NetworkStream.Read( buffer );

[Copilot]

The use of stackalloc in ReadLine creates a Span on the stack that is used across loop iterations and potentially across async boundaries (since ReadLine can be called from async methods like GetResponseAsync). While this specific usage appears safe because ReadLine itself is synchronous and the span doesn't escape the method, it's worth noting that this pattern requires careful consideration if the method is ever converted to async.

Additionally, the buffer is only used to read one byte at a time, so the performance benefit of Span over byte[] is minimal here. The real benefit would come from fixing the Read(Span) implementation to avoid the heap allocation in ReadByte().

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

Shared buffer in ReadByte causes thread safety issue

Low Severity

The new readByteBuffer instance field is shared across all calls to ReadByte(). If multiple threads call ReadByte() concurrently on the same FtpControlStream instance, they will read into and return from the same buffer, causing data corruption. Previously, the base Stream.ReadByte() implementation used a local buffer per call, making it inherently thread-safe at the buffer level. The class's internal usage through GetResponseAsync is protected by receiveSemaphore, but ReadByte() is a public override that external code can call directly.

Additional Locations (1)

• src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs#L114-L122

 

[cursor]

TOCTOU race in ReadByte can throw NullReferenceException

Low Severity

The new ReadByte() evaluates the NetworkStream property twice—once for the null check and again for the Read call. Since NetworkStream is a computed property (SslStream ?? BaseStream), a concurrent call to Disconnect() can set BaseStream to null between these evaluations, causing the second evaluation to return null and throw a NullReferenceException. The old Read(byte[], int, int) method used the null-conditional operator ?. which evaluates once and captures the reference, avoiding this race.

 

[sparkeh9]

@copilot check if any more changes are needed. Add tests to any potential edge cases

[Copilot]

@sparkeh9 I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you.