From 419f30d9172dd5c57bcacfc329fdefb8cbc6cb88 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Emmanuel=20Andr=C3=A9?= <2341261+manandre@users.noreply.github.com> Date: Thu, 17 Jun 2021 00:41:57 +0200 Subject: [PATCH] Improve cancellation in StreamPipeReader.ReadAtLeastAsync (#53306) * Improve cancellation in StreamPipeReader.ReadAtLeastAsync * Introduce task variable Co-authored-by: Stephen Toub * Fix tests * Expect TaskCanceledException on canceled token Co-authored-by: Stephen Toub --- .../System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs | 10 ++++++++++ .../src/System/IO/Pipelines/StreamPipeReader.cs | 10 ++++++++-- .../System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs | 2 +- .../tests/PipeReaderReadAtLeastAsyncTests.cs | 3 ++- .../System.IO.Pipelines/tests/ReadAsyncCancellationTests.cs | 8 +++----- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs index 14f0092..bea6289 100644 --- a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs +++ b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs @@ -664,6 +664,11 @@ namespace System.IO.Pipelines ThrowHelper.ThrowInvalidOperationException_NoReadingAllowed(); } + if (token.IsCancellationRequested) + { + return new ValueTask(Task.FromCanceled(token)); + } + CompletionData completionData = default; ValueTask result; lock (SyncObj) @@ -715,6 +720,11 @@ namespace System.IO.Pipelines ThrowHelper.ThrowInvalidOperationException_NoReadingAllowed(); } + if (token.IsCancellationRequested) + { + return new ValueTask(Task.FromCanceled(token)); + } + ValueTask result; lock (SyncObj) { diff --git a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs index ccafce2..7b33539 100644 --- a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs +++ b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs @@ -194,7 +194,10 @@ namespace System.IO.Pipelines // TODO ReadyAsync needs to throw if there are overlapping reads. ThrowIfCompleted(); - cancellationToken.ThrowIfCancellationRequested(); + if (cancellationToken.IsCancellationRequested) + { + return new ValueTask(Task.FromCanceled(cancellationToken)); + } // PERF: store InternalTokenSource locally to avoid querying it twice (which acquires a lock) CancellationTokenSource tokenSource = InternalTokenSource; @@ -273,7 +276,10 @@ namespace System.IO.Pipelines // TODO ReadyAsync needs to throw if there are overlapping reads. ThrowIfCompleted(); - cancellationToken.ThrowIfCancellationRequested(); + if (cancellationToken.IsCancellationRequested) + { + return new ValueTask(Task.FromCanceled(cancellationToken)); + } // PERF: store InternalTokenSource locally to avoid querying it twice (which acquires a lock) CancellationTokenSource tokenSource = InternalTokenSource; diff --git a/src/libraries/System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs b/src/libraries/System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs index 2076d4e..e58e5af 100644 --- a/src/libraries/System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs +++ b/src/libraries/System.IO.Pipelines/tests/PipeReaderCopyToAsyncTests.cs @@ -178,7 +178,7 @@ namespace System.IO.Pipelines.Tests Pipe.Writer.WriteEmpty(10); await Pipe.Writer.FlushAsync(); - await Assert.ThrowsAsync(() => task); + await Assert.ThrowsAsync(() => task); } [Fact] diff --git a/src/libraries/System.IO.Pipelines/tests/PipeReaderReadAtLeastAsyncTests.cs b/src/libraries/System.IO.Pipelines/tests/PipeReaderReadAtLeastAsyncTests.cs index c2aaea3..c12d3b8 100644 --- a/src/libraries/System.IO.Pipelines/tests/PipeReaderReadAtLeastAsyncTests.cs +++ b/src/libraries/System.IO.Pipelines/tests/PipeReaderReadAtLeastAsyncTests.cs @@ -141,7 +141,8 @@ namespace System.IO.Pipelines.Tests [Fact] public Task ReadAtLeastAsyncThrowsIfPassedCanceledCancellationToken() { - return Assert.ThrowsAsync(async () => await PipeReader.ReadAtLeastAsync(0, new CancellationToken(true))); + ValueTask task = PipeReader.ReadAtLeastAsync(0, new CancellationToken(canceled: true)); + return Assert.ThrowsAsync(async () => await task); } [Fact] diff --git a/src/libraries/System.IO.Pipelines/tests/ReadAsyncCancellationTests.cs b/src/libraries/System.IO.Pipelines/tests/ReadAsyncCancellationTests.cs index 0e9fe23..8422de3 100644 --- a/src/libraries/System.IO.Pipelines/tests/ReadAsyncCancellationTests.cs +++ b/src/libraries/System.IO.Pipelines/tests/ReadAsyncCancellationTests.cs @@ -362,12 +362,10 @@ namespace System.IO.Pipelines.Tests } [Fact] - public void ReadAsyncThrowsIfPassedCanceledCancellationToken() + public Task ReadAsyncThrowsIfPassedCanceledCancellationToken() { - var cancellationTokenSource = new CancellationTokenSource(); - cancellationTokenSource.Cancel(); - - Assert.Throws(() => Pipe.Reader.ReadAsync(cancellationTokenSource.Token)); + ValueTask task = Pipe.Reader.ReadAsync(new CancellationToken(canceled: true)); + return Assert.ThrowsAsync(async () => await task); } [Fact] -- 2.7.4