Synchronous StreamPipeReader

[jods4]

I just found this library and it's fantastic when running on pre-std 2.1 or migrating old code! 😍

In that spirit, would you consider adding StreamPipeReader.Read, a non-async version of ReadAsync?

It would significantly help migrating old code and even boost performance sometimes, here's why:

1. Async trickles up the complete call tree, unless you stop it with .Wait() or .Result.
   This former makes rewriting existing code quite hard; the latter is considered a bad practice, but it's what many end up doing.
2. Sometimes your stream doesn't have a true async implementation, e.g. Oracle ODP.NET.
   You end up doing async-over-sync, which is not good either. (Combine with 1: sync-over-async-over-sync)
   This just hurts perf.
3. You may ask: why even use PipeReader then?
   Because it still offers great improvements over the Stream API: how it manages buffers (pooling, zero-copying, etc.), how it lets you "rewind" if data is incomplete.
   If we give up on PipeReader we would end up having to do the buffer management ourselves, which is not the most pleasant idea.
4. I don't think it would be hard at all.
   Basically copy and rewrite ReadAsync with a sync call and every irrelevant bit removed, e.g. CancellationToken.

Only issue I see would be how to expose the API, as the StreamPipeReader class is currently internal...

Thoughts?

[AArnott]

I agree adding a Read method would be easy.
Making StreamPipeReader public is not particularly attractive considering it's already marked as Obsolete since .NET now has a method to adapt a Stream to a PipeReader.
I'll leave the issue active to let it simmer in my mind a bit though.

[jods4]

I thought about it a bit.
What do you think of this?

public abstract class SyncPipeReader : PipeReader
{
  public abstract ReadResult Read();
}

internal sealed class StreamPipeReader : SyncPipeReader
{
  // Existing class + new Read() implementation
}

public static class PipeExtensions
{
  public SyncPipeReader UseStrictPipeReader(this Stream stream, int sizeHint)
  {
    return new StreamPipeReader(..);
  }
}

[AArnott]

It isn't exposing a concrete class that concerns me. Nerdbank.Streams has already made plenty of them public. It's that .NET now has an API to own the Stream-to-PipeReader operation so I was eager to get out of it to continue focus on adding value to .NET rather than replacing it.

I guess another issue is that UseStrictPipeReader is already public and returns PipeReader. Changing the return type would be a binary breaking change to existing users.

Maybe what we could do is just make the existing class public, with the addition you're asking for, remove the Obsolete attribute, and call it good. People who want to use it can instantiate it directly rather than going through the extension method whose return type is a PipeReader (or they can dare downcast its result, but I wouldn't recommend that).

[jods4]

If you don't want to change the return type, you could add UseSyncPipeReader?

public SyncPipeReader UseSyncPipeReader(..)
{
  // .., current implementation
}

public PipeReader UseStrictPipeReader(..) => UseSyncPipeReader(..);

That way the existing public API has not changed at all?