Skip to content

[Prototype] Add cache size limit and eviction. #3104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pmaytak wants to merge 4 commits into from
Closed

[Prototype] Add cache size limit and eviction. #3104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
caf1827
Initial. Add app cache size limit, check, and cache clearing.
pmaytak Jan 12, 2022
5f4559d
Merge remote-tracking branch 'origin/master' into pmaytak/cache-evict…
pmaytak Feb 11, 2022
64f8718
Add cache limit and eviction functionality.
pmaytak Feb 11, 2022
ccb06a5
Address feedback.
pmaytak Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/client/Microsoft.Identity.Client/AppConfig/CacheOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;

namespace Microsoft.Identity.Client
{
/// <summary>
Expand Down Expand Up @@ -39,6 +41,17 @@ public CacheOptions(bool useSharedCache)
UseSharedCache = useSharedCache;
}

/// <summary>
/// Constructor
/// </summary>
/// <param name="useSharedCache">Set to true to share the cache between all ClientApplication objects. The cache becomes static. <see cref="UseSharedCache"/> for a detailed description. </param>
/// <param name="sizeLimit">Token cache size limit in bytes. <see cref="SizeLimit"/> for a detailed description.</param>
public CacheOptions(bool useSharedCache, long sizeLimit)

@SameerK-MSFT SameerK-MSFT Jan 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have default values here (as per any recommended practices)?

@pmaytak pmaytak Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can provide a realistic default value; it would be based on the user's scenario.

@bgavrilMS bgavrilMS Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good question we're struggling with :). What would be a good default? ...

@SameerK-MSFT SameerK-MSFT Feb 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If default varies widely based on the user scenarios, we cannot do much. But if it primarly leans towards a certain value, that can be used. But we do not have answer, then it is good as it is.

@pmaytak pmaytak Feb 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Danny also touched on a point of should there bit a minimum. Currently if someone specifies small value (like 1), cache will be compacted on each operation.

{
UseSharedCache = useSharedCache;
SizeLimit = sizeLimit;
}
Comment thread
pmaytak marked this conversation as resolved.

Comment thread
pmaytak marked this conversation as resolved.
/// <summary>
/// Share the cache between all ClientApplication objects. The cache becomes static. Defaults to false.
/// </summary>
Expand All @@ -50,5 +63,37 @@ public CacheOptions(bool useSharedCache)
/// </remarks>
public bool UseSharedCache { get; set; }

private long? _sizeLimit;

/// <summary>
/// Total token cache size limit in bytes for both app and user token caches.
Comment thread
pmaytak marked this conversation as resolved.
/// </summary>
/// <remarks>
/// Once the limit is reached, either the app or user cache will be fully cleared, depending on which was most recently used.
/// MSAL doesn't calculate the exact memory usage and uses approximations of the token sizes.
/// For instance, app token cache entry is approximately at least 4500 bytes; user access token entry - 6500 bytes,
/// user refresh token entry - 3700 bytes.
/// Using a MemoryCache via Microsoft.Identity.Web.TokenCache is more accurate but slower.
/// This size limit applies only to internal memory cache and is not a concern when distributed caching is used.
/// IMPORTANT: Monitor app health metrics (including memory usage) and cache performance (<see href="https://aka.ms/msal-net-token-cache-serialization"/>)
/// and adjust size limit accordingly.
/// </remarks>
public long? SizeLimit

@pmaytak pmaytak Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size limit applies only when cache serialization is disabled.

{
get => _sizeLimit;
set
{
ValidateSizeLimit(value);
_sizeLimit = value;
}
}

private void ValidateSizeLimit(long? sizeLimit)
{
if (sizeLimit < 0)
{
throw new ArgumentOutOfRangeException(nameof(sizeLimit), $"{nameof(sizeLimit)} must be a positive number.");
}
}
}
}
46 changes: 42 additions & 4 deletions ...rosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Microsoft.Identity.Client.Cache;
using Microsoft.Identity.Client.Cache.Items;
using Microsoft.Identity.Client.Cache.Keys;
Expand Down Expand Up @@ -35,6 +36,11 @@ internal class InMemoryPartitionedAppTokenCacheAccessor : ITokenCacheAccessor
protected readonly ICoreLogger _logger;
private readonly CacheOptions _tokenCacheAccessorOptions;

// Approximate size of cache item objects
private const long AccessTokenSizeInBytes = 4500;

@bgavrilMS bgavrilMS Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is that a constant approximation will not work well:

  1. AAD allows for configuration of optional claims
  2. POP tokens are a bit bigger because they contain the POP key id
  3. AAD is adding support for SAML tokens (there is already an OBO flow for this). We won't be able to update this aproximation for all token types.

I think there are a few ways out of this:

  1. Allow customers to define this constant (this is what SAL does). But this is a pretty "obscure" scenario
  2. Use non-constant size approximation, i.e. we know the size of the actual token (it's a string) at which we add some metadata (fairly constant)
  3. Do not expose sizeLimit, expose countLimit

@bgavrilMS bgavrilMS Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannybtsai @henrik-me - can you please review this strategy?

@dannybtsai dannybtsai Feb 11, 2022 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use a constant value instead of the actual size (the #2 option)?

@pmaytak pmaytak Feb 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgavrilMS

  1. Hmm, interesting, I'd like to see the implementation in SAL. How would customers know that constant?
  2. Yea, metadata should be fairly constant. This does seem like a better approximation.
  3. Specific to the user cache, what would the count represent: only access tokens, all tokens? Because in both cases, the count would not represent the whole cache size accurately.

@dannybtsai
How would you measure the actual size of objects in memory? We could measure the strings size in cache objects like Bogdan mentioned below and in Microsoft.Identity.Web we serialize the whole cache, so take size that way. But in this case cache entry is an object.


private long _appCacheSize;

public InMemoryPartitionedAppTokenCacheAccessor(
ICoreLogger logger,
CacheOptions tokenCacheAccessorOptions)
Expand All @@ -57,12 +63,26 @@ public InMemoryPartitionedAppTokenCacheAccessor(
#region Add
public void SaveAccessToken(MsalAccessTokenCacheItem item)
{
if (IsCacheOverCapacity(AccessTokenSizeInBytes))
{
_logger.Always("[AppCache] Cache is over capacity.");
Compact();
}

string itemKey = item.GetKey().ToString();
string partitionKey = CacheKeyFactory.GetClientCredentialKey(item.ClientId, item.TenantId, item.KeyId);

// Update cache size only if cache item is added, not updated
if (!AccessTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
{
Interlocked.Add(ref _appCacheSize, AccessTokenSizeInBytes);

@dannybtsai dannybtsai Feb 15, 2022 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_appCacheSize

Is it really necessary to use a variable to track the size? Isn't _appCacheSize == AccessTokenCacheDictionary.Count * AccessTokenSizeInBytes?

There seems to be some risk of not setting the size correctly even though Interlocked methods are atomic. And it is more complicated to set them in the Clear() method without locking.

@pmaytak pmaytak Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConcurrentDictionary<string, ConcurrentDictionary<string, MsalAccessTokenCacheItem>> AccessTokenCacheDictionary
TokenCacheDictionaries are dictionary of dictionaries. with the outer key being a partition key and an inner key is an actual cache item key. So unfortunately count does not work.

@dannybtsai dannybtsai Feb 17, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If AccessTokenSizeInBytes is added to the _appCacheSize every time a token is saved, isn't _appCacheSize == AccessTokenCacheDictionary.Count * AccessTokenSizeInBytes?

Interlocked.Add(ref TokenCache.CacheSize, AccessTokenSizeInBytes);

@dannybtsai dannybtsai Feb 15, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TokenCache.CacheSize

Can this be calculated?

@pmaytak pmaytak Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think so. There are two instances of TokenCache class created and then one instance has app token cache accessor and one has user accessor, but they don't know about each other. And I don't think CacheSize can be moved into a higher parent class.

}

// if a conflict occurs, pick the latest value
AccessTokenCacheDictionary
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalAccessTokenCacheItem>())[itemKey] = item;
_logger.Verbose($"[AppCache] Saved access token. App cache size: {Interlocked.Read(ref _appCacheSize)}.");
}

/// <summary>
Expand Down Expand Up @@ -134,9 +154,14 @@ public void DeleteAccessToken(MsalAccessTokenCacheItem item)
if (partition == null || !partition.TryRemove(item.GetKey().ToString(), out _))
{
_logger.InfoPii(
$"Cannot delete access token because it was not found in the cache. Key {item.GetKey()}.",
"Cannot delete access token because it was not found in the cache.");
$"[AppCache] Cannot delete access token because it was not found in the cache. Key {item.GetKey()}.",
"[AppCache] Cannot delete access token because it was not found in the cache.");
return;
}

Interlocked.Add(ref _appCacheSize, -AccessTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, -AccessTokenSizeInBytes);
_logger.Verbose($"[AppCache] Removed access token. App cache size: {Interlocked.Read(ref _appCacheSize)}.");
}

/// <summary>
Expand Down Expand Up @@ -175,7 +200,7 @@ public void DeleteAccount(MsalAccountCacheItem item)
/// </summary>
public virtual IReadOnlyList<MsalAccessTokenCacheItem> GetAllAccessTokens(string partitionKey = null)
{
_logger.Always($"[GetAllAccessTokens] Total number of cache partitions found while getting access tokens: {AccessTokenCacheDictionary.Count}");
_logger.Always($"[AppCache] Total number of cache partitions found while getting access tokens: {AccessTokenCacheDictionary.Count}");
if (string.IsNullOrEmpty(partitionKey))
{
return AccessTokenCacheDictionary.SelectMany(dict => dict.Value).Select(kv => kv.Value).ToList();
Expand Down Expand Up @@ -216,13 +241,26 @@ public void SetiOSKeychainSecurityGroup(string keychainSecurityGroup)
public virtual void Clear()
{
AccessTokenCacheDictionary.Clear();

@SameerK-MSFT SameerK-MSFT Jan 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threading issue: Between Clear and setting CacheSize to zero, something may get added to the dictionary and the two can go out of sync

@pmaytak pmaytak Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but we also don't lock on cache operations in general (based on eventual consistency principle). So this behavior can happen with other add/remove methods.

@dannybtsai dannybtsai Feb 11, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventual consistency is not guaranteed here. For example:

  1. Thread 1 calls the AccessTokenCacheDictionary.Clear() method and is paused.
  2. Thread 2 adds an item and updates the _appCacheSize.
  3. Thread 1 resumes execution and sets the appCacheSize to 0 (Interlocked.Exchange(ref _appCacheSize, 0)).
  4. Now there is one item in the cache but the size is 0.
  5. The same issue applies to the TokenCache.CacheSize, and in this case the error can accumulate over multiple Clear() calls.

@pmaytak pmaytak Feb 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannybtsai Do you suggest locking? Only when clearing or always when updating the cache size?

@dannybtsai dannybtsai Feb 15, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to achieve consistency without locking as far as I can tell. And you need to always lock, not just when clearing cache otherwise it is the same problem.

_logger.Always("[Clear] Clearing access token cache data.");
Interlocked.Add(ref TokenCache.CacheSize, -_appCacheSize);
Interlocked.Exchange(ref _appCacheSize, 0);
_logger.Always("[AppCache] Cleared access token cache data.");
// app metadata isn't removable
}

public virtual bool HasAccessOrRefreshTokens()
{
return AccessTokenCacheDictionary.Any(partition => partition.Value.Any(token => !token.Value.IsExpiredWithBuffer()));
}

private bool IsCacheOverCapacity(long sizeToAdd)
{
return _tokenCacheAccessorOptions.SizeLimit.HasValue && (Interlocked.Read(ref TokenCache.CacheSize) + sizeToAdd) > _tokenCacheAccessorOptions.SizeLimit;
}

private void Compact()
Comment thread
SameerK-MSFT marked this conversation as resolved.
{
_logger.Always("[AppCache] Compacting cache.");
Clear();
}
}
}
106 changes: 96 additions & 10 deletions ...osoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Microsoft.Identity.Client.Cache;
using Microsoft.Identity.Client.Cache.Items;
using Microsoft.Identity.Client.Cache.Keys;
Expand Down Expand Up @@ -44,6 +45,14 @@ internal class InMemoryPartitionedUserTokenCacheAccessor : ITokenCacheAccessor
protected readonly ICoreLogger _logger;
private readonly CacheOptions _tokenCacheAccessorOptions;

// Approximate size of cache item objects
private const long AccessTokenSizeInBytes = 6500;
private const long RefreshTokenSizeInBytes = 3700;
private const long IDTokenSizeInBytes = 3300;
private const long AccountSizeInBytes = 1300;

private long _userCacheSize;

public InMemoryPartitionedUserTokenCacheAccessor(ICoreLogger logger, CacheOptions tokenCacheAccessorOptions)
{
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
Expand All @@ -70,38 +79,82 @@ public InMemoryPartitionedUserTokenCacheAccessor(ICoreLogger logger, CacheOption
#region Add
public void SaveAccessToken(MsalAccessTokenCacheItem item)
{
// When saving tokens in SaveTokenResponseAsync, AT is saved first, then other tokens.
// For the user cache, checking size limit and compacting only when saving AT,
// because otherwise, if compact is run in other save methods,
// it could leave unassociated tokens in the cache.
// (For ex, compact in SaveAccount, would leave only account in the cache, without other tokens.)
if (IsCacheOverCapacity(AccessTokenSizeInBytes))
{
_logger.Always("[UserCache] Cache is over capacity.");
Compact();
Comment thread
pmaytak marked this conversation as resolved.
}

string itemKey = item.GetKey().ToString();
string partitionKey = CacheKeyFactory.GetKeyFromCachedItem(item);

// Update cache size only if cache item is added, not updated
if (!AccessTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
{
Interlocked.Add(ref _userCacheSize, AccessTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, AccessTokenSizeInBytes);
}

// if a conflict occurs, pick the latest value
AccessTokenCacheDictionary
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalAccessTokenCacheItem>())[itemKey] = item; // if a conflict occurs, pick the latest value
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalAccessTokenCacheItem>())[itemKey] = item;
_logger.Verbose($"[UserCache] Saved access token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void SaveRefreshToken(MsalRefreshTokenCacheItem item)
{
string itemKey = item.GetKey().ToString();
string partitionKey = CacheKeyFactory.GetKeyFromCachedItem(item);

// Update cache size only if cache item is added, not updated
if (!RefreshTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
{
Interlocked.Add(ref _userCacheSize, RefreshTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, RefreshTokenSizeInBytes);
}

RefreshTokenCacheDictionary
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalRefreshTokenCacheItem>())[itemKey] = item;
_logger.Verbose($"[UserCache] Saved refresh token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void SaveIdToken(MsalIdTokenCacheItem item)
{
string itemKey = item.GetKey().ToString();
string partitionKey = CacheKeyFactory.GetKeyFromCachedItem(item);

// Update cache size only if cache item is added, not updated
if (!IdTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
{
Interlocked.Add(ref _userCacheSize, IDTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, IDTokenSizeInBytes);
}

IdTokenCacheDictionary
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalIdTokenCacheItem>())[itemKey] = item;
_logger.Verbose($"[UserCache] Saved ID token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void SaveAccount(MsalAccountCacheItem item)
{
string itemKey = item.GetKey().ToString();
string partitionKey = CacheKeyFactory.GetKeyFromCachedItem(item);

// Update cache size only if cache item is added, not updated
if (!AccountCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
{
Interlocked.Add(ref _userCacheSize, AccountSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, AccountSizeInBytes);
}

AccountCacheDictionary
.GetOrAdd(partitionKey, new ConcurrentDictionary<string, MsalAccountCacheItem>())[itemKey] = item;
_logger.Verbose($"[UserCache] Saved account. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void SaveAppMetadata(MsalAppMetadataCacheItem item)
Expand Down Expand Up @@ -155,9 +208,14 @@ public void DeleteAccessToken(MsalAccessTokenCacheItem item)
if (partition == null || !partition.TryRemove(item.GetKey().ToString(), out _))
{
_logger.InfoPii(
$"Cannot delete access token because it was not found in the cache. Key {item.GetKey()}.",
"Cannot delete access token because it was not found in the cache.");
$"[UserCache] Cannot delete access token because it was not found in the cache. Key {item.GetKey()}.",
"[UserCache] Cannot delete access token because it was not found in the cache.");
return;
}

Interlocked.Add(ref _userCacheSize, -AccessTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, -AccessTokenSizeInBytes);
_logger.Verbose($"[UserCache] Removed access token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void DeleteRefreshToken(MsalRefreshTokenCacheItem item)
Expand All @@ -168,9 +226,14 @@ public void DeleteRefreshToken(MsalRefreshTokenCacheItem item)
if (partition == null || !partition.TryRemove(item.GetKey().ToString(), out _))
{
_logger.InfoPii(
$"Cannot delete refresh token because it was not found in the cache. Key {item.GetKey()}.",
"Cannot delete refresh token because it was not found in the cache.");
$"[UserCache] Cannot delete refresh token because it was not found in the cache. Key {item.GetKey()}.",
"[UserCache] Cannot delete refresh token because it was not found in the cache.");
return;
}

Interlocked.Add(ref _userCacheSize, -RefreshTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, -RefreshTokenSizeInBytes);
_logger.Verbose($"[UserCache] Removed refresh token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void DeleteIdToken(MsalIdTokenCacheItem item)
Expand All @@ -181,9 +244,14 @@ public void DeleteIdToken(MsalIdTokenCacheItem item)
if (partition == null || !partition.TryRemove(item.GetKey().ToString(), out _))
{
_logger.InfoPii(
$"Cannot delete ID token because it was not found in the cache. Key {item.GetKey()}.",
"Cannot delete ID token because it was not found in the cache.");
$"[UserCache] Cannot delete ID token because it was not found in the cache. Key {item.GetKey()}.",
"[UserCache] Cannot delete ID token because it was not found in the cache.");
return;
}

Interlocked.Add(ref _userCacheSize, -IDTokenSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, -IDTokenSizeInBytes);
_logger.Verbose($"[UserCache] Removed ID token. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}

public void DeleteAccount(MsalAccountCacheItem item)
Expand All @@ -194,9 +262,14 @@ public void DeleteAccount(MsalAccountCacheItem item)
if (partition == null || !partition.TryRemove(item.GetKey().ToString(), out _))
{
_logger.InfoPii(
$"Cannot delete account because it was not found in the cache. Key {item.GetKey()}.",
"Cannot delete account because it was not found in the cache");
$"[UserCache] Cannot delete account because it was not found in the cache. Key {item.GetKey()}.",
"[UserCache] Cannot delete account because it was not found in the cache");
return;
}

Interlocked.Add(ref _userCacheSize, -AccountSizeInBytes);
Interlocked.Add(ref TokenCache.CacheSize, -AccountSizeInBytes);
_logger.Verbose($"[UserCache] Removed account. User cache size: {Interlocked.Read(ref _userCacheSize)}.");
}
#endregion

Expand Down Expand Up @@ -277,11 +350,13 @@ public void SetiOSKeychainSecurityGroup(string keychainSecurityGroup)

public virtual void Clear()
{
_logger.Always("[Clear] Clearing access token cache data.");
AccessTokenCacheDictionary.Clear();
RefreshTokenCacheDictionary.Clear();
IdTokenCacheDictionary.Clear();
AccountCacheDictionary.Clear();
Interlocked.Add(ref TokenCache.CacheSize, -_userCacheSize);
Interlocked.Exchange(ref _userCacheSize, 0);
_logger.Always("[UserCache] Cleared access token cache data.");
// app metadata isn't removable
}

Expand All @@ -292,5 +367,16 @@ public virtual bool HasAccessOrRefreshTokens()
return RefreshTokenCacheDictionary.Any(partition => partition.Value.Count > 0) ||
AccessTokenCacheDictionary.Any(partition => partition.Value.Any(token => !token.Value.IsExpiredWithBuffer()));
}

private bool IsCacheOverCapacity(long sizeToAdd)
{
return _tokenCacheAccessorOptions.SizeLimit.HasValue && (Interlocked.Read(ref TokenCache.CacheSize) + sizeToAdd) > _tokenCacheAccessorOptions.SizeLimit;
}

private void Compact()
{
_logger.Always("[UserCache] Compacting cache.");
Clear();

@pmaytak pmaytak Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing the cache because it's simpler and other forms of compaction become complex for the user cache. (For ex. removing only ATs will leave the user cache with non-AT tokens; trying to remove all tokens for a user doesn't work for OBO since partition keys are different; randomly removing token entries will leave orphaned/unassociated tokens.)

}
}
}
7 changes: 6 additions & 1 deletion src/client/Microsoft.Identity.Client/TokenCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ public sealed partial class TokenCache : ITokenCacheInternal
/// </summary>
internal bool UsesDefaultSerialization { get; set; } = false;

/// <summary>
/// Static, used by both app and user caches to track approximate token cache size.
/// </summary>
internal static long CacheSize = 0L;

@pmaytak pmaytak Feb 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache size includes user+ app token cache. 2 instances of TokenCache class are created (1 with app cache accessors, 1 with user accessors). So moved this size property up a level so it can be accessed by both. However, because of this design, clearing the cache is only done for that respective cache, app or user.


internal string ClientId => ServiceBundle.Config.ClientId;

ITokenCacheAccessor ITokenCacheInternal.Accessor => Accessor;
Expand Down Expand Up @@ -117,7 +122,7 @@ public void SetIosKeychainSecurityGroup(string securityGroup)
(LegacyCachePersistence as Microsoft.Identity.Client.Platforms.iOS.iOSLegacyCachePersistence).SetKeychainSecurityGroup(securityGroup);
#endif
}

private void UpdateAppMetadata(string clientId, string environment, string familyId)
{
if (_featureFlags.IsFociEnabled)
Expand Down