diff --git a/src/Compilers/Test/Core/Assert/AssertEx.cs b/src/Compilers/Test/Core/Assert/AssertEx.cs index 365df6758ee91..a23908c2a41a9 100644 --- a/src/Compilers/Test/Core/Assert/AssertEx.cs +++ b/src/Compilers/Test/Core/Assert/AssertEx.cs @@ -551,7 +551,7 @@ public static void Fail(string format, params object[] args) throw new Xunit.Sdk.XunitException(string.Format(format, args)); } - public static void NotNull(T @object, string message = null) + public static void NotNull([NotNull] T @object, string message = null) { Assert.False(AssertEqualityComparer.IsNull(@object), message); } diff --git a/src/EditorFeatures/Test/Utilities/AsyncLazyTests.cs b/src/EditorFeatures/Test/Utilities/AsyncLazyTests.cs deleted file mode 100644 index 30c2d48f992ed..0000000000000 --- a/src/EditorFeatures/Test/Utilities/AsyncLazyTests.cs +++ /dev/null @@ -1,104 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -#nullable disable - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Test.Utilities; -using Roslyn.Test.Utilities; -using Roslyn.Utilities; -using Xunit; - -namespace Microsoft.CodeAnalysis.UnitTests -{ - [Trait(Traits.Feature, Traits.Features.AsyncLazy)] - public class AsyncLazyTests - { - [Fact] - public void CancellationDuringInlinedComputationFromGetValueStillCachesResult() - { - CancellationDuringInlinedComputationFromGetValueOrGetValueAsyncStillCachesResultCore((lazy, ct) => lazy.GetValue(ct), includeSynchronousComputation: true); - CancellationDuringInlinedComputationFromGetValueOrGetValueAsyncStillCachesResultCore((lazy, ct) => lazy.GetValue(ct), includeSynchronousComputation: false); - } - - private static void CancellationDuringInlinedComputationFromGetValueOrGetValueAsyncStillCachesResultCore(Func, CancellationToken, object> doGetValue, bool includeSynchronousComputation) - { - var computations = 0; - var requestCancellationTokenSource = new CancellationTokenSource(); - object createdObject = null; - - Func synchronousComputation = c => - { - Interlocked.Increment(ref computations); - - // We do not want to ever use the cancellation token that we are passed to this - // computation. Rather, we will ignore it but cancel any request that is - // outstanding. - requestCancellationTokenSource.Cancel(); - - createdObject = new object(); - return createdObject; - }; - - var lazy = new AsyncLazy( - c => Task.FromResult(synchronousComputation(c)), - includeSynchronousComputation ? synchronousComputation : null); - - var thrownException = Assert.Throws(() => - { - // Do a first request. Even though we will get a cancellation during the evaluation, - // since we handed a result back, that result must be cached. - doGetValue(lazy, requestCancellationTokenSource.Token); - }); - - // And a second request. We'll let this one complete normally. - var secondRequestResult = doGetValue(lazy, CancellationToken.None); - - // We should have gotten the same cached result, and we should have only computed once. - Assert.Same(createdObject, secondRequestResult); - Assert.Equal(1, computations); - } - - [Fact] - public void SynchronousRequestShouldCacheValueWithAsynchronousComputeFunction() - { - var lazy = new AsyncLazy(c => Task.FromResult(new object())); - - var firstRequestResult = lazy.GetValue(CancellationToken.None); - var secondRequestResult = lazy.GetValue(CancellationToken.None); - - Assert.Same(secondRequestResult, firstRequestResult); - } - - [Theory] - [CombinatorialData] - public async Task AwaitingProducesCorrectException(bool producerAsync, bool consumerAsync) - { - var exception = new ArgumentException(); - Func> asynchronousComputeFunction = - async cancellationToken => - { - await Task.Yield(); - throw exception; - }; - Func synchronousComputeFunction = - cancellationToken => - { - throw exception; - }; - - var lazy = producerAsync - ? new AsyncLazy(asynchronousComputeFunction) - : new AsyncLazy(asynchronousComputeFunction, synchronousComputeFunction); - - var actual = consumerAsync - ? await Assert.ThrowsAsync(async () => await lazy.GetValueAsync(CancellationToken.None)) - : Assert.Throws(() => lazy.GetValue(CancellationToken.None)); - - Assert.Same(exception, actual); - } - } -} diff --git a/src/Workspaces/CoreTest/UtilityTest/AsyncLazyTests.cs b/src/Workspaces/CoreTest/UtilityTest/AsyncLazyTests.cs index 0e811b5f95715..836e141ebc277 100644 --- a/src/Workspaces/CoreTest/UtilityTest/AsyncLazyTests.cs +++ b/src/Workspaces/CoreTest/UtilityTest/AsyncLazyTests.cs @@ -2,11 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - using System; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CodeAnalysis.Test.Utilities; using Roslyn.Test.Utilities; using Roslyn.Utilities; @@ -29,19 +28,11 @@ public void GetValueAsyncReturnsCompletedTaskIfAsyncComputationCompletesImmediat Assert.Equal(5, t.Result); } - [Fact] - public void SynchronousContinuationsDoNotRunWithinGetValueCallForCompletedTask() - => SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskStatus.RanToCompletion); - - [Fact] - public void SynchronousContinuationsDoNotRunWithinGetValueCallForCancelledTask() - => SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskStatus.Canceled); - - [Fact] - public void SynchronousContinuationsDoNotRunWithinGetValueCallForFaultedTask() - => SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskStatus.Faulted); - - private static void SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskStatus expectedTaskStatus) + [Theory] + [InlineData(TaskStatus.RanToCompletion)] + [InlineData(TaskStatus.Canceled)] + [InlineData(TaskStatus.Faulted)] + public void SynchronousContinuationsDoNotRunWithinGetValueCall(TaskStatus expectedTaskStatus) { var synchronousComputationStartedEvent = new ManualResetEvent(initialState: false); var synchronousComputationShouldCompleteEvent = new ManualResetEvent(initialState: false); @@ -71,7 +62,7 @@ private static void SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskS }); // Second, start a synchronous request. While we are in the GetValue, we will record which thread is being occupied by the request - Thread synchronousRequestThread = null; + Thread? synchronousRequestThread = null; Task.Factory.StartNew(() => { try @@ -116,8 +107,9 @@ private static void SynchronousContinuationsDoNotRunWithinGetValueCallCore(TaskS // And wait for our continuation to run asyncContinuation.Wait(); + AssertEx.NotNull(asyncContinuationRanSynchronously, "The continuation never ran."); Assert.False(asyncContinuationRanSynchronously.Value, "The continuation did not run asynchronously."); - Assert.Equal(expectedTaskStatus, observedAntecedentTaskStatus.Value); + Assert.Equal(expectedTaskStatus, observedAntecedentTaskStatus!.Value); } [Fact] @@ -152,7 +144,7 @@ private static void GetValueOrGetValueAsyncThrowsCorrectExceptionDuringCancellat var computeFunctionRunning = new ManualResetEvent(initialState: false); AsyncLazy lazy; - Func synchronousComputation = null; + Func? synchronousComputation = null; if (includeSynchronousComputation) { @@ -219,9 +211,206 @@ public void GetValueAsyncThatIsCancelledReturnsTaskCancelledWithCorrectToken() } catch (AggregateException ex) { - var operationCancelledException = (OperationCanceledException)ex.Flatten().InnerException; + var operationCancelledException = (OperationCanceledException)ex.Flatten().InnerException!; Assert.Equal(cancellationTokenSource.Token, operationCancelledException.CancellationToken); } } + + [Theory] + [CombinatorialData] + private static void CancellationDuringInlinedComputationFromGetValueOrGetValueAsyncStillCachesResult(bool includeSynchronousComputation) + { + var computations = 0; + var requestCancellationTokenSource = new CancellationTokenSource(); + object? createdObject = null; + + Func synchronousComputation = c => + { + Interlocked.Increment(ref computations); + + // We do not want to ever use the cancellation token that we are passed to this + // computation. Rather, we will ignore it but cancel any request that is + // outstanding. + requestCancellationTokenSource.Cancel(); + + createdObject = new object(); + return createdObject; + }; + + var lazy = new AsyncLazy( + c => Task.FromResult(synchronousComputation(c)), + includeSynchronousComputation ? synchronousComputation : null); + + var thrownException = Assert.Throws(() => + { + // Do a first request. Even though we will get a cancellation during the evaluation, + // since we handed a result back, that result must be cached. + lazy.GetValue(requestCancellationTokenSource.Token); + }); + + // And a second request. We'll let this one complete normally. + var secondRequestResult = lazy.GetValue(CancellationToken.None); + + // We should have gotten the same cached result, and we should have only computed once. + Assert.Same(createdObject, secondRequestResult); + Assert.Equal(1, computations); + } + + [Fact] + public void SynchronousRequestShouldCacheValueWithAsynchronousComputeFunction() + { + var lazy = new AsyncLazy(c => Task.FromResult(new object())); + + var firstRequestResult = lazy.GetValue(CancellationToken.None); + var secondRequestResult = lazy.GetValue(CancellationToken.None); + + Assert.Same(secondRequestResult, firstRequestResult); + } + + [Theory] + [CombinatorialData] + public async Task AwaitingProducesCorrectException(bool producerAsync, bool consumerAsync) + { + var exception = new ArgumentException(); + Func> asynchronousComputeFunction = + async cancellationToken => + { + await Task.Yield(); + throw exception; + }; + Func synchronousComputeFunction = + cancellationToken => + { + throw exception; + }; + + var lazy = producerAsync + ? new AsyncLazy(asynchronousComputeFunction) + : new AsyncLazy(asynchronousComputeFunction, synchronousComputeFunction); + + var actual = consumerAsync + ? await Assert.ThrowsAsync(async () => await lazy.GetValueAsync(CancellationToken.None)) + : Assert.Throws(() => lazy.GetValue(CancellationToken.None)); + + Assert.Same(exception, actual); + } + + [Fact] + public async Task CancelledAndReranAsynchronousComputationDoesNotBreakSynchronousRequest() + { + // We're going to create an AsyncLazy where we will call GetValue synchronously, and while that operation is + // running we're going to call GetValueAsync() more than once; the first time we will let cancel, the second time will + // run to completion. + var synchronousComputationStartedEvent = new ManualResetEvent(initialState: false); + var synchronousComputationShouldCompleteEvent = new ManualResetEvent(initialState: false); + + // We don't want the async path to run sooner than we expect, so we'll set it once ready + Func>? asynchronousComputation = null; + + var lazy = new AsyncLazy( + asynchronousComputeFunction: ct => + { + AssertEx.NotNull(asynchronousComputation, $"The asynchronous computation was not expected to be running."); + return asynchronousComputation(ct); + }, + synchronousComputeFunction: _ => + { + // Let the test know we've started, and we'll continue once asked + synchronousComputationStartedEvent.Set(); + synchronousComputationShouldCompleteEvent.WaitOne(); + return "Returned from synchronous computation: " + Guid.NewGuid(); + }); + + // Step 1: start the synchronous operation and wait for it to be running + var synchronousRequest = Task.Run(() => lazy.GetValue(CancellationToken.None)); + synchronousComputationStartedEvent.WaitOne(); + + // Step 2: it's running, so let's let a async operation get started and then cancel. We're ensuring that if this cancels, we might forget we have + // the synchronous operation running if we weren't careful. + var cancellationTokenSource = new CancellationTokenSource(); + + var asynchronousRequestToBeCancelled = lazy.GetValueAsync(cancellationTokenSource.Token); + cancellationTokenSource.Cancel(); + await asynchronousRequestToBeCancelled.NoThrowAwaitableInternal(); + Assert.Equal(TaskStatus.Canceled, asynchronousRequestToBeCancelled.Status); + + // Step 3: let's now let an async request run normally, producing a value + asynchronousComputation = _ => Task.FromResult("Returned from asynchronous computation: " + Guid.NewGuid()); + + var asynchronousRequest = lazy.GetValueAsync(CancellationToken.None); + + // Now let's finally complete our synchronous request that's been waiting for awhile + synchronousComputationShouldCompleteEvent.Set(); + var valueReturnedFromSynchronousRequest = await synchronousRequest; + + // We expect that in this case, we should still get the same value back + Assert.Equal(await asynchronousRequest, valueReturnedFromSynchronousRequest); + } + + [Fact] + public async Task AsynchronousResultThatWasCancelledDoesNotBreakSynchronousRequest() + { + // We're going to do the following sequence of operations: + // + // 1. Start an asynchronous request + // 2. Cancel the asynchronous request (but it's still consuming CPU because it hasn't observed the cancellation yet) + // 3. Start a synchronous request + // 4. Let the asynchronous request complete, as if the cancellation was never observed + // 5. Complete the synchronous request + var synchronousComputationStartedEvent = new ManualResetEvent(initialState: false); + var synchronousComputationShouldCompleteEvent = new ManualResetEvent(initialState: false); + var asynchronousComputationReadyToComplete = new ManualResetEvent(initialState: false); + var asynchronousComputationShouldCompleteEvent = new ManualResetEvent(initialState: false); + + var asynchronousRequestCancellationToken = new CancellationTokenSource(); + + var lazy = new AsyncLazy( + asynchronousComputeFunction: ct => + { + asynchronousRequestCancellationToken.Cancel(); + + // Now wait until the cancellation is sent to this underlying computation + while (!ct.IsCancellationRequested) + Thread.Yield(); + + // Now we're ready to complete, so this is when we want to pause + asynchronousComputationReadyToComplete.Set(); + asynchronousComputationShouldCompleteEvent.WaitOne(); + + return Task.FromResult("Returned from asynchronous computation: " + Guid.NewGuid()); + }, + synchronousComputeFunction: _ => + { + // Let the test know we've started, and we'll continue once asked + synchronousComputationStartedEvent.Set(); + synchronousComputationShouldCompleteEvent.WaitOne(); + return "Returned from synchronous computation: " + Guid.NewGuid(); + }); + + // Steps 1 and 2: start asynchronous computation and wait until it's running; this will cancel itself once it's started + var asynchronousRequest = Task.Run(() => lazy.GetValueAsync(asynchronousRequestCancellationToken.Token)); + + asynchronousComputationReadyToComplete.WaitOne(); + + // Step 3: while the async request is cancelled but still "thinking", let's start the synchronous request + var synchronousRequest = Task.Run(() => lazy.GetValue(CancellationToken.None)); + synchronousComputationStartedEvent.WaitOne(); + + // Step 4: let the asynchronous compute function now complete + asynchronousComputationShouldCompleteEvent.Set(); + + // At some point the asynchronous computation value is now going to be cached + string? asyncResult; + while (!lazy.TryGetValue(out asyncResult)) + Thread.Yield(); + + // Step 5: let the synchronous request complete + synchronousComputationShouldCompleteEvent.Set(); + + var synchronousResult = await synchronousRequest; + + // We expect that in this case, the synchronous result should have been thrown away since the async result was computed first + Assert.Equal(asyncResult, synchronousResult); + } } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs index 779bba564f612..c39aff10a6b58 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs @@ -289,7 +289,12 @@ public T GetValue(CancellationToken cancellationToken) // processing a value somebody never wanted cancellationToken.ThrowIfCancellationRequested(); - return result; + // Because we called CompleteWithTask with an actual result, _cachedResult must be set. However, it's possible that result is a different result than + // what is in our local variable here; it's possible that an asynchronous computation was running and cancelled, but eventually completed (ignoring cancellation) + // and then set the cached value. Because that could be a different instance than what we have locally, we can't use the local result if the other value + // got cached first. Always returning the cached value ensures we always return a single value from AsyncLazy once we got one. + Contract.ThrowIfNull(_cachedResult, $"We called {nameof(CompleteWithTask)} with a result, there should be a cached result."); + return _cachedResult.Result; } }