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.

Sign up for GitHub

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 for an async deadlock, reported in #758 (and probably #717) #761

Merged
merged 2 commits into from
Sep 5, 2019
Merged

Fix for an async deadlock, reported in #758 (and probably #717) #761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e789f78
Fix for an async deadlock, reported in #758 (and probably #717)
jkonecki Sep 4, 2019
5788002
Added a UT for async deadlock using a custom Task Scheduler.
jkonecki Sep 5, 2019
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
2 changes: 1 addition & 1 deletion Microsoft.Azure.Cosmos/src/Routing/AsyncCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public ICollection<TKey> Keys

public void Set(TKey key, TValue value)
{
AsyncLazy<TValue> lazyValue = new AsyncLazy<TValue>(() => value, CancellationToken.None);
AsyncLazy<TValue> lazyValue = new AsyncLazy<TValue>(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on how to UT/guard for future changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will TestCluster UT based approach validate it?
https://dotnet.github.io/orleans/Documentation/tutorials_and_samples/testing.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a test that uses AsyncCache with a single thread scheduler should suffice, without needing to use Orleans at all. I will try to create one.

Copy link
Contributor Author

@jkonecki jkonecki Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kirankumarkolli I've added the test. Instead of adding a dependency on Orleans I've implemented a simple task scheduler in the Testing project that processes a single task at a time (SingleTaskScheduler). It can be reused in other tests to confirm the code doesn't deadlock.
I would suggest a code review, during which you could identify other places in the codebase where a task result is obtained in a blocking way (eg. .Result is called) - those will be potential deadlock spots.


// Access it to mark as created+completed, so that further calls to getasync do not overwrite.
#pragma warning disable VSTHRD002 // Avoid problematic synchronous waits
Expand Down
5 changes: 5 additions & 0 deletions Microsoft.Azure.Cosmos/src/Routing/AsyncLazy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ namespace Microsoft.Azure.Cosmos.Common

internal sealed class AsyncLazy<T> : Lazy<Task<T>>
{
public AsyncLazy(T value)
: base(() => Task.FromResult(value))
kirankumarkolli marked this conversation as resolved.
Show resolved Hide resolved
{
}

public AsyncLazy(Func<T> valueFactory, CancellationToken cancellationToken)
: base(() => Task.Factory.StartNewOnCurrentTaskSchedulerAsync(valueFactory, cancellationToken)) // Task.Factory.StartNew() allows specifying task scheduler to use which is critical for compute gateway to track physical consumption.
{
Expand Down