Share SharedPreferencesInMemoryCache across cache instances Fixes AB#3428107

[siddhijain]

Fixes AB#3428107
Proposing to use static instance of in-memory cache layer for performance benefits.

[github-actions]

✅ Work item link check complete. Description contains link AB#3428107 to an Azure Boards work item.

[github-actions]

❌ Invalid work item number: AB#3428107
Proposing. Work item number must be a valid integer.

Click here to learn more.

[Copilot]

Pull Request Overview

This PR introduces a performance optimization by sharing SharedPreferencesAccountCredentialCacheWithMemoryCache instances across multiple BrokerOAuth2TokenCache instances. Instead of creating a new in-memory cache wrapper for each token cache, the code now maintains a static ConcurrentHashMap that maps INameValueStorage instances to their corresponding cached SharedPreferencesAccountCredentialCacheWithMemoryCache objects. This avoids redundant memory allocation and duplicate cache loading operations when the in-memory cache flight is enabled.

Key Changes

• Extracted cache initialization logic into a new getCacheToBeUsed() helper method that implements the caching strategy
• Added a static ConcurrentHashMap (inMemoryCacheMapByStorage) to store and reuse SharedPreferencesAccountCredentialCacheWithMemoryCache instances keyed by their underlying storage
• Removed unused setSingleSignOnState and targetCacheSetSingleSignOnState methods from BrokerOAuth2TokenCache (these remain available through the parent MsalOAuth2TokenCache class)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
common4j/src/main/com/microsoft/identity/common/java/cache/BrokerOAuth2TokenCache.java	Refactored cache creation to use a static map for sharing in-memory cache instances; added getCacheToBeUsed() method; removed unused SSO state methods
changelog.txt	Added changelog entry documenting this minor performance improvement

[Copilot]

Severity: Medium – CacheKeyValueDelegate instantiated per call instead of being reused

Issue: A new CacheKeyValueDelegate instance is created on every call to getCacheToBeUsed(), even when returning a cached SharedPreferencesAccountCredentialCacheWithMemoryCache instance. However, the delegate is only used during the initial construction (inside computeIfAbsent) when the cache is first created.

Impact:

1. Unnecessary object allocation on every cache lookup (minor performance overhead)
2. Inconsistency: when the flight is disabled, each call creates a new SharedPreferencesAccountCredentialCache with a new delegate (expected), but when enabled, the returned cached instance uses the delegate from the first creation, not the current one
3. This could cause confusion during debugging if delegate behavior needs to be analyzed

Recommendation: Move the cacheKeyValueDelegate instantiation inside the lambda passed to computeIfAbsent so it's only created when a new cache instance is actually constructed. This makes the code more efficient and clearer:

private static IAccountCredentialCache getCacheToBeUsed(@NonNull final INameValueStorage<String> spfm) {
    final boolean isFlightEnabled = CommonFlightsManager.INSTANCE
            .getFlightsProvider()
            .isFlightEnabled(CommonFlight.USE_IN_MEMORY_CACHE_FOR_ACCOUNTS_AND_CREDENTIALS);
    SpanExtension.current().setAttribute(AttributeName.in_memory_cache_used_for_accounts_and_credentials.name(), isFlightEnabled);
    if (isFlightEnabled) {
        return inMemoryCacheMapByStorage.computeIfAbsent(spfm, s ->
                new SharedPreferencesAccountCredentialCacheWithMemoryCache(
                        new CacheKeyValueDelegate(),
                        spfm
                )
        );
    } else {
        return new SharedPreferencesAccountCredentialCache(
                new CacheKeyValueDelegate(),
                spfm
        );
    }
}

[siddhijain]

changes in the next commit.

[moumighosh]

@siddhijain , let's not do this in the future, all comments must be addressed in the same PR

[siddhijain]

Yes - I did take care of all the comments in this PR. I commented changes in the next 'commit' not the next 'PR'. This specific comment however got outdated after made key related changes in the PR.

[Copilot]

Severity: Medium – Static cache map not cleared during clearAll() operations

Issue: The static inMemoryCacheMapByStorage map is never cleared, even when BrokerOAuth2TokenCache.clearAll() is called (line 1326). The clearAll() method clears individual token caches and application metadata, but the cached SharedPreferencesAccountCredentialCacheWithMemoryCache instances remain in the static map.

Impact:

1. After clearAll(), subsequent cache operations may still use the old cached instances which have already been cleared, leading to inconsistent state
2. The in-memory cache layer within SharedPreferencesAccountCredentialCacheWithMemoryCache will be out of sync with the underlying storage after a clear operation
3. In testing scenarios, this can cause test pollution where one test's data affects another test even after explicit cache clearing

Recommendation:

1. Add a static method to clear the inMemoryCacheMapByStorage map (e.g., clearCacheMap()) and call it from clearAll()
2. Alternatively, call clear() on the map itself within clearAll() to ensure the in-memory cache references are released
3. Consider whether clearing should trigger reloading of the in-memory cache or if the instances should be invalidated entirely

Example fix:

@Override
public void clearAll() {
    final List<BrokerApplicationMetadata> allClientsMetadata = mApplicationMetadataCache.getAll();
    for (final BrokerApplicationMetadata clientMetadata : allClientsMetadata) {
        final OAuth2TokenCache clientTokenCache = getTokenCacheForClient(
                clientMetadata.getClientId(),
                clientMetadata.getEnvironment(),
                clientMetadata.getUid()
        );

        if (clientTokenCache != null) {
            clientTokenCache.clearAll();
        }
    }

    this.mFociCache.clearAll();
    this.mApplicationMetadataCache.clear();
    inMemoryCacheMapByStorage.clear(); // Clear the static cache map
}

[siddhijain]

Changes in the next commit.

[moumighosh]

@siddhijain , let's discuss this offline, we should not be auto-resolving CoPilot comments when it is flagging high severity/Medium severity issues. All issues must be addressed in the same PR

[mohitc1]

[rpdome]

As discussed offline, consider using ReadWriteLock in this layer for further optimization.
(maybe consider benchmarking if it's worth it)

[moumighosh]

@siddhijain, is this comment addressed or you are going to address it in a future commit. @rpdome , @mohitc1 , please block PRs from merging in instances like this.

[rpdome]

@moumighosh We synced offline. This will be picked up as a separate work item.
(It's not a bug/regression in this PR - more like "what you can look into improving next.")