Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hang in AsyncLazy<T>.DisposeValueAsync #1340

Merged
merged 1 commit into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/Microsoft.VisualStudio.Threading/AsyncLazy`1.cs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ public void DisposeValue()
/// </returns>
/// <remarks>
/// <para>Calling this method will put this object into a disposed state where future calls to obtain the value will throw <see cref="ObjectDisposedException"/>.</para>
/// <para>If the value has already been produced and implements <see cref="IDisposable"/>, <see cref="IAsyncDisposable"/>, or <see cref="System.IAsyncDisposable"/> it will be disposed of.
/// <para>If the value has already been produced and implements <see cref="IDisposable"/>, <see cref="IAsyncDisposable"/>, or <see cref="System.IAsyncDisposable"/> it will be disposed of.
/// If the value factory has already started but has not yet completed, its value will be disposed of when the value factory completes.</para>
/// <para>If prior calls to obtain the value are in flight when this method is called, those calls <em>may</em> complete and their callers may obtain the value, although <see cref="IDisposable.Dispose"/>
/// may have been or will soon be called on the value, leading those users to experience a <see cref="ObjectDisposedException"/>.</para>
Expand All @@ -308,6 +308,7 @@ public void DisposeValue()
/// </remarks>
public async Task DisposeValueAsync()
{
JoinableTask<T>? localJoinableTask = null;
Task<T>? localValueTask = null;
object? localValue = default;
lock (this.syncObject)
Expand All @@ -331,6 +332,7 @@ public async Task DisposeValueAsync()
// We'll schedule the value for disposal outside the lock so it can be synchronous with the value factory,
// but will not execute within our lock.
localValueTask = this.value;
localJoinableTask = this.joinableTask;
break;
}

Expand All @@ -345,7 +347,11 @@ public async Task DisposeValueAsync()
this.valueFactory = null;
}

if (localValueTask is not null)
if (localJoinableTask is not null)
{
localValue = await localJoinableTask;
}
else if (localValueTask is not null)
{
localValue = await localValueTask.ConfigureAwait(false);
}
Expand Down
25 changes: 25 additions & 0 deletions test/Microsoft.VisualStudio.Threading.Tests/AsyncLazyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,31 @@ public async Task Dispose_CalledTwice_Disposable_Completed()
await this.AssertDisposedLazyAsync(lazy);
}

[Fact]
public void DisposeValue_MidFactoryThatContestsForMainThread()
{
JoinableTaskContext context = this.InitializeJTCAndSC();

AsyncLazy<object> lazy = new(
async delegate
{
// Ensure the caller keeps control of the UI thread,
// so that the request for the main thread comes in when it's controlled by others.
await Task.Yield();
await context.Factory.SwitchToMainThreadAsync(this.TimeoutToken);
return new();
},
context.Factory);

Task<object> lazyFactory = lazy.GetValueAsync(this.TimeoutToken);

// Make a JTF blocking call on the main thread that won't return until the factory completes.
context.Factory.Run(async delegate
{
await lazy.DisposeValueAsync().WithCancellation(this.TimeoutToken);
});
}

[Fact(Skip = "Hangs. This test documents a deadlock scenario that is not fixed (by design, IIRC).")]
public async Task ValueFactoryRequiresReadLockHeldByOther()
{
Expand Down