[Prototype] Use IIdentityCache and CompositeCache#3439
Conversation
… option. Add CompositeCache as an implementation.
|
|
||
| internal IdentityCacheWrapper(CacheOptions cacheOptions) | ||
| { | ||
| // Set (or overwrite) cache to user-specified implementation, otherwise set to default implementation, if not already set. |
There was a problem hiding this comment.
not sure about default implentation.
There was a problem hiding this comment.
Yea, definitely it's adding a dependency to .NET Memory Cache. Added it for simplicity to show that it could work E2E. Practically we could replace it with some other memory cache with eviction that doesn't have dependencies (eg MiseCache).
| { | ||
| internal class IdentityCacheWrapper | ||
| { | ||
| internal static IIdentityCache s_iIdentityCache { get; set; } |
There was a problem hiding this comment.
This should probably be updated. The lifecycle of user-provided cache should be managed externally to MSAL. Whether the default cache is static or not would be set via CacheOptions.UseSharedCache.
| } | ||
| } | ||
|
|
||
| private async Task<ITokenCacheAccessor> GetOrCreateAccessorAsync(string partitionKey) |
There was a problem hiding this comment.
I think the order of precedence of which cache to use should be something like:
- Use user-provided IIdentityCache implementation, or
- Use user-specified legacy cache serialization, or
- Use default in-memory cache based on IIdentityCache.
| return Task.FromResult(result); | ||
| } | ||
|
|
||
| public Task SetAsync<T>(string category, string key, T value, CacheEntryOptions cacheEntryOptions, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
T is of type ITokenCacheAccessor. Although we could change it to a simpler data structure.
| } | ||
| else | ||
| { | ||
| var cachedAccessor = await IdentityCacheWrapper.GetAsync<ITokenCacheAccessor>(partitionKey).ConfigureAwait(false); |
There was a problem hiding this comment.
The accessors could be simplified to just a dictionary, since we're partitioning in the cache implementation now.
… implementation. Set default static cache properly.
|
Ran some perf tests comparing current cache and new cache. When
|
3 participants
Related to cache improvements #3532.
General problem statement
Currently if user does not specify caching mechanism, MSAL's default cache is in-memory without eviction. If in-memory cache with eviction is desired, the recommendation is use a separate Microsoft.Identity.Web.TokenCaches package, however, which also uses object serialization.
Goal
Changes proposed in this request
IIdentityCachewhen creating a confidential client app instance via theCacheOptions.IIdentityCacheusing .NET's Memory Cache if user does not set up caching.ICompositeCache.Previous prototypes for cache improvements (#3104, #3177) focused on how to implement better cache (namely Wilson's) by replacing current caching code in non-breaking way. The biggest obstacle was the use of
GetAccountsmethod and otherGetAllXmethods that are used in cache serialization which are unable to provide a partition key. It is an issue because generally cache implementation don't provideGetAllfunctionality. (Everywhere else we do have a partition key). There was also a challenge of replacing the storage structures themselves (ex. accessors) to work with the new cache - it would require significant refactoring.This prototype only focuses on CCA. The
GetAccountsand otherGetAllXmethods without partition key will not use the new cache. (GetAccountsis not applicable to confidential apps because we recommend distributed caching there, so there would only ever be one account in the cache at a time.) This prototype also has the new cache instance side-by-side with the current cache serialization which are mutually exclusive.CodeTour
CodeTour file going over overall end-to-end code changes: IIdentityCache_Prototype.zip
How-to: