Add async overloads to BinaryReader/Writer

[davidfowl]

Because I'd love to use it to read/write things, asynchronously from the network.

Edit: API Proposal can be found here.

[ianhays]

Because I'd love to use it to read/write things, asynchronously from the network.

To clarify, you mean every write/read method yes? So if you create a BinaryReader/Writer around a stream you can call binary.WriteAsync(sbyte value) instead of being forced into the synchronous binary.Write(sbyte value)?

To do that we'd need to call the underlying stream's WriteAsync(byte[], int, int) instead of Write(byte[], int, int). We'd also probably want to lock the buffer used for small writes/reads and unlock it when the underlying call completes.

Then I suppose we'd want to consider adding cancellation token overloads. So to write a double you could do either:

• Write(double value)
• WriteAsync(double value)
• WriteAsync(double value, CancellationToken token)

[davidfowl]

To clarify, you mean every write/read method yes?

Yep!

So if you create a BinaryReader/Writer around a stream you can call binary.WriteAsync(sbyte value) instead of being forced into the synchronous binary.Write(sbyte value)?

Yep!

[karelz]

We need API proposal.

[JonHanna]

We also need a plan for how it should behave in the case of the async overload being called on a derived class.

[ronnieoverby]

https://github.com/ronnieoverby/AsyncBinaryReaderWriter

[markusschaber]

@ronnieoverby Nice code. Unfortunately, there's no license information anywhere, so the project cannot be used legally anywhere :-(

[ronnieoverby]

@markusschaber DOH! Thanks. Added MIT LICENSE.

[markusschaber]

@ronnieoverby Thanks a lot! :-)

[h82258652]

@ronnieoverby Great job! and is it support .net core?

[ronnieoverby]

@h82258652 yes, it's a .NET standard library, so core/framework support.

[tstivers1990]

@ronnieoverby Thank you.

[steji113]

Is this something that would still be beneficial for the BinaryReader / BinaryWriter? Or would it be more prudent to introduce a more efficient type akin to what was proposed in dotnet/corefx#24180? I think we have a lot of the appropriate building blocks for efficiently writing to buffers, but there is still some assembly required, notably when working with Spans as mentioned in the linked issue.

/cc: @davidfowl / @Drawaes

[manandre]

Async overloads to BinaryWriter are required to add IAsyncDisposable support to ZipArchive (#42130). If it can help, I propose to work on the API proposal. ✍

[manandre]

API proposal

Add async overloads to BinaryReader and BinaryWriter classes.
Also useful to enable async features in other dotnet libraries relying on BinaryReader/Writer like System.IO.Compression.ZipArchive (#42130)

BinaryReader

namespace System.IO
{
    public partial class BinaryReader : System.IDisposable
    {
        public virtual ValueTask<int> ReadAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<int> ReadAsync(byte[] buffer, int index, int count, CancellationToken cancellationToken = default);
        public virtual ValueTask<int> ReadAsync(char[] buffer, int index, int count, CancellationToken cancellationToken = default);
        public virtual ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default);
        public virtual ValueTask<int> ReadAsync(Memory<char> buffer, CancellationToken cancellationToken = default);

        public virtual ValueTask<bool> ReadBooleanAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<byte> ReadByteAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<byte[]> ReadBytesAsync(int count, CancellationToken cancellationToken = default);
        public virtual ValueTask<char> ReadCharAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<char[]> ReadCharsAsync(int count, CancellationToken cancellationToken = default);
        public virtual ValueTask<decimal> ReadDecimalAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<double> ReadDoubleAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<short> ReadInt16Async(CancellationToken cancellationToken = default);
        public virtual ValueTask<int> ReadInt32Async(CancellationToken cancellationToken = default);
        public virtual ValueTask<long> ReadInt64Async(CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask<sbyte> ReadSByteAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<float> ReadSingleAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask<string> ReadStringAsync(CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask<ushort> ReadUInt16Async(CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask<uint> ReadUInt32Async(CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask<ulong> ReadUInt64Async(CancellationToken cancellationToken = default);
        public virtual ValueTask<int> PeekCharAsync(CancellationToken cancellationToken = default);

        protected internal ValueTask<int> Read7BitEncodedIntAsync(CancellationToken cancellationToken = default);
        protected virtual ValueTask FillBufferAsync(int numBytes, CancellationToken cancellationToken = default);
    }
}

BinaryWriter

namespace System.IO
{
    public partial class BinaryWriter : IAsyncDisposable, IDisposable
    {
        public virtual ValueTask FlushAsync(CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(bool value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(byte value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(byte[] buffer, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(byte[] buffer, int index, int count, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(char ch, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(char[] chars, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(char[] chars, int index, int count, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(decimal value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(double value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(short value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(int value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(long value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(ReadOnlyMemory<char> chars, CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask WriteAsync(sbyte value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(float value, CancellationToken cancellationToken = default);
        public virtual ValueTask WriteAsync(string value, CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask WriteAsync(ushort value, CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask WriteAsync(uint value, CancellationToken cancellationToken = default);
        [CLSCompliantAttribute(false)]
        public virtual ValueTask WriteAsync(ulong value, CancellationToken cancellationToken = default);
        
        protected void Write7BitEncodedInt(int value) { }
        protected ValueTask Write7BitEncodedIntAsync(int value, CancellationToken cancellationToken = default);
    }
}

Stream

Associated byte-level async read/write methods for the Stream API.
Also useful for implementation of async methods of the BinaryReader/Writer classes

public abstract partial class Stream : MarshalByRefObject, IAsyncDisposable, IDisposable
{
    public virtual ValueTask<int> ReadByteAsync(CancellationToken cancellationToken = default);
    public virtual ValueTask WriteByteAsync(byte value, CancellationToken cancellationToken = default);
}

[svick]

public virtual ValueTask<int> ReadAsync(byte[] buffer, int index, int count, CancellationToken cancellationToken = default);
public virtual ValueTask<int> ReadAsync(char[] buffer, int index, int count, CancellationToken cancellationToken = default);
public virtual ValueTask WriteAsync(byte[] buffer, CancellationToken cancellationToken = default);
public virtual ValueTask WriteAsync(byte[] buffer, int index, int count, CancellationToken cancellationToken = default);
public virtual ValueTask WriteAsync(char[] chars, CancellationToken cancellationToken = default);
public virtual ValueTask WriteAsync(char[] chars, int index, int count, CancellationToken cancellationToken = default);

Is there a reason to have these array-based overloads, when Memory-based overloads can be used instead?

[manandre]

Yes, to avoid the performance and memory costs of creating Memory objects where it is not needed.

[svick]

@manandre What costs? Memory<T> is a struct, which does not allocate any heap memory. And while the relevant Memory<T> constructors do some input validation, I doubt that would cause significant performance overhead, especially considering that the array-based overloads also have to perform input validation.

[manandre]

@svick I agree, but in the Stream API context, the array-based methods are generally the legacy and more optimized ones, while the Memory-based ones are only implemented as a kind of extension methods, like the default implementation in stream.cs
My question would then be: Is there a reason to force API clients to have only access to Memory-based methods of the Stream API?

[svick]

@manandre

in the Stream API context, the array-based methods are generally the legacy and more optimized ones, while the Memory-based ones are only implemented as a kind of extension methods, like the default implementation in stream.cs

We're talking about brand new APIs, so I think legacy implementations are irrelevant.

My question would then be: Is there a reason to force API clients to have only access to Memory-based methods of the Stream API?

Yes: it makes the API simpler. And it shouldn't hurt API clients much, since T[] is implicitly convertible to Memory<T>, so code like await reader.ReadAsync(array) would still work.

[manandre]

in the Stream API context, the array-based methods are generally the legacy and more optimized ones, while the Memory-based ones are only implemented as a kind of extension methods, like the default implementation in stream.cs

We're talking about brand new APIs, so I think legacy implementations are irrelevant.

Here I do not talk of the new BinaryReader/Writer API, but the existing Stream one, i.e the API of the output stream given as parameter at construction time. And I make the strong hypothesis that array-based methods (resp. Memory-based ones) of the BinaryReader/Writer API will call by default their homologue from the Stream API.

My question would then be: Is there a reason to force API clients to have only access to Memory-based methods of the Stream API?

Yes: it makes the API simpler. And it shouldn't hurt API clients much, since T[] is implicitly convertible to Memory, so code like await reader.ReadAsync(array) would still work.

I consider that using Memory-based methods must be an explicit choice of the API client.

[benaadams]

Yes: it makes the API simpler. And it shouldn't hurt API clients much, since T[] is implicitly convertible to Memory<T>

Unless the underlying Stream explicitly has overrides for the Memory<T> overloads then Stream will either deconstruct the Memory<T> back to an array, offset, count and then call the other virtual array based ReadAsync or if the Memory is not array backed, rent an array from the arraypool; copy the memory there and then use that array to call the other virtual array based ReadAsync.

If the Stream is not Memory aware, its very inefficient due to the api evolution.

As a user of the api you'd probably be be using Memory with Memory aware streams; but if you are using arrays, then there is a significant chance you are also using it with Streams that are not Memory aware,

[stephentoub]

If the Stream is not Memory aware, its very inefficient due to the api evolution.

If the Memory<T> is backed by an array, the overhead is just fishing out the array from the Memory<T> and calling the other overload; that's not particularly expensive. And if the Memory<T> is backed by something other than an array, well, you couldn't have used the existing overload anyway.

if you are using arrays, then there is a significant chance you are also using it with Streams that are not Memory aware,

Really? All of the Streams in .NET Core that matter override these (if you find one that doesn't in a case where perf matters, please open an issue). And I'd be willing to bet that the majority of Stream uses are of these built-in stream types.

[manandre]

@stephentoub So, do you also recommend to skip the array-based overloads from the API proposal?

[manandre]

@carlossanlop @davidfowl Can we get this API proposal reviewed as it and let reviewers decide whether to keep or remove these array-based overloads?

[Mart-Bogdan]

While we are waiting for a new API, I've found NuGet Package with this functionality: https://github.com/ronnieoverby/AsyncBinaryReaderWriter

Would check how good it is in the following days. Looks promising.

[ronnieoverby]

While we are waiting for a new API, I've found NuGet Package with this functionality: https://github.com/ronnieoverby/AsyncBinaryReaderWriter

Would check how good it is in the following days. Looks promising.

Just let me know if you find any issues.

[0xced]

@carlossanlop Is there something holding this issue in the api-needs-work state. Isn't it ready for review with @manandre proposal? It is currently blocking Add async dispose support to ZipArchive (#1560).

[lanwin]

Wouldn't it be a better idea to discard the binary reader and implement its methods as extension method on Stream?

[felipepessoto]

@lanwin I think it is not possible, because BinaryReader/Writer holds some state. We could pass the parameters to every call to the static method (for the Writer only. For the Reader I think it is not possible since it contains buffers), but I think that is inconvenient, and BinaryReader/Writer also allows inherited classes to override its methods, what wouldn't be possible with extension methods.

[geekox86]

Greetings all. What is the technical hold up on this feature request?