Change how the key ring cache is updated by amcasey · Pull Request #54675 · dotnet/aspnetcore

amcasey · 2024-03-21T21:55:13Z

I wanted to address two concerns:

If there is a cached key ring, but it is expired, the first thread to discover this will synchronously refresh the cache - other threads will continue to use the old value.
If a key ring refresh is forced, it will always hit the backing repository, even if several threads want a refresh at the same time.

With this change, all key ring updates are computed on a thread-pool thread and callers block exactly when there is no cached key ring for them to fall back on (first run, for example) or if they are forcing a refresh (an in-flight refresh is considered satisfactory).

Moving this work to a background thread will give us more room to be generous with retries when (e.g.) Azure KeyVault is unreachable or a file is locked.

The old behavior can be restored using the appcontext switch Microsoft.AspNetCore.DataProtection.KeyManagement.DisableAsyncKeyRingUpdate. This is a safety valve and not part of configuration - we'll remove the switch and the old code path in the next release if nothing blows up.

Micro-benchmarking (on 9.0 on x64) shows that the new version has comparable performance in the common case of finding an unexpired key ring in the cache.

I wanted to address two concerns: 1. If there is a cached key ring, but it is expired, the first thread to discover this will _synchronously_ refresh the cache - other threads will continue to use the old value. 2. If a key ring refresh is forced, it will always hit the backing repository, even if several threads want a refresh at the same time. With this change, _all_ key ring updates are computed on a thread-pool thread and callers block exactly when there is no cached key ring for them to fall back on (first run, for example) or if they are forcing a refresh (an in-flight refresh is considered satisfactory). Moving this work to a background thread will give us more room to be generous with retries when (e.g.) Azure KeyVault is unreachable or a file is locked. The old behavior can be restored using the appcontext switch `Microsoft.AspNetCore.DataProtection.KeyManagement.DisableAsyncKeyRingUpdate`. This is a safety valve and not part of configuration - we'll remove the switch and the old code path in the next release if nothing blows up. Micro-benchmarking (on 9.0 on x64) shows that the new version has comparable performance in the common case of finding an unexpired key ring in the cache.

amcasey · 2024-03-21T22:17:22Z

