Deprecate PipeWriter.OnReaderCompleted

[davidfowl]

PipeWriter.OnReaderCompleted is basically impossible to implement on anything but the default Pipe implementation. When there's no reader on the other side consuming the data (like the StreamPipeReader), this method can't do anything but noop. It should have been put on the concrete implementation but not on the abstract class.

[terrajobst]

Video

@davidfowl the method is abstract. Does that mean we'll make it virtual and a no-nop? Also, what does deprecation mean to you, marking in Obsolete? What's the message gonna say?

[terrajobst]

Video

Decision:

• Make the implementation virtual.
• Make the implementation a no-op
• Mark the API as [Obsolete]
• Apply the same to PipeWriter.OnWriterCompleted

@davidfowl, what should the message be for PipeReader.OnReaderCompleted and PipeWriter.OnWriterCompleted?

[davidfowl]

I'm going to make these virtual for now with a noop. There's no real replacement for the events so the obsolete message has to say something like "These will be removed in a future release" without a suggestion on the replacement. Is that kewl?

[AArnott]

Whoa. Without these callbacks how can someone who has a PipeReader know when no more data will be forthcoming? Is PipeReader.ReadAsync's ReadResult.IsCompleted the only way?

[davidfowl]

Right, only the consumer can know (which can be problematic I agree). The reasoning for doing this is:

• Both callbacks don't have a way to remove subscriptions
• PipeWriter.OnReaderCompleted only make sense for Pipe
• PipeReader.OnWriterCompleted can be implemented but there's no writer, it's inappropriately named.

[AArnott]

only the consumer can know (which can be problematic I agree)

I'm hitting this now in trying to remove my own uses of this API. I have a method that allows reading from a Stream via PipeReader. It's implemented by creating a Pipe and reading from the stream and writing to the Pipe.Writer in a loop. The loop exits when either the stream ends or Pipe.Reader quits listening (which we determine via Pipe.Writer.OnReaderCompleted).

What should I do instead? If I just keep writing I'll fill up the Pipe's buffer and it will be an async hang.

[AArnott]

My scenario above becomes more interesting when combined with the other API which allows writing to a Stream with PipeWriter. The two combined can be accessed via my Stream.UsePipe() extension method. Once someone calls that, today they can just complete the reader and writer and we detect that via these deprecated callbacks and we call Stream.Dispose(). Without the deprecated APIs, we'll have no way to know when we should dispose the stream and we'll have to put that onus back on the user who didn't want to deal with the stream at all.

[mgravell]

@AArnott talking to @davidfowl about that offline, the way he's suggested I try it is by decorating the exposed reader/writer, and having my extra handling in the public override void Complete(...) - something like:

        private sealed class WrappedWriter : PipeWriter
        {
            private readonly PipeWriter _writer;
            private readonly SocketConnection _connection;

            public WrappedWriter(PipeWriter writer, SocketConnection connection)
            {
                _writer = writer;
                _connection = connection;
            }
            public override void Complete(Exception exception = null)
            {
                _connection.WriterCompleted(exception); // <=== do my shutdown etc
                _writer.Complete(exception);
            }
            // not shown: all the forwards
        }

[mgravell]

seems viable - there's my experimental commit ^^^