Skip to content

Use Span<T>-based methods in System.Net.Mail #115111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Apr 28, 2025

This PR refactors some of the internals to move away from methods that take byte[] + offset + count and replaces them with those taking (ReadOnly)Span or Memory.

This PR also refactors DelegatedStream and its derived classes, now the derived classes need only implement a few methods (one sync and one async for read/write) and all other overloads will use it.

This is a first step in replacing ATP (i.e. Begin+End methods) with async-await methods.

@Copilot Copilot AI review requested due to automatic review settings April 28, 2025 13:29
Copy link
Contributor

@Copilot 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 refactors several System.Net.Mail components to use Span- and Memory-based overloads in place of the legacy byte[] + offset + count patterns, as part of the migration toward async–await and improving performance and safety.

  • Replaces legacy encoding/decoding methods with span-based alternatives in test and production code.
  • Updates DelegatedStream and its derived classes so that only key read/write methods need implementation.
  • Adjusts supporting classes (e.g. Base64Encoder, ByteEncoder, QEncoder) to work with the new APIs.

Reviewed Changes

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

Show a summary per file
File Description
tests/Unit/ByteEncodingTest.cs Updated to call EncodeBytes with a byte array directly.
tests/Unit/Base64EncodingTest.cs Switched DecodeBytes and EncodeBytes to span-based variants.
src/System/Net/Mime/QuotedPrintableStream.cs Updated EncodeBytes/DecodeBytes to use spans and fixed a typo in method naming.
src/System/Net/Mime/QEncoder.cs Changed signature to use ReadOnlySpan and corrected a method name typo.
src/System/Net/Mime/QEncodedStream.cs Replaced legacy async patterns with span/memory operations and updated write/read methods.
src/System/Net/Mime/IEncodableStream.cs, IByteEncoder.cs Updated interface definitions to use span-based methods.
src/System/Net/Mime/ByteEncoder.cs Refactored methods to use spans and fixed a naming typo.
src/System/Net/Mime/Base64Encoder.cs Replaced offset/count parameters with span-based calls and updated FlushAsync accordingly.
src/System/Net/Mail/SmtpReplyReaderFactory.cs Modified read processing to work with Span instead of arrays.
src/System/Net/DelegatedStream.cs Refactored read/write overrides to delegate to abstract span-based implementations and updated Begin/End methods.
src/System/Net/CloseableStream.cs Updated read/write methods to call BaseStream methods using spans.
src/System/Net/BufferedReadStream.cs Updated Read/ReadAsync and Push to use spans for improved memory operations.
src/System/Net/Base64Stream.cs Updated encoding/decoding, read/write and FlushAsync to use spans/memory.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@rzikm rzikm requested review from a team and Copilot April 28, 2025 13:40
Copy link
Contributor

@Copilot 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 modernizes System.Net.Mail by refactoring legacy byte[]‑based encoding and stream methods to use (ReadOnly)Span and Memory overloads. Key changes include:

  • Updating encoding methods across multiple MIME and stream classes to use Span‑based APIs.
  • Refactoring stream operations (both sync and async) in DelegatedStream, Base64Stream, EightBitStream, and related classes.
  • Aligning associated tests with the new Span‑based methods.

Reviewed Changes

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

Show a summary per file
File Description
tests/Unit/ByteEncodingTest.cs Updated tests to call Span‑based overloads for encoding bytes.
tests/Unit/Base64EncodingTest.cs Replaced byte[] overload calls with Span‑based overloads in encoding/decoding tests.
src/System/Net/Mime/QuotedPrintableStream.cs Refactored encoding and decoding methods to use Span and ReadOnlySpan; adjusted async flush implementation.
src/System/Net/Mime/QEncoder.cs Fixed a typo in the encoded byte method name from “ApppendEncodedByte” to “AppendEncodedByte”.
src/System/Net/Mime/QEncodedStream.cs Updated stream methods to use the new Span‑based encoding/decoding patterns.
src/System/Net/Mime/MimeBasePart.cs Updated header decoding to pass a span view of the input buffer.
src/System/Net/Mime/IEncodableStream.cs and IByteEncoder.cs Updated interface definitions to use Span/ReadOnlySpan parameters.
src/System/Net/Mime/EightBitStream.cs Refactored Write methods to leverage async WriteAsync with Span/Memory; note potential return type mismatch.
src/System/Net/Mime/ByteEncoder.cs Modernized encoding logic via Span‑based byte processing.
src/System/Net/Mime/Base64Encoder.cs Adjusted Base64 encoding to operate on ReadOnlySpan input.
src/System/Net/Mail/SmtpReplyReaderFactory.cs Revised reply reader methods to use Span-based processing on buffers.
src/System/Net/DelegatedStream.cs Refactored abstract stream members to require Span/Memory‐based overrides, improving async support.
src/System/Net/CloseableStream.cs Updated read/write methods to use Span/Memory overloads for streamlined operations.
src/System/Net/BufferedReadStream.cs Refactored buffered read logic to use Span/Memory, improving buffer copy operations.
src/System/Net/Base64Stream.cs Modernized read, write, async flush, and decoding routines using Span/Memory APIs.
Comments suppressed due to low confidence (1)

