-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Advance call to protocol reader #12
Conversation
@@ -87,89 +89,89 @@ public ProtocolReader(ConnectionContext connection, IProtocolReader<TReadMessage | |||
|
|||
public ConnectionContext Connection { get; } | |||
|
|||
public async ValueTask<TReadMessage> ReadAsync(CancellationToken cancellationToken = default) | |||
public async ValueTask<ReadResult<TReadMessage>> ReadAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Nested structs aren't great but there's nothing we can do about that.
- Should this return the previously read value if Advance isn't called?
|
||
public readonly struct ReadResult<TMessage> | ||
{ | ||
public ReadResult(TMessage message, bool isCanceled, bool isCompleted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this causes TMessage message
(that's a struct in the example) being copied into ReadResult?
should this be a ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. I'm not sure the struct can be readonly and return a ref to the TMessage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im okay with the api change in general. but when applying this to my mqtt playground i found an error.
in case TryParseMessage fails because the message is incomplete the next call to readasync throws because you did not call advance and the pipe thinks there is a read operation in progress.
@JanEggers might be time to add tests 😄 |
- Added some basic tests
IProtocolReader<TMessage>
to avoid copying incomingbyte[]
directly, we add anAdvance()
method to the reader to allow the caller to decide when memory is recycled.ReadResult<TMessage>
to propagate cancellation and completion information.cc @JanEggers