EntityFramework Core 3.1.0 uses synchronous API calls when using async APIs #21818
Comments
|
This was indeed fixed for 5.0 in #20694 (by the commit mentioned above), so this issue is asking to backport that to 3.1. However, @martinmine, I'm not sure how a switch from async to sync APIs could cause a dramatic increase in CPU usage - in 3.1 both ultimately call into the same synchronous DbTransaction.Commit... In other words, you're indeed not expected to get any benefit from using async API (because it's actually async-over-sync, rather than async all the way), but you're definitely not supposed to see a meaningful regression either. Could you please carefully check on that and report back? |
|
Thanks for such a quick response @roji! In my case, I have my .NET Core app running on two application servers towards one SQL Server. And the SQL Server is at times acting slow. The impact on the slow SQL server resulted in some cases unstable response times up to 2-3 seconds, compared to having a stable 45ms response time with using synchronous APIs in EF. With the async API I can see that bursts of traffic can suddenly go through my service with first a large spike in response time that then steadily declines over time including my service suddenly consuming up to 60% CPU on both machines. If the underlying APIs are synchronous, this behavior would be expected and I am experiencing symptoms of thread pool starvation: How come EF does not use the asynchronous APIs? |
|
@martinmine your analysis sounds correct to me... If indeed you're executing sync operations from TP threads, and those operations block for a long time (because of failovers), that could indeed cause TP starvation as you describe.
For transaction management (e.g. commit), the async System.Data APIs have only recently been added, in .NET Core 3.0 (dotnet/runtime#28596), which is why EF Core 3.1 doesn't yet use them. Note that EF Core 5.0 (currently in development) already does use them - as you found above. Note also that SqlClient itself hasn't yet implemented the async transaction APIs (dotnet/SqlClient#113), which I hope they do soon. So at the end of the day, this is a somewhat new API that is still propagating throughout the various components in the ecosystem... |
|
I see. I have also narrowed down my problem to the following lines of code: public override async Task<Client> FindClientByIdAsync(string clientId)
{
var baseQuery = Context.Clients.Where(x => x.ClientId == clientId);
var client = await baseQuery.FirstOrDefaultAsync();
await baseQuery.SelectMany(x => x.AllowedCorsOrigins).LoadAsync();
await baseQuery.SelectMany(x => x.AllowedGrantTypes).LoadAsync();
await baseQuery.SelectMany(x => x.AllowedScopes).LoadAsync();
await baseQuery.SelectMany(x => x.Claims).LoadAsync();
await baseQuery.SelectMany(x => x.ClientSecrets).LoadAsync();
await baseQuery.SelectMany(x => x.IdentityProviderRestrictions).LoadAsync();
await baseQuery.SelectMany(x => x.PostLogoutRedirectUris).LoadAsync();
await baseQuery.SelectMany(x => x.Properties).LoadAsync();
await baseQuery.SelectMany(x => x.RedirectUris).LoadAsync();
return client.ToModel();
}So in my case, the more awaitable tasks I have that requires to be done for each request, the more threads it will require for my request to get processed. So if I understand you correctly, you wouldn't recommend the usage of |
Not necessarily - the code above only has one query running at any point, so the thread gets released when completing each LoadAsync before going on to the next. Importantly, I also don't see any transactions in this code: EF Core 3.1 does call async underlying APIs, transaction management is the exception (again, because those APIs were only recently added).
I wouldn't necessary say that. Again, keep in mind that the problem we're discussing is limited to transaction management; other operations should be async. You can specifically concentrate on code sites where transactions are being managed, and see how to work around the problem, e.g. by not executing that code on TP threads, or by having enough TP threads available up-front to support your load. I agree this is an unfortunate situation - ideally all components here would support async properly, but we're not quite there yet... |
|
I understand. Do you have any suggestions why the query I run performs so poorly in that case? Would it be what I experience is the overhead of async? (It is however the artifact of a workaround that was proposed in this sage: #18022) |
|
@martinmine it's difficult to know without seeing some code - if you can put together a minimal repro for me to look at, I can investigate further. If you really do suspect that BeginTransaction/Commit/Rollback specifically hang for long periods of time (you can probably verify this using SqlClient's DiagnosticSource support, or your own logging), and you know they are executing from TP threads, then indeed this could be the source of TP starvation and extremely poor perf (you should be able to diagnose TP starvation from dotnet counters or similar). I'd first make sure I understand exactly where the slowdown is coming from, using these diagnostic tools, to rule out that this is something unrelated. |
|
Why are you not simply using Includes? |
|
It is true that I am using this pattern to avoid the cartesian explosion, otherwise I would have used Includes as @ErikEJ suggests. |
|
Closing as there is nothing actionable on EF Core and the SqlClient issue is tracked in their repo. |
I just switched from synchronous API to asynchronous API in EntityFramework and I could see a tremendous increase in CPU usage and response times on my service during a load test. Looking at memory dumps I have taken during high load, I can see that the following statement calls
Storage.RelationalTransaction.Commit()fromUpdate.Internal.BatchExecutor.ExecuteAsync: https://github.com/dotnet/efcore/blob/v3.1.0/src/EFCore.Relational/Update/Internal/BatchExecutor.cs#L145It seems to me that this is a synchronous API-call, while it should be calling .CommitAsync in this case. It is however been fixed by a later commit here: 1e977ae#diff-c5e724774bdd22fa0e9f3c2143e74d1b
This is not backported to 3.x of EF-Core but seems to be target for 5.0. How come it calls the synchronous API under the hood? Is there any reason for this and could we possibly backport the fix to 3.x?
Steps to reproduce
Call
Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteReaderAsync(Microsoft.EntityFrameworkCore.Storage.RelationalCommandParameterObject parameterObject, System.Threading.CancellationToken cancellationToken)that will produce similar stack trace to the one above.Further technical details
EF Core version: v3.1.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Windows Server 2012 R2
Relates to this issue in IdentityServer4: IdentityServer/IdentityServer4#4672
The text was updated successfully, but these errors were encountered: