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 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

merged 2 commits into from
Sep 5, 2019

Conversation

jkonecki
Copy link
Contributor

@jkonecki jkonecki commented Sep 4, 2019

Resolving an async deadlock caused by AsyncCache performing a blocking wait.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Closing issues

Closes #758 and probably #717

@jkonecki jkonecki requested a review from kirillg as a code owner September 4, 2019 08:21
j82w
j82w previously approved these changes Sep 4, 2019
@j82w
Copy link
Contributor

j82w commented Sep 4, 2019

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

ealsur
ealsur previously approved these changes Sep 4, 2019
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

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

I wonder if there is a way to add a UT to avoid any future regressions

@@ -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.

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

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.

@kirankumarkolli
Copy link
Member

Thanks alot @jkonecki for contribution.

@jkonecki jkonecki dismissed stale reviews from ealsur and j82w via 5788002 September 5, 2019 08:37
@kirankumarkolli
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kirankumarkolli
Copy link
Member

Thanks alot @jkonecki

@kirankumarkolli kirankumarkolli merged commit 82317af into Azure:master Sep 5, 2019
@jkonecki jkonecki deleted the fix/758-async-deadlock branch September 5, 2019 16:48
@jkonecki
Copy link
Contributor Author

jkonecki commented Sep 5, 2019

Do you have any ETA for the next release of the NuGet package?
How can I track when this fix will be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CosmosClient hang up when run under Orleans
4 participants