Skip to content

Add FtpControlStream edge case unit tests; fix Span read path heap allocation#51

Merged
sparkeh9 merged 2 commits intocodex/replace-manual-buffer-with-span-typefrom
copilot/sub-pr-50
Feb 28, 2026
Merged

Add FtpControlStream edge case unit tests; fix Span read path heap allocation#51
sparkeh9 merged 2 commits intocodex/replace-manual-buffer-with-span-typefrom
copilot/sub-pr-50

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 28, 2026

Stream.ReadByte() in the base class allocates a byte[1] on every call, defeating the goal of the Span<byte> read path. Additionally, no unit tests existed for the control stream's line-reading logic.

Changes

  • Override ReadByte() with a cached readByteBuffer = new byte[1] field, so the single-byte read path no longer allocates per call:

    private readonly byte[] readByteBuffer = new byte[1];
    
    public override int ReadByte()
    {
        if (NetworkStream == null) return -1;
        var bytesRead = NetworkStream.Read(readByteBuffer, 0, 1);
        return bytesRead == 0 ? -1 : readByteBuffer[0];
    }

    Note: NetworkStream.Read(Span<byte>) is not available on the target frameworks (netstandard1.6, net452), so the cached-buffer approach is the correct level of optimization here.

  • Unit tests added in FtpControlStreamTests/When_reading_control_stream.cs via a TestStream subclass that injects a MemoryStream as BaseStream, covering:

    • ReadLine: empty stream → null, LF-terminated, CRLF stripping, incomplete line (no LF) → null, null encoding throws, pre-cancelled token throws
    • ReadByte: null NetworkStream-1, byte available, EOF → -1

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: sparkeh9 <5622386+sparkeh9@users.noreply.github.com>
Copilot AI changed the title [WIP] Use span-based read helper for control stream Add FtpControlStream edge case unit tests; fix Span read path heap allocation Feb 28, 2026
@sparkeh9 sparkeh9 marked this pull request as ready for review February 28, 2026 17:17
Copilot AI review requested due to automatic review settings February 28, 2026 17:17
@sparkeh9 sparkeh9 merged commit 642a5c5 into codex/replace-manual-buffer-with-span-type Feb 28, 2026
1 check passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes per-call heap allocations from FtpControlStream’s single-byte read path and adds coverage for control-stream line parsing behavior, improving both performance and confidence in edge-case handling.

Changes:

  • Override FtpControlStream.ReadByte() to use a cached byte[1] buffer instead of allocating on every call.
  • Add integration tests covering ReadLine edge cases (empty stream, LF/CRLF termination, incomplete line, null encoding, pre-cancelled token) and ReadByte behavior (null stream, data available, EOF).

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

File Description
src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs Adds cached-buffer ReadByte() override to avoid repeated allocations in the 1-byte read path used by line reading.
tests/CoreFtp.Tests.Integration/FtpControlStreamTests/When_reading_control_stream.cs Adds tests validating ReadLine and ReadByte behavior across key edge cases.

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

sparkeh9 added a commit that referenced this pull request Feb 28, 2026
* Use span-based read helper for control stream

* Override ReadByte for control stream

* Merge PR #51

Merge Copilot tests for FtpControlStream

* Upgrade CoreFTP to .NET 10 and resolve vulnerable dependencies

* Fix CI annotations: resolve SYSLIB0039 and CS0108 warnings

* Bump version to 2.0.0 and add NuGet publish workflow

---------

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Nick Briscoe <nick.briscoe@razor.co.uk>
@sparkeh9 sparkeh9 deleted the copilot/sub-pr-50 branch February 28, 2026 21:55
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