[API Proposal]: Amend `WritableMemoryStream` and `ReadOnlyMemoryStream` to derive from `Stream`, not `MemoryStream`

[jozkee]

Background and motivation

.NET 11 adds two new public System.IO types, both currently deriving from MemoryStream:

public sealed class WritableMemoryStream : MemoryStream;
public sealed class ReadOnlyMemoryStream : MemoryStream;

Each wraps a Memory<byte>/ReadOnlyMemory<byte>, respectively. However, I think we should amend #82801 to use Stream as the base class given that none of the members MemoryStream adds on top of Stream are useful on these types, they are either deliberately disabled (GetBuffer/TryGetBuffer) or redundant, as detailed below.

• byte[] GetBuffer() and bool TryGetBuffer(out ArraySegment<byte>) are both deliberately overridden to be inert (GetBuffer always throws UnauthorizedAccessException, TryGetBuffer always returns false). They are disabled because these types are always over a user-supplied buffer and never opt into MemoryStream's publiclyVisible exposure: exposure can't work for ReadOnlyMemoryStream (its accessors return writable byte[]/ArraySegment<byte> over read-only memory); it could work for WritableMemoryStream but only when the backing of Memory<byte> is an array, so it is intentionally left off to keep the two aligned.
• int Capacity returns _memory.Length, which equals Length for these fixed buffers, making it redundant.
• byte[] ToArray() is just a caller-side copy that any Stream consumer can perform with Read/CopyTo, so it adds nothing unique.
• void WriteTo(Stream) is identical in effect to Stream.CopyTo(Stream), so it is redundant as well.

The change also removes a footgun for code that type-checks is MemoryStream and then calls GetBuffer/TryGetBuffer (which throw / return false here) turning a runtime surprise into a compile-time absence.

As a bonus, changing the base to Stream shrinks each instance from 80 B to 56 B (‑30%). The savings comes from the unused MemoryStream fields: _buffer, _capacity, _expandable, _exposable (never read) plus _origin (always 0), _writable (constant per type), and _length (equals _memory.Length). Each type then needs only _memory, _position, _isOpen, and _lastReadTask.

cc @ViveliDuCh @adamsitnik

API Proposal

namespace System.IO;

-public sealed partial class WritableMemoryStream : MemoryStream
+public sealed partial class WritableMemoryStream : Stream
{
-   public override int Capacity { get; set; }
-   public override byte[] GetBuffer();
-   public override bool TryGetBuffer(out ArraySegment<byte> buffer);
-   public override byte[] ToArray();
-   public override void WriteTo(Stream stream);
}

-public sealed partial class ReadOnlyMemoryStream : MemoryStream
+public sealed partial class ReadOnlyMemoryStream : Stream
{
-   public override int Capacity { get; set; }
-   public override byte[] GetBuffer();
-   public override bool TryGetBuffer(out ArraySegment<byte> buffer);
-   public override byte[] ToArray();
-   public override void WriteTo(Stream stream);
}

API Usage

N/A, usage is identical; only the base class and the redundant MemoryStream-specific members change. Code consuming these as Streams is unaffected.

Alternative (additional) Designs

Expose the underlying buffer without having to opt-into it.

public sealed partial class WritableMemoryStream : Stream
{
+    public Memory<byte> GetMemory();
}

public sealed partial class ReadOnlyMemoryStream : Stream
{
+    public ReadOnlyMemory<byte> GetReadOnlyMemory();
}

No response

Risks

Preview-only break: code assigning to MemoryStream or matching is MemoryStream breaks.

Additional Information

But the types have MemoryStream in their name...

There's precedent of that being overthrown by UnmanagedMemoryStream.

MemoryStream has so many modes.

Extending on the dangers of deriving from MemoryStream, there's cases where code is unaware of other modalities and don't handle it correctly or discourage them:

• https://github.com/Azure/azure-cosmos-dotnet-v3/blob/a0b6d5b8399dbe504bdc7e2e9084435876b1d4bf/Microsoft.Azure.Cosmos/src/ChangeFeedProcessor/ChangeFeedProcessorBuilder.cs#L252-L257

• https://github.com/dotnet/orleans/blob/a637aaff8e1de557919fad764f2402113c8b96fb/src/Orleans.Serialization/Buffers/Adaptors/MemoryStreamBufferWriter.cs#L44

• https://github.com/dotnet/msbuild/blob/ad7e074ca3a93f4af30405c4cd24db04151d7ce8/src/Framework/BackEnd/InterningBinaryReader.cs#L107-L109

These cases are already broken for when MemoryStream is used with a user-buffer but thought it would be worth mentioning them since these types are in user-buffer mode.

[dotnet-policy-service]

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

[adamsitnik]

It sounds reasonable. We need to make sure this change is reviewed by the API Review Board (and merged into main if approved) before Preview 7 is code complete (16-Jul).

[teo-tsirpanis]

In this case, should the memories be exposed as properties?

[adamsitnik]

In this case, should the memories be exposed as properties?

I think it would be good to expose them, we could follow the pattern established by the MemoryStream class. Namely a boolean ctor parameter that controls whether the buffers can be exposed:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs

Line 92 in e6e4d8d

_exposable = publiclyVisible; // Can TryGetBuffer/GetBuffer return the array?

enabled by default:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs

Line 50 in e6e4d8d

_exposable = true;

But I would stick only to the Try pattern and ofc return Memory/ReadOnlyMemory instead of an array

[teo-tsirpanis]

If you asked me, I don't think that customizing the exposability of the underlying buffers is very useful. MemoryStream has had this capability since the very beginning of .NET Framework, so I personally wouldn't consider adding this to other stream types solely on the basis of precedent.

People can already hide a stream's backing source, by creating a subclass that forwards all virtual members to another stream.

[bartonjs]

Video

Looks good as proposed.

namespace System.IO;

-public sealed partial class WritableMemoryStream : MemoryStream
+public sealed partial class WritableMemoryStream : Stream
{
-   public override int Capacity { get; set; }
-   public override byte[] GetBuffer();
-   public override bool TryGetBuffer(out ArraySegment<byte> buffer);
-   public override byte[] ToArray();
-   public override void WriteTo(Stream stream);
}

-public sealed partial class ReadOnlyMemoryStream : MemoryStream
+public sealed partial class ReadOnlyMemoryStream : Stream
{
-   public override int Capacity { get; set; }
-   public override byte[] GetBuffer();
-   public override bool TryGetBuffer(out ArraySegment<byte> buffer);
-   public override byte[] ToArray();
-   public override void WriteTo(Stream stream);
}