API Proposal: DelegatingStream

stephentoub commented 3 years ago

Background and Motivation

Stream-based implementations sometimes want to wrap an existing stream and add a little functionality on top of it, such as doing additional work as part of Dispose: https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/System.Net.WebClient/src/System/Net/WebClient.cs#L1982-L1995 or prohibiting the use of certain operations, e.g. https://github.com/dotnet/runtime/blob/1beb1c635a2961d96d56d52025ee0fa7322933bb/src/libraries/System.Net.Http/src/System/Net/Http/StreamContent.cs#L144 or tracking additional state as part of certain operations, e.g. https://github.com/dotnet/wcf/blob/d71557a6ba42bd6d4c5223e978cdafd5b535b970/src/System.Private.ServiceModel/src/System/ServiceModel/Channels/MaxMessageSizeStream.cs#L119-L127 etc.

It can be cumbersome in such cases to have to override every abstract/virtual method on Stream in order to just turn around and delegate all of those calls to the wrapped Stream instance.

Proposed API

namespace System.IO
{
    public abstract class DelegatingStream : Stream
    {
        private readonly Stream _baseStream;

        protected DelegatingStream(Stream baseStream) => _baseStream = baseStream ?? throw new ArgumentNullException(nameof(baseStream));

        public Stream BaseStream => _baseStream;

        public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, object state) => _baseStream.BeginRead(buffer, offset, count, callback, state);
        public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object state) => _baseStream.BeginWrite(buffer, offset, count, callback, state);
        public override bool CanRead => _baseStream.CanRead;
        public override bool CanWrite => _baseStream.CanWrite;
        public override bool CanSeek => _baseStream.CanSeek;
        public override bool CanTimeout => _baseStream.CanTimeout;
        public override void CopyTo(Stream destination, int bufferSize) => _baseStream.CopyTo(destination, bufferSize);
        public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => _baseStream.CopyToAsync(destination, bufferSize, cancellationToken);
        protected override void Dispose(bool disposing)
        {
            // Can't call _baseStream.Dispose(bool) as it's protected,
            // so instead if disposing is true we call _baseStream.Dispose,
            // which will in turn call _baseStream.Dispose(true).  If disposing is
            // false, then this is from a derived stream's finalizer, and it shouldn't
            // be calling _baseStream.Dispose(false) anyway; that should be left up
            // to that stream's finalizer, if it has one.
            if (disposing) _baseStream.Dispose();
        }
        public override ValueTask DisposeAsync() => _baseStream.DisposeAsync();
        public override int EndRead(IAsyncResult asyncResult) => _baseStream.EndRead(asyncResult);
        public override void EndWrite(IAsyncResult asyncResult) => _baseStream.EndWrite(asyncResult);
        public override void Flush() => _baseStream.Flush();
        public override Task FlushAsync(CancellationToken cancellationToken) => _baseStream.FlushAsync(cancellationToken);
        public override long Length => _baseStream.Length;
        public override long Position { get => _baseStream.Position; set => _baseStream.Position = value; }
        public override int Read(byte[] buffer, int offset, int count) => _baseStream.Read(buffer, offset, count);
        public override int Read(Span<byte> buffer) => _baseStream.Read(buffer);
        public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => _baseStream.ReadAsync(buffer, offset, count, cancellationToken);
        public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default) => _baseStream.ReadAsync(buffer, cancellationToken);
        public override int ReadByte() => _baseStream.ReadByte();
        public override int ReadTimeout { get => _baseStream.ReadTimeout; set => _baseStream.ReadTimeout = value; }
        public override long Seek(long offset, SeekOrigin origin) => _baseStream.Seek(offset, origin);
        public override void SetLength(long value) => _baseStream.SetLength(value);
        public override void Write(byte[] buffer, int offset, int count) => _baseStream.Write(buffer, offset, count);
        public override void Write(ReadOnlySpan<byte> buffer) => _baseStream.Write(buffer);
        public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => _baseStream.WriteAsync(buffer, offset, count, cancellationToken);
        public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default) => _baseStream.WriteAsync(buffer, cancellationToken);
        public override void WriteByte(byte value) => _baseStream.WriteByte(value);
        public override int WriteTimeout { get => _baseStream.WriteTimeout; set => _baseStream.WriteTimeout = value; }

        // Doesn't override:
        // - Close. Streams aren't supposed to override Close even though it's virtual, as Dispose() just calls Close() and Close() calls Dispose(bool),
        //   so Streams are supposed to override Dispose(bool); that Close even exists and that it's virtual are mistakes of history.
        // - CreateWaitHandle. It's obsolete, not used for anything, and as it's protected, it can't actually be delegated to.
        // - Equals / GetHashCode. Streams shouldn't be overriding these, and we generally don't want to delegate their behavior.
        // - InitializeLifetimeService / ObjectInvariant. These comes from MarshalByRefObject, which isn't relevant any more.
        // - ToString.  The base implementation prints out the type of the current type; delegating it would result in strange behavior.
    }
}

Alternative Designs

Naming: DelegatingStream vs DelegatedStream vs ...
Naming: baseStream vs innerStream vs ...
Expose BaseStream property or not

Risks

Such a type, while useful, is also potentially problematic in a couple of ways:

In some cases it could be tempting to use DelegatingStream as a base type for other public types, e.g. DeflateStream, SslStream, etc. However, doing so essentially exposes implementation detail into the hierarchy of the type; we wouldn't want to do that in any public types in dotnet/runtime, and we'd want to discourage others from doing so as well. Its intended purpose is for internal usage, e.g. creating a custom stream that's exposed publicly as Stream rather than as a concrete public type.
Various read and write operations have multiple ways they're exposed, e.g. the synchronous Read(byte[], ...) and the asynchronous ReadAsync(byte[], ...). We have a choice, either a) have DelegatingStream only override the abstract methods from the base, such that the other virtuals pick up their default implementations from the base Stream, which will in turn use the abstract methods, or b) delegate each overload to its corresponding member on the _innerStream. Option (a) is problematic, because it means that, for example, ReadAsync will by default just queue a work item to execute Read rather than employing whatever async functionality exists in the wrapped stream's ReadAsync. But option (b) also has some potential pitfalls, e.g. if a developer wants to add some functionality to every read operation, the developer has to override each relevant read overload, and if in the future we add an additional read method, and it delegates directly to the wrapped stream, that new overload will no longer pick up the derived type's intended functionality. This pokes a hole in our strategy of introducing new virtuals that by default use existing methods. This may even be a pit of failure for existing APIs. For example, if someone overrides ReadAsync to customize some functionality but then neglects to override CopyToAsync, then anyone using CopyToAsync will end up skipping the custom logic in ReadAsync, and there's no way to tell C# to delegate to the base's base implementation, which means such a dev would need to re-implement the Stream.CopyToAsync implementation.

Tornhoof commented 3 years ago

an alternative design might be a roslyn codefix, which automatically creates the delegated calls to a different Stream, if you create a Type derived from Stream.

Resharper has this (in a more generic way) functionality, especially useful for things like Stream which require a lot of boilerplate Code.

stephentoub commented 3 years ago

Nice suggestion. It addresses one of the key concerns, that you sort of want to only delegate the functionality that was present at the time you last touched your implementation, and pick up the default base implementations for anything newer that was subsequently added. And it puts the whole implementation in your face such that you can then customize it and choose to not delegate certain members. It does lead to a lot of additional boilerplate code in your project that needs maintaining... but maybe that's the lesser evil.

Tornhoof commented 3 years ago

It does lead to a lot of additional boilerplate code in your project that needs maintaining... but maybe that's the lesser evil.

Maybe sourcegenerators could be used to automatically generate the boilerplate code and practically "hide" it from the developer.

benaadams commented 3 years ago

Protected basestream rather than public?

public Stream BaseStream => _baseStream;

GSPP commented 3 years ago

A public BaseStream would break the abstraction. There would be no way to hide this property.

Would the BCL make use of this class? As noted in the issue, it cannot be used as part of a public inheritance hierarchy because it leaks implementation details. Even if just returning Stream from a method, callers can do result is DelegatingStream and break the abstraction. OK, they should not do that but they might and this will create compatibility burdens. BCL users depend on undocumented internals all the time and it's an issue for the evolution of the framework.

If it's not good enough for the BCL then it probably should not be in the BCL. I can see this being useful as a NuGet package owned by the community.

stephentoub commented 3 years ago

If it's not good enough for the BCL then it probably should not be in the BCL.

It's unlikely to be used by any public Stream types. But it'd be used heavily in tests, and it'd likely be used in a few internal stream types.

scalablecory commented 3 years ago

I would use this in a few places.

Naming consistency... not sure which is more prevalent, but I know of at least BufferedStream.UnderlyingStream and StreamWriter.BaseStream.

scalablecory commented 3 years ago

One problem I can see with this is that we would not be able to delegate any future additions to Stream. Doing so could cause a breaking change to users who simply upgrade to a newer version of .NET.

So, this would be a nice solution but we'd end up with the same problem in the future.

i.e.

Consider, a user has a custom WriteBufferingStream that is deriving from DelegatingStream. They have a WriteBufferingStream on top of NetworkStream.

And now we add that Stream.ShutdownWrites() method.

The user would probably want to either throw if there are pending writes, or flush during the ShutdownWrites call.

But, if we start delegating it, then the NetworkStream would get shutdown while there is pending data... a probably subtle bug.

stephentoub commented 3 years ago

One problem I can see

Yup. This is the second issue discussed in the risks section.

airbreather commented 1 year ago

After touching some ASP.NET (classic) code, I was curious how System.IO.Stream implemented TAP in the base class, given that APM was the go-to model for quite some time.

Sure enough, I stumbled upon these helpers, "HasOverriddenFoo" which I suppose must be special-cased in the runtime, and I immediately thought of this issue. I wonder if it might help here to extend that concept to something more general (maybe like a RuntimeHelpers.MethodIsOverridden(this, @this => @this.SomeMethod(x, y, z))) and use that at runtime to help mitigate the second risk? It seems overkill to me, but that's not my call...

Jozkee commented 1 year ago

It does lead to a lot of additional boilerplate code in your project that needs maintaining... but maybe that's the lesser evil.

Maybe sourcegenerators could be used to automatically generate the boilerplate code and practically "hide" it from the developer.

Maybe we could re-use/extend the source generator proposed in https://github.com/dotnet/runtime/issues/79839, with a different attribute [GenerateStreamFromBaseStream(string nameOfBaseStreamType)] hinting that it should generate the boilerplate in terms of a private BaseStreamType BaseStream passed to a generated ctor(BaseStreamType baseStream).

dotnet / runtime