Why does the IdentityServer4.EntityFramework project not use asynchronous EF methods?

[neyromant]

Hi, I'm surprised that all calls to Db Context are synchronous. And I'm ready to replace them with asynchronous ones.
I wanted to know why it was not done before?

Is there any good reason for this?

[neyromant]

UPD:
@brockallen I found your use_async_ef branch (https://github.com/IdentityServer/IdentityServer4.EntityFramework/compare/use_async_ef)

Is there any reasons why it does not merge into the main branch?

[brockallen]

We've had issues with the underlying EF implementation under load.

[neyromant]

Currently we can see the following behavior - under load and with a long ping to DB, all application's threads get busy. Kestrel successfully processes connections, but the application is not responsible for sufficient time.

Our application is IdentityServer4 + IdentityServer4.EntityFramework on Kestrel. Nothing more

[neyromant]

What kind of issues did you have? Do you have any load tests?

[neyromant]

@brockallen I have another question - why don't you use AsNoTracking for read-only queries? Is there any reason for this? Can I put them on?

[neyromant]

@brockallen I added AsNoTracking for readonly queries - IdentityServer/IdentityServer4.EntityFramework#136

[brockallen]

What does AsNoTracking do? How does it affect different providers?

[TomCJones]

Is this designed to hold the value of DNT - or DoNotTrack from the HTTP header?

DNT would mean (inter alia) that any existing OP signin cannot store or release information about the sites visited. (Obviously the OP does record all the clients that have been registered for a user, at least if pairwise sub is implemented.) It should imply that no client or external provider can get any information about signin to other sites. Vanilla OpenID Connect should provide this by default. API grants might leak this information if not careful.

[neyromant]

@brockallen AsNoTracking avoids the overhead of setting up change tracking for each entity instance. See https://docs.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.entityframeworkqueryableextensions.asnotracking?view=efcore-2.0

What does AsNoTracking do? How does it affect different providers?

[brockallen]

Since this store API is designed as read-only, would it make more sense to set this globally? https://docs.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.changetracking.changetracker.querytrackingbehavior?view=efcore-2.0#Microsoft_EntityFrameworkCore_ChangeTracking_ChangeTracker_QueryTrackingBehavior?

[scottbrady91]

On a per project basis, yes. It would be a breaking change to bring into the EF library though. A lot of people are using the context's and our registrations for client & scope management.

[brockallen]

On a per project basis, yes. It would be a breaking change to bring into the EF library though. A lot of people are using the context's and our registrations for client & scope management.

Actually, yea, so maybe it's the extension method level where we could do this rather then in the DbCOntext defaults.

[brockallen]

Also, does anyone really know the perf difference for using this?

[neyromant]

@brockallen I have prepared some simple benchmarks
for AsNoTracking (https://github.com/neyromant/Benchmarks/blob/master/Benchmarks/AsNoTracking/AsNoTrackingTests.cs) using BenchmarkDotNet (https://github.com/dotnet/BenchmarkDotNet)

Here is the results:

Method	Records	DbType	Mean	Error	StdDev	Scaled	Gen 0	Gen 1	Gen 2	Allocated
Track	1	InMemory	1,997.5 us	13.308 us	12.448 us	1.00	42.9688	23.4375	7.8125	166.06 KB
NoTrack	1	InMemory	1,766.5 us	10.005 us	8.355 us	0.88	42.9688	21.4844	9.7656	164.92 KB
Track	1	SqlLight	962.1 us	4.921 us	4.362 us	1.00	3.9063	-	-	19.45 KB
NoTrack	1	SqlLight	950.4 us	1.304 us	1.156 us	0.99	3.9063	-	-	18.95 KB
Track	10	InMemory	1,991.6 us	4.890 us	4.574 us	1.00	44.9219	21.4844	9.7656	169.07 KB
NoTrack	10	InMemory	1,835.1 us	3.521 us	3.294 us	0.92	42.9688	21.4844	9.7656	165.53 KB
Track	10	SqlLight	1,011.0 us	2.834 us	2.512 us	1.00	5.8594	-	-	24.33 KB
NoTrack	10	SqlLight	968.5 us	4.634 us	3.870 us	0.96	3.9063	-	-	20.74 KB
Track	100	InMemory	2,195.0 us	7.693 us	7.196 us	1.00	50.7813	27.3438	7.8125	202.41 KB
NoTrack	100	InMemory	1,884.8 us	7.144 us	6.333 us	0.86	42.9688	19.5313	9.7656	166.59 KB
Track	100	SqlLight	1,423.6 us	8.378 us	7.837 us	1.00	17.5781	-	-	74.24 KB
NoTrack	100	SqlLight	1,091.7 us	5.925 us	5.542 us	0.77	7.8125	-	-	38.19 KB
Track	1000	InMemory	4,997.9 us	30.450 us	28.483 us	1.00	101.5625	39.0625	15.6250	534.86 KB
NoTrack	1000	InMemory	1,957.9 us	3.175 us	2.652 us	0.39	42.9688	23.4375	7.8125	173.64 KB
Track	1000	SqlLight	5,880.8 us	43.999 us	41.156 us	1.00	117.1875	39.0625	-	575.44 KB
NoTrack	1000	SqlLight	2,417.2 us	11.036 us	10.323 us	0.41	50.7813	11.7188	-	214.11 KB

[neyromant]

What do you think about it?

[brockallen]

I've not had time to consider it. In general I think I will like the idea -- I just need to spend the time learning more about it.

[brockallen]

As you should have seen from the other issue, the AsNoTracking was merged. Thanks!

We're still debating the async EF features. Feel free to comment on that.

[brockallen]

No additional input on this thread? We're planning on releasing a 2.3 preview1 soon, so time to discuss is now.

[quanterion]

It is suprising to me that EF access is sync. Everywhere in Asp.Net Identity it is async. So I think IdentityServer should follow this pattern.

[brockallen]

@D3MaxT From what I've seen, using async when hitting the DB puts more stress on the DB. Or you end up allow more throughput into your web server, which causes more connections to be requested, which then floods the connection pool, and you get time out exceptions.

So by blocking synchronously you're sort of throttling the attempts to the DB, thus distributing the load. Too many async operations to the DB concentrates the load.

I admit, I'm not an expert in this area. This is just what I've seen.

[jepperc]

Late to the party, but I think this makes sense to do.
What is happening because of this, is that threads are synchronously waiting in middleware for SQL connections to be available. This means less threads are able to handle requests, which might not even use SQL. So I think the argument of "putting more load" on sql is wrong, and the database should definitely not be throttled on purpose using threadpool starvation.

I have been trying out https://github.com/benaadams/Ben.BlockingDetector , which clearly tells you that this is a bad idea, and I'm pretty sure Ben Adams is an expert :)
It highlights exactly the entity framework sync calls from Identityserver.

Is there anything I can do to convince you/progress on the issue?

[flotteur]

@brockallen your observation isn't wrong. It's mostly because once you lifted the bottleneck on the web server, the DB become the next target. But it doesn't mean that the bottleneck on the web server should stay in place.

As @jepperc mentionned, blocking threads in async code isn't a good the way to avoid hitting a bottleneck on the DB server. The current implementation can also impact requests on the same thread that would otherwise not be blocked.

[ChristianPejrup]

What about using Entity Framework DbContext Pooling ?
https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-2.0#dbcontext-pooling

Good blog that explains connection pooling

Benefit

• It limits the active connections to the database (and can be ajusted default:128)
• It reuses connections which should make the initial connection process to the database faster
• It prevents TCP connection starvation

Drawback

• It does not address blocking calls in the code
• If there is some bottleneck with Entity Framework and Async that will still be an issue here
  -- I know getting big objects with EF in Async will spawn more threads to get the data from the database, which will eventually cause TCP connection starvation

This should keep any bottlenecks in the code and not push them to the database.

[mackie1001]

My take on it is that we shouldn't be intentionally throttling in the application by using synchronous IO when there's an async alternative. It's about efficient use of threads rather than absolute response times. If rate limiting is the goal then this should be managed at traffic manager level IMO - it can make much better decisions about how to go about doing that.

It does have implications though. We've encountered connection pool exhaustion under load testing before. The lack of blocking and the huge and sudden ramp up of the load test caught the application off guard and it just couldn't open connections fast enough.
In the end we got the results we wanted just by making the connection pool much larger by default. When the load hit the idle app it was ready and waiting with hundreds/thousands of open connections and breezed through the test without any timeouts. Some DB operations took a little longer than usual but actual CPU usage was relatively low if I recall correctly.

The data access in IDS4 is nearly all simple queries producing small result sets and single row updates/inserts using clustered indexes so we've never even got close to a performance problem even on quite a modest DB server setup with synchronous commits to the secondary in an AlwaysOn cluster. This is despite scaling out (and thus supporting more throughput even for synchronous operations) the application multiple times over the last couple of years.

I'm willing to create my own async versions and be a bit of a guinea pig - I've got no qualms about the stability of async EF in a production environment.

[brockallen]

This is slated for our 4.0 release, which will target ASP.NET Core 3.1 (which will be their LTS).

[mackie1001]

Good to know, thanks.

We're currently completing our 1.1 -> 2.1 upgrade - painful. I'm thinking 2.1 -> 3.1 LTS will be a lot easier when it's available.

[AliBazzi]

is it ok to work on this and open a PR ? or you have other plans ?

[gubenkoved]

I was quite surprised to see that IdS4 is having the same (?) issue with Thread Pool starvation we have with IdentityServer3 (according to the reports above). V3 is using async EF methods by default, but there is a caveat -- the class ClientConverter and ScopeConverter which implement JSON.NET's JsonConverter which has synchronous signatures. In order to use these synchronous signatures with asynchronous IdS repositories (e.g. IClientStore) IdS developers (Brock ?) were forced to come up with something that is called AsyncHelper in IdS3 there which is causing the Thread Pool starvation as it blocks the current thread to wait for asynchronous operation to complete. We had to rewrite some of the implementations for the stores to get around this (ones which are inherited from IdentityServer3.EntityFramework.BaseTokenStore<T>).

(also wondering how synchronous methods are implemented by the EF -- if they are the wrappers against the asynchronous (like the trick with AsyncHelper), then no wonder why would we get the same problem)

[HoboRobo123]

I know it's not ideal to use external libraries but I believe Polly is included by default in the ASP.NET Core metapackage now.

Have you considered leveraging Polly's Bulkhead-Isolation, Rety (with exponential back up), and Circuit Breaker policies to prevent load from causing starvation or affecting other processes?

https://github.com/App-vNext/Polly

[brockallen]

finally done in PR.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.