Skip to content

Add Read(Span<byte>) override to MultipartReaderStream - #69070

Open
SimonCropp wants to merge 3 commits into
dotnet:mainfrom
SimonCropp:webutilities/multipartreaderstream-span
Open

Add Read(Span<byte>) override to MultipartReaderStream#69070
SimonCropp wants to merge 3 commits into
dotnet:mainfrom
SimonCropp:webutilities/multipartreaderstream-span

Conversation

@SimonCropp

@SimonCropp SimonCropp commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Add Read(Span) override to MultipartReaderStream

Description

Move the synchronous read implementation onto Read(Span) and have the array overload delegate to it, so a caller reading a section into a span does not pay the base Stream fallback's rented array and copy. Also add the throwing Write(ReadOnlySpan) to match the other write overrides.

Move the synchronous read implementation onto Read(Span<byte>) and have
the array overload delegate to it, so a caller reading a section into a
span does not pay the base Stream fallback's rented array and copy. Also
add the throwing Write(ReadOnlySpan<byte>) to match the other write
overrides.
Copilot AI lite review requested due to automatic review settings September 4, 2026 23:44
@github-actions github-actions Bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 4, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Sep 4, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Thanks for your PR, @SimonCropp. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new span-based sync read path calls into BufferedReadStream.Read(Span<byte>) fallback (rent+copy) and—because Read(byte[],...) now delegates—appears to introduce a performance regression for all sync reads.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates MultipartReaderStream’s synchronous read/write surface to add a Read(Span<byte>) override (and have the byte[] overload delegate to it), plus adds a throwing Write(ReadOnlySpan<byte>) override to match the existing write behavior.

Changes:

  • Added Write(ReadOnlySpan<byte>) override that throws NotSupportedException.
  • Routed Read(byte[], int, int) to Read(Span<byte>) and implemented the core sync read logic on the span overload.
  • Updated internal sync read slices to use span-based reads.
File summaries
File Description
src/Http/WebUtilities/src/MultipartReaderStream.cs Moves sync read implementation to Read(Span<byte>) and adds Write(ReadOnlySpan<byte>) throw override.
Review details

Suppressed comments (1)

src/Http/WebUtilities/src/MultipartReaderStream.cs:176

  • The new synchronous Read(Span<byte>) override isn’t exercised by existing multipart tests (e.g., MultipartReaderTests uses CopyToAsync/ReadAsync paths). Since this change affects the sync read implementation and is intended to improve span-based reads, please add a unit test that reads section.Body via Span<byte> (and ideally also via the byte[] overload) to validate correct data/boundary behavior and guard against regressions.
    public override int Read(byte[] buffer, int offset, int count)
        => Read(buffer.AsSpan(offset, count));

    public override int Read(Span<byte> buffer)
    {
        if (_finished)
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Http/WebUtilities/src/MultipartReaderStream.cs
Read each section of a two part body into a span smaller than the
section, with both the default buffer and one smaller than the body so
the partial boundary match path is exercised as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants