MonitorStream 2.11.79 Breaking Change

[JRuggero]

I didn't felt happy with Breaking change introduced in 2.11.79.
So I'm propossing a way to maintain compatibility with previous versions yet introducing the new Events.
I've added new Events (WillReadArraySegment, DidReadArraySegment, WillWriteArraySegment and DidWriteArraySegment to raise when methods signatures are specifically needed, and maintain the WillRead, DidRead, WillWrite and DidWrite events original behaviour.
I think this will remove the Breaking Change.

This is the suggested code:

// Copyright (c) Andrew Arnott. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
namespace Nerdbank.Streams
{
    using System;
    using System.IO;
    using System.Runtime.InteropServices;
    using System.Threading;
    using System.Threading.Tasks;
    using Microsoft;

    /// <summary>
    /// A <see cref="Stream"/> that wraps another and reports all I/O taking place by raising events.
    /// </summary>
    public class MonitoringStream : Stream, IDisposableObservable
    {
        /// <summary>
        /// The underlying stream serving the I/O.
        /// </summary>
        private readonly Stream inner;

        /// <summary>
        /// A value indicating whether the <see cref="EndOfStream"/> event has already been raised.
        /// </summary>
        private bool endOfStreamRaised;

        /// <summary>
        /// Initializes a new instance of the <see cref="MonitoringStream"/> class.
        /// </summary>
        /// <param name="inner">The stream to wrap and monitor I/O for.</param>
        public MonitoringStream(Stream inner)
        {
            this.inner = inner ?? throw new ArgumentNullException(nameof(inner));
        }

        /// <summary>
        /// A delegate used for events that pass <see cref="Span{T}"/> around.
        /// </summary>
        /// <param name="sender">The sender.</param>
        /// <param name="span">The span.</param>
        public delegate void SpanEventHandler(object sender, Span<byte> span);

        /// <summary>
        /// A delegate used for events that pass <see cref="ReadOnlySpan{T}"/> around.
        /// </summary>
        /// <param name="sender">The sender.</param>
        /// <param name="span">The span.</param>
        public delegate void ReadOnlySpanEventHandler(object sender, ReadOnlySpan<byte> span);

        /// <summary>
        /// Occurs the first time a read method returns 0 bytes.
        /// </summary>
        public event EventHandler? EndOfStream;

        /// <summary>
        /// Occurs after <see cref="Seek(long, SeekOrigin)"/> is invoked.
        /// </summary>
        public event EventHandler<long>? DidSeek;

        /// <summary>
        /// Occurs before <see cref="Read(byte[], int, int)"/> or 
        ///               <see cref="ReadAsync(byte[], int, int, CancellationToken)"/> or
        ///               <see cref="ReadAsync(Memory{byte}, CancellationToken)"/> or
        ///               <see cref="Read(Span{byte})"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? WillRead;

        /// <summary>
        /// Occurs before <see cref="Read(byte[], int, int)"/> or <see cref="ReadAsync(byte[], int, int, CancellationToken)"/> is invoked.
        /// </summary>
        /// <remarks>
        /// The <see cref="ArraySegment{T}.Count"/> value is the maximum bytes that may be read.
        /// </remarks>
        public event EventHandler<ArraySegment<byte>>? WillReadArraySegment;

        /// <summary>
        /// Occurs after <see cref="Read(byte[], int, int)"/> or 
        ///              <see cref="ReadAsync(byte[], int, int, CancellationToken)"/> or
        ///              <see cref="ReadAsync(Memory{byte}, CancellationToken)"/> or
        ///              <see cref="Read(Span{byte})"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? DidRead;

        /// <summary>
        /// Occurs after <see cref="Read(byte[], int, int)"/> or <see cref="ReadAsync(byte[], int, int, CancellationToken)"/> is invoked.
        /// </summary>
        /// <remarks>
        /// The <see cref="ArraySegment{T}.Count"/> value is the actual bytes that were read.
        /// </remarks>
        public event EventHandler<ArraySegment<byte>>? DidReadArraySegment;

#pragma warning disable CS1574
#pragma warning disable CS0067 // Only .NET Core 2.1 raises these

        /// <summary>
        /// Occurs before <see cref="ReadAsync(Memory{byte}, CancellationToken)"/> is invoked.
        /// </summary>
        /// <remarks>
        /// The <see cref="Memory{T}.Length"/> value is the maximum bytes that may be read.
        /// </remarks>
        public event EventHandler<Memory<byte>>? WillReadMemory;

        /// <summary>
        /// Occurs after <see cref="ReadAsync(Memory{byte}, CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler<Memory<byte>>? DidReadMemory;

        /// <summary>
        /// Occurs before <see cref="Read(Span{byte})"/> is invoked.
        /// </summary>
        public event SpanEventHandler? WillReadSpan;

        /// <summary>
        /// Occurs after <see cref="Read(Span{byte})"/> is invoked.
        /// </summary>
        public event SpanEventHandler? DidReadSpan;

        /// <summary>
        /// Occurs before <see cref="Write(ReadOnlySpan{byte})"/> is invoked.
        /// </summary>
        public event ReadOnlySpanEventHandler? WillWriteSpan;

        /// <summary>
        /// Occurs after <see cref="Write(ReadOnlySpan{byte})"/> is invoked.
        /// </summary>
        public event ReadOnlySpanEventHandler? DidWriteSpan;

        /// <summary>
        /// Occurs before <see cref="WriteAsync(ReadOnlyMemory{byte}, CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler<ReadOnlyMemory<byte>>? WillWriteMemory;

        /// <summary>
        /// Occurs after <see cref="WriteAsync(ReadOnlyMemory{byte}, CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler<ReadOnlyMemory<byte>>? DidWriteMemory;

#pragma warning restore CS0067 // Only .NET Core 2.1 raises these
#pragma warning restore CS1574

        /// <summary>
        /// Occurs before <see cref="Write(byte[], int, int)"/> or
        ///               <see cref="WriteAsync(byte[], int, int, CancellationToken)"/> is invoked.
        ///               <see cref="WriteAsync(Memory{byte}, CancellationToken)"/> or
        ///               <see cref="Write(Span{byte})"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? WillWrite;

        /// <summary>
        /// Occurs before <see cref="Write(byte[], int, int)"/> or <see cref="WriteAsync(byte[], int, int, CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? WillWriteArraySegment;

        /// <summary>
        /// Occurs after <see cref="Write(byte[], int, int)"/> or 
        ///              <see cref="WriteAsync(byte[], int, int, CancellationToken)"/> or
        ///              <see cref="WriteAsync(Memory{byte}, CancellationToken)"/> or
        ///              <see cref="Write(Span{byte})"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? DidWrite;

        /// <summary>
        /// Occurs after <see cref="Write(byte[], int, int)"/> or <see cref="WriteAsync(byte[], int, int, CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler<ArraySegment<byte>>? DidWriteArraySegment;

        /// <summary>
        /// Occurs before <see cref="SetLength(long)"/> is invoked.
        /// </summary>
        public event EventHandler<long>? WillSetLength;

        /// <summary>
        /// Occurs after <see cref="SetLength(long)"/> is invoked.
        /// </summary>
        public event EventHandler<long>? DidSetLength;

        /// <summary>
        /// Occurs before <see cref="ReadByte"/> is invoked.
        /// </summary>
        public event EventHandler? WillReadByte;

        /// <summary>
        /// Occurs after <see cref="ReadByte"/> is invoked.
        /// </summary>
        public event EventHandler<int>? DidReadByte;

        /// <summary>
        /// Occurs before <see cref="WriteByte(byte)"/> is invoked.
        /// </summary>
        public event EventHandler<byte>? WillWriteByte;

        /// <summary>
        /// Occurs after <see cref="WriteByte(byte)"/> is invoked.
        /// </summary>
        public event EventHandler<byte>? DidWriteByte;

        /// <summary>
        /// Occurs after <see cref="Flush"/> or <see cref="FlushAsync(CancellationToken)"/> is invoked.
        /// </summary>
        public event EventHandler? DidFlush;

        /// <summary>
        /// Occurs when <see cref="Stream.Dispose()"/> is invoked.
        /// </summary>
        public event EventHandler? Disposed;

        /// <inheritdoc/>
        public bool IsDisposed { get; private set; }

        /// <inheritdoc/>
        public override bool CanRead => this.inner.CanRead;

        /// <inheritdoc/>
        public override bool CanSeek => this.inner.CanSeek;

        /// <inheritdoc/>
        public override bool CanWrite => this.inner.CanWrite;

        /// <inheritdoc/>
        public override long Length => this.inner.Length;

        /// <inheritdoc/>
        public override long Position
        {
            get => this.inner.Position;
            set => this.inner.Position = value;
        }

        /// <inheritdoc/>
        public override int ReadTimeout
        {
            get => this.inner.ReadTimeout;
            set => this.inner.ReadTimeout = value;
        }

        /// <inheritdoc/>
        public override int WriteTimeout
        {
            get => this.inner.WriteTimeout;
            set => this.inner.WriteTimeout = value;
        }

        /// <inheritdoc/>
        public override bool CanTimeout => this.inner.CanTimeout;

        /// <inheritdoc/>
        public override void Flush()
        {
            this.inner.Flush();
            this.DidFlush?.Invoke(this, EventArgs.Empty);
        }

        /// <inheritdoc/>
        public override async Task FlushAsync(CancellationToken cancellationToken)
        {
            await this.inner.FlushAsync(cancellationToken).ConfigureAwait(false);
            this.DidFlush?.Invoke(this, EventArgs.Empty);
        }

        /// <inheritdoc/>
        public override int Read(byte[] buffer, int offset, int count)
        {
            this.WillReadArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.WillRead?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            int bytesRead = this.inner.Read(buffer, offset, count);
            this.DidReadArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, bytesRead));
            this.DidRead?.Invoke(this, new ArraySegment<byte>(buffer, offset, bytesRead));
            this.RaiseEndOfStreamIfNecessary(bytesRead);
            return bytesRead;
        }

        /// <inheritdoc/>
        public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
        {
            this.WillReadArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.WillRead?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            int bytesRead = await this.inner.ReadAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false);
            this.DidReadArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, bytesRead));
            this.DidRead?.Invoke(this, new ArraySegment<byte>(buffer, offset, bytesRead));
            this.RaiseEndOfStreamIfNecessary(bytesRead);
            return bytesRead;
        }

#if SPAN_BUILTIN

        /// <inheritdoc/>
        public override int Read(Span<byte> buffer)
        {
            this.WillReadSpan?.Invoke(this, buffer);
            this.WillRead?.Invoke(this, buffer.ToArray());
            int bytesRead = this.inner.Read(buffer);
            //this.DidReadSpan?.Invoke(this, buffer);
            this.DidReadSpan?.Invoke(this, buffer[..bytesRead]);
            this.DidRead?.Invoke(this, buffer[..bytesRead].ToArray());
            this.RaiseEndOfStreamIfNecessary(bytesRead);
            return bytesRead;
        }

        /// <inheritdoc/>
        public override async ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
        {
            this.WillReadMemory?.Invoke(this, buffer);
            this.WillRead?.Invoke(this, buffer.ToArray());
            int bytesRead = await this.inner.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);
            //this.DidReadMemory?.Invoke(this, buffer);
            this.DidReadMemory?.Invoke(this, buffer[..bytesRead]);
            this.DidRead?.Invoke(this, buffer[..bytesRead].ToArray());
            this.RaiseEndOfStreamIfNecessary(bytesRead);
            return bytesRead;
        }

        /// <inheritdoc/>
        public override void Write(ReadOnlySpan<byte> buffer)
        {
            this.WillWriteSpan?.Invoke(this, buffer);
            this.WillWrite?.Invoke(this, buffer.ToArray());
            this.inner.Write(buffer);
            this.DidWriteSpan?.Invoke(this, buffer);
            this.DidWrite?.Invoke(this, buffer.ToArray());
        }

        /// <inheritdoc/>
        public override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
        {
            this.WillWriteMemory?.Invoke(this, buffer);
            this.WillWrite?.Invoke(this, buffer.ToArray());
            await this.inner.WriteAsync(buffer, cancellationToken).ConfigureAwait(false);
            this.DidWriteMemory?.Invoke(this, buffer);
            this.DidWrite?.Invoke(this, buffer.ToArray());
        }

#endif

        /// <inheritdoc/>
        public override long Seek(long offset, SeekOrigin origin)
        {
            long position = this.inner.Seek(offset, origin);
            this.DidSeek?.Invoke(this, position);
            return position;
        }

        /// <inheritdoc/>
        public override void SetLength(long value)
        {
            this.WillSetLength?.Invoke(this, value);
            this.inner.SetLength(value);
            this.DidSetLength?.Invoke(this, value);
        }

        /// <inheritdoc/>
        public override void Write(byte[] buffer, int offset, int count)
        {
            this.WillWriteArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.WillWrite?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.inner.Write(buffer, offset, count);
            this.DidWriteArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.DidWrite?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
        }

        /// <inheritdoc/>
        public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
        {
            this.WillWriteArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.WillWrite?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            await this.inner.WriteAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false);
            this.DidWriteArraySegment?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
            this.DidWrite?.Invoke(this, new ArraySegment<byte>(buffer, offset, count));
        }

        /// <inheritdoc/>
        public override int ReadByte()
        {
            this.WillReadByte?.Invoke(this, EventArgs.Empty);
            int result = this.inner.ReadByte();
            this.DidReadByte?.Invoke(this, result);
            this.RaiseEndOfStreamIfNecessary(result == -1 ? 0 : 1);
            return result;
        }

        /// <inheritdoc/>
        public override void WriteByte(byte value)
        {
            this.WillWriteByte?.Invoke(this, value);
            this.inner.WriteByte(value);
            this.DidWriteByte?.Invoke(this, value);
        }

        /// <inheritdoc/>
        protected override void Dispose(bool disposing)
        {
            if(disposing)
            {
                this.IsDisposed = true;
                this.inner.Dispose();
                this.Disposed?.Invoke(this, EventArgs.Empty);
            }
        }

        private void RaiseEndOfStreamIfNecessary(int bytesRead)
        {
            if(bytesRead == 0 && !this.endOfStreamRaised)
            {
                this.EndOfStream?.Invoke(this, EventArgs.Empty);
                this.endOfStreamRaised = true;
            }
        }
    }
}

It´s my first time so, I apologize if I am not respecting the etiquette.
Regards.

[AArnott]

It´s my first time so, I apologize if I am not respecting the etiquette.

You're fine. Thank you for thinking about this problem. So long as you're going to propose a well-thought out change, feel free to send a PR (maybe a draft PR if you're uncertain) so that at least I can readily see the diff that you're applying. As it is, I have to sort through the whole 'file' you've shared to find the differences.

I think you already realized an issue with this proposed fix, because you adapted to it by calling .ToArray() on the span in order to get an ArraySegment.
This isn't a non-breaking change, really, either. You're API design is reasonable (aside from the .ToArray() call), but by applying it, you're raising DidRead in more cases than it did before. It was already doubling up on some events, but now it's doubling up on all of them. So folks who had coded up handlers just right to only get each event once will now get it twice.

We're in an unfortunate here. I suppose the case could be made that as a breaking change, I ought to bump the major version number. I could still do that -- revert the fix, ship a servicing release, then ship a new major version with the fix.

I'm curious how hard this is impacting you. Is it that you can't change the calling code to adapt for some reason? Or is it that it's a pain to add handlers for all events?

What if I added a DidReadAny event that was raised for all of them, and was Span based so that no copying was ever required?

[JRuggero]

You're fine. Thank you for thinking about this problem. So long as you're going to propose a well-thought out change, feel free to send a PR (maybe a draft PR if you're uncertain) so that at least I can readily see the diff that you're applying. As it is, I have to sort through the whole 'file' you've shared to find the differences.

Done. I hope it 's right (It's my first PR) .

I think you already realized an issue with this proposed fix, because you adapted to it by calling .ToArray() on the span in order to get an ArraySegment.

Indeed.

This isn't a non-breaking change, really, either. You're API design is reasonable (aside from the .ToArray() call), but by applying it, you're raising DidRead in more cases than it did before. It was already doubling up on some events, but now it's doubling up on all of them. So folks who had coded up handlers just right to only get each event once will now get it twice.

I'm assuming that WillRead, WillWrite, WillRead and DidRead were being used to being notified of this events independently of the specific api (signature) invoked.

For new proyects, it´s obviously preferable to use the ad-hoc events (DidReadSpan, DidReadMemory or the new DidReadArraySegment, for example)

I'm curious how hard this is impacting you. Is it that you can't change the calling code to adapt for some reason? Or is it that it's a pain to add handlers for all events?

It's not hard impact for me. It was an unexpected behavioural change. I admit that usually I update dependencies without reading the Release notes. Unfortunatelly it's an extended practice.

What if I added a DidReadAny event that was raised for all of them, and was Span based so that no copying was ever required?

Great!. I would like it. But it will be usefull for new projects.
May be WillRead, DisRead, WillWrite and DidWrite should be tagget with ObsoleteAttribute. Leading developper to switch to DidReadAny

I hope my contribution is that and not only annoyance.

Thank you for your patience and sorry for the inconveniences.

[AArnott]

May be WillRead, DisRead, WillWrite and DidWrite should be tagget with ObsoleteAttribute. Leading developper to switch to DidReadAny

This, and reverting the original fix, sounds like a great way forward.

[AArnott]

BTW, as a tip for future GitHub collaboration... When you respond to a thread in a Discussion, take care to write in the "Write a reply" field (which adds a reply to that thread) instead of the "Add a comment" field, which creates a new "answer" to your own question.

[JRuggero]

Thank you!

[AArnott]

Now tracked by #856