-        mockCacheableKeyRingProvider.Setup(o => o.GetCacheableKeyRing(updatedKeyRingTime))
-            .Returns<DateTimeOffset>(dto =>
-            {
-                // at this point we're inside the critical section - spawn the background thread now


This test was unsalvageable because it depended upon the lambda being evaluated under a particular lock. MultipleThreadsSeeExpiredCachedValue attempts to provide analogous coverage.

captainsafia

Change seems sound overall! Would definitely want other eyes on all the multi-threaded logic as well.

amcasey · 2024-04-18T21:35:40Z

Thanks @captainsafia and @BrennanConroy! This was a complicated one.

halter73 · 2024-04-22T18:25:03Z

+            if (newCacheableKeyRing is null)
+            {
+                // There will have been a better exception from the winning thread
+                throw Error.KeyRingProvider_RefreshFailedOnOtherThread();


The exception must have already happened at this point. Why can't we capture it and throw it from any thread that fails to retrieve the KeyRing due to the exception?

I didn't think it would be helpful to print the underlying exception once per affected task, even if we were to add a note that it was being re-reported from elsewhere.

I still think it's more helpful than throwing a generic exception. We don't have to log it each time.

As a compromise, I've attached existingTask.Exception as the inner exception.

- Take locks earlier since they're going to be taken anyway - Observe exceptions on background tasks - Rethrow task exceptions in the standard way - Remove redundant Volatiles - Attach an inner exception on losing threads

* Fix KeyRingProvider thread pool starvation on forced refresh The cold-start starvation fix (#66683/#66737) left a second variant: when a valid key ring is cached and a caller forces a refresh (e.g. during the two-minute startup auto-refresh window an incoming cookie names a key not in the current ring), GetCurrentKeyRingCoreNew scheduled the refresh onto TaskScheduler.Default and blocked on Task.Wait(). With every thread-pool thread parked on that wait, the queued refresh could not run, causing thread pool starvation and a multi-second freeze. See #66380. Unify the invariant that already protects cold start: any caller that must block for a value produces it inline on its own thread, so no caller ever blocks on TaskScheduler.Default work. Non-forced callers with a stale ring still dispatch the async refresh and return the stale ring without waiting, preserving the perf win from #54675. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec35c6be-ab9b-47d1-9563-d7a61490ebb2 * add debug assert * Harden forced-refresh path per PR review Addresses two issues raised in review of the forced-refresh starvation fix: - On inline refresh failure, only extend the lifetime of a ring that is already stale. A forced refresh can fail while the cached ring is still valid; previously WithTemporaryExtendedLifetime would then cut that valid ring's expiration to 2 minutes and swap its expiration token for CancellationToken.None, causing key changes to be ignored until the shortened lifetime elapsed. This now mirrors the guard in GetKeyRingFromCompletedTaskUnsynchronized. - After publishing an inline forced-refresh result, discard any async refresh task still in flight so its older result cannot later be consumed and clobber the fresher ring. The discarded task's exception is observed to avoid an unobserved TaskScheduler.UnobservedTaskException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d900e559-5b83-4d0c-b179-e1cc2cda9792 * add regression tests --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec35c6be-ab9b-47d1-9563-d7a61490ebb2 Copilot-Session: d900e559-5b83-4d0c-b179-e1cc2cda9792

…#68001) * Fix KeyRingProvider thread pool starvation on forced refresh The cold-start starvation fix (#66683/#66737) left a second variant: when a valid key ring is cached and a caller forces a refresh (e.g. during the two-minute startup auto-refresh window an incoming cookie names a key not in the current ring), GetCurrentKeyRingCoreNew scheduled the refresh onto TaskScheduler.Default and blocked on Task.Wait(). With every thread-pool thread parked on that wait, the queued refresh could not run, causing thread pool starvation and a multi-second freeze. See #66380. Unify the invariant that already protects cold start: any caller that must block for a value produces it inline on its own thread, so no caller ever blocks on TaskScheduler.Default work. Non-forced callers with a stale ring still dispatch the async refresh and return the stale ring without waiting, preserving the perf win from #54675. * add debug assert * Harden forced-refresh path per PR review Addresses two issues raised in review of the forced-refresh starvation fix: - On inline refresh failure, only extend the lifetime of a ring that is already stale. A forced refresh can fail while the cached ring is still valid; previously WithTemporaryExtendedLifetime would then cut that valid ring's expiration to 2 minutes and swap its expiration token for CancellationToken.None, causing key changes to be ignored until the shortened lifetime elapsed. This now mirrors the guard in GetKeyRingFromCompletedTaskUnsynchronized. - After publishing an inline forced-refresh result, discard any async refresh task still in flight so its older result cannot later be consumed and clobber the fresher ring. The discarded task's exception is observed to avoid an unobserved TaskScheduler.UnobservedTaskException. * add regression tests --------- (cherry picked from commit a973708) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec35c6be-ab9b-47d1-9563-d7a61490ebb2 Copilot-Session: d900e559-5b83-4d0c-b179-e1cc2cda9792

ghost added the area-dataprotection Includes: DataProtection label Mar 21, 2024

amcasey requested a review from captainsafia March 21, 2024 21:55

amcasey mentioned this pull request Mar 21, 2024

Limit parallelism of forced key ring refreshes #54627

Closed

amcasey commented Mar 21, 2024

View reviewed changes

Comment thread src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs Resolved

amcasey commented Mar 21, 2024

View reviewed changes

Comment thread src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs Resolved

amcasey commented Mar 21, 2024

View reviewed changes

Comment thread src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs Resolved

amcasey commented Mar 21, 2024

View reviewed changes

amcasey mentioned this pull request Mar 23, 2024

Allow retries in DefaultKeyResolver.CanCreateAuthenticatedEncryptor #54711

Merged

dotnet-policy-service Bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 29, 2024

captainsafia approved these changes Apr 15, 2024

View reviewed changes

amcasey added 2 commits April 18, 2024 11:56

Add explanatory comments

37f28f8

Clarify that the background task can't be cancelled

de61ffd

BrennanConroy approved these changes Apr 18, 2024

View reviewed changes

Comment thread src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs Resolved

Clarify comment

fb4f415

amcasey enabled auto-merge (squash) April 18, 2024 21:35

amcasey merged commit c7aae8f into dotnet:main Apr 18, 2024

dotnet-policy-service Bot added this to the 9.0-preview4 milestone Apr 18, 2024

amcasey deleted the AsyncRefresh branch April 19, 2024 00:05

halter73 reviewed Apr 22, 2024

View reviewed changes

Youssef1313 mentioned this pull request Apr 20, 2026

KeyRingProvider causes thread pool starvation on cold start in .NET 10 #66380

Closed

1 task done

KennethHoff mentioned this pull request Jun 27, 2026

Data Protection: CreateProtector reads the key store eagerly on .NET 11 (was lazy until first Protect/Unprotect on .NET 10) #67447

Closed

1 task done

DeagleGross mentioned this pull request Jul 23, 2026

Fix KeyRingProvider thread pool starvation on forced refresh #67986

Merged

DeagleGross mentioned this pull request Jul 24, 2026

[release/10.0] Fix KeyRingProvider thread pool starvation on forced refresh #68001

Merged

5 of 8 tasks

Conversation

amcasey commented Mar 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amcasey Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amcasey commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

amcasey Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amcasey Mar 21, 2024 •

edited

Loading

halter73 Apr 22, 2024 •

edited

Loading