Why IConcurrencyLock in DefaultCache<T>?

[hugoqribeiro]

Which version of Duende IdentityServer are you using?

6.2.3

Which version of .NET are you using?

.NET 7

Describe the bug/question

While investigating an issue in our production environment - from time to time, one of our servers rises above 95% CPU - I found a lot of exceptions raised by DefaultCache<T> around the same time:

Failed to obtain cache lock for: 'Duende.IdentityServer.Services.DefaultCache`1[Duende.IdentityServer.Models.Client]'

One could argue that, given the CPU is loaded, the connection to the database gets much slower, so we get this problem with the cache timeout. I am still to find the reason for the CPU being under stress.

My issue is with the implementation of GetOrAddAsync().

Isn't this a huge bottleneck? You are blocking retrieving client X while retrieving client Y?
Does that make sense? Why the lock in the first place? What would be the problem of 2 threads reading the same client at the same time from the database?

On a side note, it would be much better if ALL your service implementations could be easily overridable. This is something that I think you should consider, to improve Identity Server extensibility at large. I could change GetOrAddAsync() without having to implement the whole service...

Log output/exception with stacktrace

System.Exception at Duende.IdentityServer.Services.DefaultCache`1+<GetOrAddAsync>d__18.MoveNext
Failed to obtain cache lock for: 'Duende.IdentityServer.Services.DefaultCache`1[Duende.IdentityServer.Models.Client]'

[josephdecock]

Hi, thanks for getting in touch. We haven't encountered many performance problems with the DefaultCache implementation, so its really helpful to hear when folks have issues. In many cases, the cached items are updated very infrequently, so locking while they are updated seems like a sensible defensive strategy at face value to me. But, if you're seeing performance problems, my assumptions might not always hold. Have you tried replacing the cache implementation as you describe? If so, did it resolve your performance problem?

[hugoqribeiro]

I will do that... but it will take some time for that release to get in production. And this is one of those cases where you can only see the results in production...

But I don't understand your point of view.

You start the service and get the first 2 requests at the same time to connect/token with different clients. Is it reasonable to have 1 request wait for the other?

More, you get to a point where for some reason the database is slow, is it reasonable to have a request fail because you are not able to add the client to the cache?

[josephdecock]

These are all good points that you're raising. While we probably want some mechanism to prevent someone from reading the cache as it is being written, the locking is probably more aggressive than it needs to be. We'll consider updating the cache lock behavior in a future release, and also look forward to hearing more about how your custom implementation of the cache works. For now, I'll close this issue, but feel free to reopen it if you get interesting results re:your custom cache implementation that you want to share. Thanks!

[alirezanet]

Hi @hugoqribeiro,
I'm facing the same error on production under heavy loads, did you solve it? any tips to make my life easier 😅?

[hugoqribeiro]

Hi @alirezanet.

I replaced the default implementation of ICache<T> that does not use the lock mentioned above.