dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

System.IO.Pipelines inifinite wait PipeReader.ReadAtLeastAsync(Int32, CancellationToken) #66645

Closed eagleoriginal closed 2 years ago

eagleoriginal commented 2 years ago

Description

Infinite wait. OperationCanceledException was not throwed as expected if only part of required length has been readen from stream. In example actually has 12 bytes for read, but syntetical code ReadAtLeastAsync wait for 30 bytes. So it leads to infinite wait ignoring cancelation token.

Reproduction Steps

using System.IO.Pipelines;
var cts = new CancellationTokenSource(3000);
var pipe = new Pipe(PipeOptions.Default);
var bytes = new byte[12];
await pipe.Writer.WriteAsync(bytes).ConfigureAwait(false);

var result = await pipe.Reader.ReadAtLeastAsync(30, cts.Token).ConfigureAwait(false);
pipe.Reader.AdvanceTo(result.Buffer.GetPosition(30));
Console.WriteLine("Stage 1");

Expected behavior

Expected getting: System.OperationCanceledException: The operation was canceled.

Actual behavior

Infinitie wait.....

Known Workarounds

Using this code from Microsoft sources avoid infinite wait:

protected static async ValueTask<ReadResult> ReadAtLeastAsyncCore(PipeReader reader, int minimumSize, CancellationToken cancellationToken)
        {
            while (true)
            {
                var result = await reader.ReadAsync(cancellationToken).ConfigureAwait(false);
                var buffer = result.Buffer;

                if (buffer.Length >= minimumSize || result.IsCompleted || result.IsCanceled)
                {
                    return result;
                }

                // Keep buffering until we get more data
                reader.AdvanceTo(buffer.Start, buffer.End);
            }
        }

Configuration

.net 6.0 System.IO.Pipelines v6.0.2

adityamandaleeka commented 2 years ago

cc @davidfowl

adityamandaleeka commented 2 years ago

We'll need to repro this and investigate.

davidfowl commented 2 years ago

I'll investigate

BrennanConroy commented 2 years ago

Looks like this is because ReadAtLeastAsync uses the token in BeginOperation which checks for IsCompleted which is true as you have a successful WriteAsync before. After that it doesn't use the token again when it waits for more data. https://github.com/dotnet/runtime/blob/a1f1ab8bc4ad933c36e4dc30172d7dd659ef1012/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeAwaitable.cs#L51

davidfowl commented 2 years ago

Yep, nice catch @BrennanConroy! We should fix and potentially patch this one.