davidfowl / BedrockFramework

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

Issue with ProtocolReader.ReadAsync and CancellationTokens #115

Closed Aenge closed 3 years ago

Aenge commented 3 years ago

I'm writing a server that uses a heartbeat packet. After 10s of inactivity on the socket, the server sends a PING to the client. If using a ProtocolReader, when the PONG comes in, an exception is thrown.

while (true) {
    try
    {
        //Wait for a packet
        ProtocolReadResult<ReadOnlyMemory<byte>> result = await     
           protocolReader.ReadAsync(authProtocol, new CancellationTokenSource(10000).Token);

        //Check for FIN
        if (result.IsCompleted || result.IsCanceled)
            return;

        protocolReader.Advance();    
    }
    catch (OperationCanceledException exception) 
    {
        SendPing(connection);
    }
}

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'startIndex')
   at System.ThrowHelper.ThrowArgumentValidationException[T](ReadOnlySequenceSegment`1 startSegment, Int32 startIndex, ReadOnlySequenceSegment`1 endSegment)
   at System.Buffers.ReadOnlySequence`1..ctor(ReadOnlySequenceSegment`1 startSegment, Int32 startIndex, ReadOnlySequenceSegment`1 endSegment, Int32 endIndex)
   at System.IO.Pipelines.StreamPipeReader.GetCurrentReadOnlySequence()
   at System.IO.Pipelines.StreamPipeReader.ReadAsync(CancellationToken cancellationToken)
   at Bedrock.Framework.Protocols.ProtocolReader.ContinueDoAsyncRead[TReadMessage](ValueTask`1 readTask, Nullable`1 maximumMessageSize, IMessageReader`1 reader, CancellationToken cancellationToken)
   at Auth.AuthHandler.OnConnectedAsync(ConnectionContext connection) in C:\Users\redacted\AuthHandler.cs:line 44

Line 44 is the protocolReader.ReadAsync instruction exactly as shown above. For testing, I tried sending the PING outside of the exception handler, and was able to receive the PONG with no issues. It seems to be related to the operation cancellation. Also, if I replace the protocol reader with a standard pipe reader, this issue does not occur.

davidfowl commented 3 years ago

Do you have a repro project?

Aenge commented 3 years ago

Do you have a repro project?

I just threw this together: https://github.com/Aenge/ReadAsyncRepro

Note the exception shown out of the box will be different than the one in the OP; there is a note regarding that in CustomProtocol.cs

davidfowl commented 3 years ago

This call is illegal https://github.com/Aenge/ReadAsyncRepro/blob/0800d1484d42b83e391c6ff4fedeb115a6126034/ReadAsyncRepro/CustomProtocol.cs#L26

You can't use this unless you were the one that created the ReadOnlySequence and you know what the underlying representation is.

Aenge commented 3 years ago

This call is illegal https://github.com/Aenge/ReadAsyncRepro/blob/0800d1484d42b83e391c6ff4fedeb115a6126034/ReadAsyncRepro/CustomProtocol.cs#L26

You can't use this unless you were the one that created the ReadOnlySequence and you know what the underlying representation is.

I've tried without that call as well and it still throws. I've updated the repro project. https://github.com/Aenge/ReadAsyncRepro/commit/f904d6e88d90798fdf3295f728adc6f9ed5e77ad

davidfowl commented 3 years ago

I can reproduce the issue! Thanks

davidfowl commented 3 years ago

I'll take a deeper look at this today.

Aenge commented 3 years ago

Thanks for the fast response. Our cancellations are working as expected now. 👍

davidfowl commented 3 years ago

Thanks for using the library!