davidfowl / BedrockFramework

High performance, low level networking APIs for building custom servers and clients.
MIT License
1.05k stars 153 forks source link

Make `connection.CreateReader()` accept options. #164

Closed TheVeryStarlk closed 1 year ago

TheVeryStarlk commented 1 year ago

It becomes quite redundant to always pass a IMessageReader to the ProtocolReader every time I want to read something. So I think it might be better to just provide that IMessageReader when creating the ProtocolReader once, so I can always read without passing the IMessageReader.

var reader = connection.CreateReader(new MyMessageReader());
await reader.ReadAsync();

Thoughts?

thenameless314159 commented 1 year ago

Hello, I don't think this would stay within the scope of the class since it was made non-generic in order to be usable with different kind of IMessageReader<T> at once.

The solution that you mentionned would require to introduce a new public strongly-typed generic type that may not be necessary since an extension method could be sufficient (user defined or made generic with a static generic protocol store within the bedrock framework, or even a generic interface with static abstract singleton member if we want to use new language features):

namespace MyProtocolNamespace;

public static class MyProtocolExtensions
{
    // suppose we implemented a singleton pattern in our IMessageReader<MyResult> implementation
    public static ValueTask<ProtocolReadResult<MyResult>> ReadAsync(this ProtocolReader reader, CancellationToken token) 
        => reader.ReadAsync(MyMessageReader.Instance, token);
}

In the user-defined case, you could even wrap the ProtocolReadResult<T> on your own readonly struct to reduce verbosity (given that you make the required changes to the extension method):



public readonly struct MyReceiveResult
{
    public required ProtocolReadResult<MyResult> ReadResult { get; init; }
    public MyResult => ReadResult.Message;
    public bool IsCompleted => ReadResult.IsCompleted;
    public bool IsCanceled => ReadResult.IsCanceled;
}
TheVeryStarlk commented 1 year ago

Hi, you're completely right, I missed this point. The idea of creating an extension class sounds much better. Thanks!