Async APIs for Data Protection key ring access
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 290
Description
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Is your feature request related to a problem? Please describe the problem.
Today, the entire Data Protection stack is synchronous:
```csharp
namespace Microsoft.AspNetCore.DataProtection.KeyManagement.Internal;
public interface IKeyRingProvider
{
IKeyRing GetCurrentKeyRing();
}
namespace Microsoft.AspNetCore.DataProtection;
public interface IDataProtector : IDataProtectionProvider
{
byte[] Protect(byte[] plaintext);
byte[] Unprotect(byte[] protectedData);
}
```
`GetCurrentKeyRing()` is on the hot path of every `Protect` / `Unprotect` call — which means it is on the hot path of every cookie auth check, antiforgery check, identity token validation, and so on. When the cached key ring is valid (the 99.9% case), the call is a single `Volatile.Read` and is essentially free. But when it isn't valid, the call blocks the calling thread — sometimes briefly, sometimes for the full duration of a repository read against the configured `IXmlRepository` (file share, Azure Blob, Azure Key Vault, Redis, EF, etc.).
Because the API contract is synchronous, every caller in the framework above must consume the result synchronously, and all the I/O the implementation might transitively perform must be turned into a synchronous wait somewhere in the chain.
### Concrete problem this causes
Issue #66380 is a direct consequence of this synchronous contract. The .NET 10 `KeyRingProvider` [rewrite](https://github.com/dotnet/aspnetcore/pull/54675) tried to dispatch refresh work onto `TaskScheduler.Default` so callers wouldn't pay the latency, but on cold start the synchronous public API forces the dispatcher and every racing caller to `Task.Wait()` on the dispatched work — pinning every thread‑pool thread on a task that needs a free thread‑pool thread to run.
The point fix in #66683 resolves the immediate bug by running the cold‑start refresh inline on the calling thread (matching the pre‑#54675 behavior). But it's a workaround for a structural limitation: as long as `GetCurrentKeyRing()` is synchronous, every refresh strategy that *doesn't* run on the calling thread has the same deadlock surface in some configuration.
### Describe the solution you'd like
### Proposed addition
Add **non‑breaking** async overloads to the public API surface, leaving the existing sync members in place for backwards compatibility:
```diff
namespace Microsoft.AspNetCore.DataProtection.KeyManagement.Internal;
public interface IKeyRingProvider
{
IKeyRing GetCurrentKeyRing();
+ ValueTask GetCurrentKeyRingAsync(CancellationToken cancellationToken = default);
}
namespace Microsoft.AspNetCore.DataProtection;
public interface IDataProtector : IDataProtectionProvider
{
byte[] Protect(byte[] plaintext);
byte[] Unprotect(byte[] protectedData);
+ ValueTask ProtectAsync(byte[] plaintext, CancellationToken cancellationToken = default);
+ ValueTask UnprotectAsync(byte[] protectedData, CancellationToken cancellationToken = default);
}
```
`ValueTask` is preferred for the hot path because the cached‑ring case is allocation‑free.
### Why async eliminates the starvation class of bugs
`await` returns the calling thread to the pool while the work is in flight, instead of pinning it inside `Task.Wait()`. Concurrent cold‑start callers no longer occupy thread‑pool slots while the refresh runs, so any refresh strategy — inline, dispatched, retried, whatever — is starvation‑safe. The refresh implementation can also legitimately do real async I/O against `IXmlRepository` instead of sync‑over‑async, which today is forced everywhere from file reads to Azure Blob calls.
### Additional context
_No response_
Contributor guide
Assessment
This issue has not been assessed yet.