Efcore: deadlock when calling SaveChangesAsync from synchronous method

[YehudahA]

[sorry for my bad English]

Some times I have to call SaveChangesAsync from synchronous method.

In EF6 it works well.
In EFCore on AspNetCore, it works well.
But in EFCore on Aspnet it causes deadlock.

Context:

public class MyContext : DbContext
{
    public MyContext(DbContextOptions<MyContext> options) : base(options)
    {
    }

    public DbSet<Customer> Customers { get; set; }

    public int SaveChangesSynchronous() 
        => SaveChangesAsync()
        .ConfigureAwait(false)
        .GetAwaiter()
        .GetResult();
}

Controller action:

[HttpPost]
public Customer Post(Customer customer)
{
    context.Customers.Update(customer);
    context.SaveChangesSynchronous();
    return customer;
}

[ajcvickers]

@YehudahA Why not just call the non-async SaveChanges method?

[pmiddleton]

Calling an aysnc method from a sync method can cause a deadlock in aspnet. This is due to the synchronization context aspnet uses.

See this article for more info - https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

You're best workaround is to call the non-async saveChanges method as @ajcvickers said.

[YehudahA]

@ajcvickers @pmiddleton, I know SaveChanges. But I override the SaveChangesAsync and put there a lot of logic: validations, insert logs, etc.

In my Asp.Net app I have a lot of calls to SaveChanges and a lot of calls to SaveChangesAsync.
With EF6, I solved this with overriding of SaveChanges to calling of SaveChangesAsync.

public override int SaveChanges()
    => this.SaveChangesAsync()
    .ConfigureAwait(false)
    .GetAwaiter()
    .GetResult();

public override async Task<int> SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default(CancellationToken))
{
    await DoSomethingAsync();

    var changes = await base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);

    await DoOtherAsync();

    return changes;
}

Now, I migrate to EFCore, and this code couses deadlock..

[YehudahA]

@pmiddleton, According to the @StephenCleary article, preventing the deadlocks is a job of library developers. they have to put ConfigureAwait(false) all the way. (as probably done in EF6)

https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html#preventing-the-deadlock

[StephenCleary]

@YehudahA In your case, I'd recommend using the boolean argument hack.

Would look something like this:

```C#
public override int SaveChanges(bool acceptAllChangesOnSuccess) => DoSaveChangesAsync(acceptAllChangesOnSuccess, CancellationToken.None, sync: true).GetAwaiter().GetResult();
public override Task SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default(CancellationToken)) => DoSaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken, sync: false);

private async Task DoSaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken, bool sync)
{
await DoSomethingAsync(sync); // If sync is true, DoSomethingAsync must execute synchronously.

var changes = sync ? base.SaveChanges(acceptAllChangesOnSuccess) :
    await base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);

await DoOtherAsync(sync); // If `sync` is true, `DoOtherAsync` must execute synchronously.

return changes;

}
```

This allows you to have one method with your logic, but also avoids issues with sync-over-async.

[YehudahA]

@StephenCleary, the DoSomethingAsync also depends on EFCore async methods. for example: context.SomeDbSet.GetListAsunc(). If EFCore captures the context (you call it "not async-fiendly"), Those methods can also blocking, so I have to use the flag all the way, and actually duplicate the code.

Thread Pool Hack suitable for me, and works well, with some hacks, like resolveing any per-context resource (HttpContext.Current.User etc.) before Task.Run starts.

[YehudahA]

I don't understand why it's "by design".
According of @StephenCleary articles it seem as a poor design.

Why you cannot put ConfigureAwait(false) on every async call? (like HttpClient lib and even EF6)

[ajcvickers]

This was discussed in triage with the following conclusions:

• EF calls into user code. This makes it more important the the context is preserved.
  
  
  
  • We could be selective and avoid the overhead on some low-level paths
  
  
• Deadlocks was not a major concern because they only happen when .Wait is used, and this is bad practice anyway
• Perf overhead was measured to be small
  
  
  
  • Perf overhead might be significant in tight loops, but these are a lot slower in async anyway, so it's not clear that it is worth doing anything here--but see idea above that we could change selected low-level paths
  
  
  • If async gets generally faster, then the relative perf gain could become more
  
  

Overall, we are not going to do anything immediately or generally on this, but based on other perf testing we could look at making targeted changes.

[ajcvickers]

Note also this text from the linked post: "Using ConfigureAwait(false) to avoid deadlocks is a dangerous practice. You would have to use ConfigureAwait(false) for every await in the transitive closure of all methods called by the blocking code, including all third- and second-party code. Using ConfigureAwait(false) to avoid deadlock is at best just a hack)."

[thumer]

Please add ConfigureAwait(false) in the EFCore Library.
We are porting our EF6 WPF application to EFCore. Most of our queries and business logic code are written in async. We are reusing same methods multiple times. In some cases we need to call .Wait() for these methods (for example the user is not allowed to change the row in a datagrid -> DB-Queries to check). In EF6 we had no problems at all. Now in EFCore we have a lot of deadlocks.
Our rule is, no await without ConfigureAwait(false) in our Domain/Library Assemblies --> we have written an roslyn analyzer to guarantee this.
If EFCore do not fix this important issue, we need to create our own branch and fix it ourselves.

[YehudahA]

@thumer, I you have a Roslyn analyzer that guarantee this, you may share it with EF team :)

[thumer]

Yes of course, I will share our branch soon. We had already changed the code, all works fine now. But every time we pull a new version, we have to adjust the code. It would be more useful if this would already be changed in the dev branch.

[thumer]

@YehudahA
We uploaded our Roslyn analyzer, that ensures that all awaitcalls are configured with .ConfigureAwait(false):
RZL.ConfigureAwaitAnalyzer

We forked EntityFrameworkCore and branched from 2.1.1 where we added .ConfigureAwait(false) to all await calls:
ConfigureAwait Branch
The analyzer is not yet added to our branch, because we haven´t published it to Nuget yet.

[YehudahA]

@thumer, it solves your problem? we can say that this simple change fixes the deadlocks?

cc @ajcvickers

[thumer]

@YehudahA
Yes it solves our problem. We created this duplicate Ticket Deadlock in WPF when calling .Wait() #12407. Before we had deadlocks when we call .Wait() on an async operations.
With the modified EFCore Version RZL.ConfigureAwaitAnalyzer it works in all cases. It is easy to test it with our Sample Project.