From d0f8f6c50d4811c63d991a6080ae779ecb93f597 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Fri, 26 Aug 2016 21:38:36 -0700 Subject: [PATCH 1/2] Add test that demonstrates AsyncLazy hang --- .../AsyncLazyTests.cs | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/src/Microsoft.VisualStudio.Threading.Tests.Shared/AsyncLazyTests.cs b/src/Microsoft.VisualStudio.Threading.Tests.Shared/AsyncLazyTests.cs index 99dc79c99..852848d4d 100644 --- a/src/Microsoft.VisualStudio.Threading.Tests.Shared/AsyncLazyTests.cs +++ b/src/Microsoft.VisualStudio.Threading.Tests.Shared/AsyncLazyTests.cs @@ -412,6 +412,97 @@ async delegate Assert.True(resultTask.Wait(AsyncDelay)); } + /// + /// Verifies that no deadlock occurs if the value factory synchronously blocks while switching to the UI thread. + /// + [Theory] + [CombinatorialData] + public void ValueFactoryRequiresMainThreadHeldByOtherSync(bool passJtfToLazyCtor) + { + var ctxt = SingleThreadedSynchronizationContext.New(); + SynchronizationContext.SetSynchronizationContext(ctxt); + var context = new JoinableTaskContext(); + var asyncPump = context.Factory; + var originalThread = Thread.CurrentThread; + + var evt = new AsyncManualResetEvent(); + var lazy = new AsyncLazy( + async delegate + { + // It is important that no await appear before this JTF.Run call, since + // we're testing that the value factory is not invoked while the AsyncLazy + // holds a private lock that would deadlock when called from another thread. + asyncPump.Run(async delegate + { + await asyncPump.SwitchToMainThreadAsync(this.TimeoutToken); + }); + await Task.Yield(); + return new object(); + }, + passJtfToLazyCtor ? asyncPump : null); // mix it up to exercise all the code paths in the ctor. + + var backgroundRequest = Task.Run(async delegate + { + return await lazy.GetValueAsync(); + }); + + Thread.Sleep(AsyncDelay); // Give the background thread time to call GetValueAsync(), but it doesn't yield (when the test was written). + var foregroundRequest = lazy.GetValueAsync(); + + var frame = SingleThreadedSynchronizationContext.NewFrame(); + var combinedTask = Task.WhenAll(foregroundRequest, backgroundRequest); + combinedTask.WithTimeout(UnexpectedTimeout).ContinueWith(_ => frame.Continue = false, TaskScheduler.Default); + SingleThreadedSynchronizationContext.PushFrame(ctxt, frame); + + // Ensure that the test didn't simply timeout, and that the individual tasks did not throw. + Assert.True(foregroundRequest.IsCompleted); + Assert.True(backgroundRequest.IsCompleted); + Assert.Same(foregroundRequest.GetAwaiter().GetResult(), backgroundRequest.GetAwaiter().GetResult()); + } + + /// + /// Verifies that no deadlock occurs if the value factory synchronously blocks while switching to the UI thread + /// and the UI thread then starts a JTF.Run that wants it too. + /// + [Fact] + public void ValueFactoryRequiresMainThreadHeldByOtherInJTFRun() + { + var ctxt = SingleThreadedSynchronizationContext.New(); + SynchronizationContext.SetSynchronizationContext(ctxt); + var context = new JoinableTaskContext(); + var asyncPump = context.Factory; + var originalThread = Thread.CurrentThread; + + var evt = new AsyncManualResetEvent(); + var lazy = new AsyncLazy( + async delegate + { + // It is important that no await appear before this JTF.Run call, since + // we're testing that the value factory is not invoked while the AsyncLazy + // holds a private lock that would deadlock when called from another thread. + asyncPump.Run(async delegate + { + await asyncPump.SwitchToMainThreadAsync(this.TimeoutToken); + }); + await Task.Yield(); + return new object(); + }, + asyncPump); + + var backgroundRequest = Task.Run(async delegate + { + return await lazy.GetValueAsync(); + }); + + Thread.Sleep(AsyncDelay); // Give the background thread time to call GetValueAsync(), but it doesn't yield (when the test was written). + asyncPump.Run(async delegate + { + var foregroundValue = await lazy.GetValueAsync(this.TimeoutToken); + var backgroundValue = await backgroundRequest; + Assert.Same(foregroundValue, backgroundValue); + }); + } + [Fact(Skip = "Hangs. This test documents a deadlock scenario that is not fixed (by design, IIRC).")] public async Task ValueFactoryRequiresReadLockHeldByOther() { From 806daef3b827f571465d1ccddbc840dba9f469f2 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Sat, 15 Oct 2016 13:07:57 -0700 Subject: [PATCH 2/2] Fix AsyncLazy hang when value factory blocks ...before first yielding await. --- .../AsyncLazy.cs | 22 ++--- .../InlineResumable.cs | 80 +++++++++++++++++++ ...ft.VisualStudio.Threading.Shared.projitems | 1 + 3 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.VisualStudio.Threading.Shared/InlineResumable.cs diff --git a/src/Microsoft.VisualStudio.Threading.Shared/AsyncLazy.cs b/src/Microsoft.VisualStudio.Threading.Shared/AsyncLazy.cs index deaf69e95..4c6f48d9f 100644 --- a/src/Microsoft.VisualStudio.Threading.Shared/AsyncLazy.cs +++ b/src/Microsoft.VisualStudio.Threading.Shared/AsyncLazy.cs @@ -133,6 +133,7 @@ public Task GetValueAsync(CancellationToken cancellationToken) Verify.FailOperation(Strings.ValueFactoryReentrancy); } + InlineResumable resumableAwaiter = null; lock (this.syncObject) { // Note that if multiple threads hit GetValueAsync() before @@ -143,12 +144,18 @@ public Task GetValueAsync(CancellationToken cancellationToken) if (this.value == null) { cancellationToken.ThrowIfCancellationRequested(); + resumableAwaiter = new InlineResumable(); + Func> originalValueFactory = this.valueFactory; + this.valueFactory = null; + Func> valueFactory = async delegate + { + await resumableAwaiter; + return await originalValueFactory().ConfigureAwait(continueOnCapturedContext: false); + }; + this.recursiveFactoryCheck.Value = RecursiveCheckSentinel; try { - var valueFactory = this.valueFactory; - this.valueFactory = null; - if (this.jobFactory != null) { // Wrapping with RunAsync allows a future caller @@ -171,18 +178,15 @@ public Task GetValueAsync(CancellationToken cancellationToken) this.value = valueFactory(); } } - catch (Exception ex) - { - var tcs = new TaskCompletionSource(); - tcs.SetException(ex); - this.value = tcs.Task; - } finally { this.recursiveFactoryCheck.Value = null; } } } + + // Allow the original value factory to actually run. + resumableAwaiter?.Resume(); } if (!this.value.IsCompleted) diff --git a/src/Microsoft.VisualStudio.Threading.Shared/InlineResumable.cs b/src/Microsoft.VisualStudio.Threading.Shared/InlineResumable.cs new file mode 100644 index 000000000..f7ab71371 --- /dev/null +++ b/src/Microsoft.VisualStudio.Threading.Shared/InlineResumable.cs @@ -0,0 +1,80 @@ +/******************************************************** +* * +* © Copyright (C) Microsoft. All rights reserved. * +* * +*********************************************************/ + +namespace Microsoft.VisualStudio.Threading +{ + using System; + using System.Runtime.CompilerServices; + using System.Threading; + + /// + /// An awaiter that can be pre-created, and later immediately execute its one scheduled continuation. + /// + internal class InlineResumable : INotifyCompletion + { + /// + /// The continuation that has been scheduled. + /// + private Action continuation; + + /// + /// The current as of when the continuation was scheduled. + /// + private SynchronizationContext capturedSynchronizationContext; + + /// + /// Whether has been called already. + /// + private bool resumed; + + /// + /// Gets a value indicating whether an awaiting expression should yield. + /// + /// Always false + public bool IsCompleted => this.resumed; + + /// + /// Does and returns nothing. + /// + [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Performance", "CA1822:MarkMembersAsStatic", Justification = "Awaiter pattern requires this to be an instance method.")] + public void GetResult() + { + } + + /// + /// Stores the continuation for later execution when is invoked. + /// + /// The delegate to execute later. + public void OnCompleted(Action continuation) + { + Requires.NotNull(continuation, nameof(continuation)); + Assumes.Null(this.continuation); // Only one continuation is supported. + + this.capturedSynchronizationContext = SynchronizationContext.Current; + this.continuation = continuation; + } + + /// + /// Gets this instance. This method makes this awaiter double as its own awaitable. + /// + /// This instance. + public InlineResumable GetAwaiter() => this; + + /// + /// Executes the continuation immediately, on the caller's thread. + /// + public void Resume() + { + this.resumed = true; + var continuation = this.continuation; + this.continuation = null; + using (this.capturedSynchronizationContext.Apply()) + { + continuation?.Invoke(); + } + } + } +} diff --git a/src/Microsoft.VisualStudio.Threading.Shared/Microsoft.VisualStudio.Threading.Shared.projitems b/src/Microsoft.VisualStudio.Threading.Shared/Microsoft.VisualStudio.Threading.Shared.projitems index 671f33692..96aca3973 100644 --- a/src/Microsoft.VisualStudio.Threading.Shared/Microsoft.VisualStudio.Threading.Shared.projitems +++ b/src/Microsoft.VisualStudio.Threading.Shared/Microsoft.VisualStudio.Threading.Shared.projitems @@ -31,6 +31,7 @@ +