src/libraries/System.Net/Mime/EightBitStream.cs:82

  • The overridden Write method is returning an asynchronous ValueTask while the method signature remains void. Adjust the method signature or use a purely synchronous implementation to maintain the expected contract.
return BaseStream.WriteAsync(buffer, cancellationToken);

@rzikm rzikm requested review from MihaZupan and Copilot April 29, 2025 06:36
Copy link
Contributor

@Copilot 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 refactors several email-related stream and encoding classes in System.Net.Mail to use (ReadOnly)Span and Memory APIs instead of byte arrays with offset/count. It also consolidates duplicated overloads into a smaller set of core async and sync methods.

  • Updated tests to call the new Encode/Decode overloads.
  • Migrated core stream methods (reading, writing, flushing) and encoding interfaces to use Span/Memory.
  • Refactored DelegatedStream and related classes to expose minimal abstract methods.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/System.Net.Mail/tests/Unit/ByteEncodingTest.cs Updated test usage of EncodeBytes overload to remove offset/count parameters.
src/libraries/System.Net.Mail/tests/Unit/Base64EncodingTest.cs Adjusted tests to use the new Span-based DecodeBytes and EncodeBytes methods.
src/libraries/System.Net.Mail/src/System/Net/Mime/QuotedPrintableStream.cs Refactored encoding methods to use Span/Memory APIs and updated flushing logic.
src/libraries/System.Net/Mime/QEncoder.cs Corrected the typo in the encoded byte method and updated method signature.
src/libraries/System.Net/Mime/QEncodedStream.cs Migrated stream decoding and writing to use Span/Memory instead of byte arrays.
src/libraries/System.Net/Mime/MimeBasePart.cs Updated decoding call to use the new DecodeBytes overload.
src/libraries/System.Net/Mime/IEncodableStream.cs Changed interface to reflect Span-based decoding API.
src/libraries/System.Net/Mime/IByteEncoder.cs Updated the encoding method signature to accept ReadOnlySpan.
src/libraries/System.Net/Mime/EightBitStream.cs Adjusted stream write operations and async writing to use Span/Memory APIs.
src/libraries/System.Net/Mime/ByteEncoder.cs Refactored encoding logic to use ReadOnlySpan and corrected method naming.
src/libraries/System.Net/Mime/Base64Encoder.cs Updated line break and padding handling to work with spans.
src/libraries/System.Net/Mail/SmtpReplyReaderFactory.cs Migrated buffered reading and pushing operations to use spans.
src/libraries/System.Net/Mail/DelegatedStream.cs Refactored abstract stream members and sealed implementations to use span-based operations.
src/libraries/System.Net/Mail/CloseableStream.cs Updated read and write methods to use asynchronous Span/Memory calls.
src/libraries/System.Net/Mail/BufferedReadStream.cs Refactored buffering and push logic to use Span APIs.
src/libraries/System.Net/Mail/Base64Stream.cs Migrated encoding/decoding and flush operations to the new span-based implementations.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Nice cleanup

@@ -80,14 +78,14 @@ public override void Close()
base.Close();
}

public unsafe int DecodeBytes(byte[] buffer, int offset, int count)
public unsafe int DecodeBytes(Span<byte> buffer)
Copy link
Member

Choose a reason for hiding this comment

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

Something to consider for the future if we're cleaning this up is to try and replace all of this with built-in base64 helpers.

}

// adds additional content to the beginning of the buffer
// so the layout of the storedBuffer will be
// <buffer><existingBuffer>
// after calling push
internal void Push(byte[] buffer, int offset, int count)
internal void Push(ReadOnlySpan<byte> buffer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion for future: This looks replaceable with our ArrayBuffer helper

{
ValidateBufferArguments(buffer, offset, count);
throw new NotImplementedException();

Choose a reason for hiding this comment

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

In QuotedPrintableStream is NotSupportedException being thrown:

throw new NotSupportedException(SR.ReadNotSupported);

But in EightBitStream and QEncodedStream the NotImplementedException is being thrown.
Shouldn't this be NotSupportedException everywhere, since reading is not supported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants