-
Notifications
You must be signed in to change notification settings - Fork 337
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.
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
Add partitioned user cache #2881
Conversation
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryTokenCacheAccessor.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
In terms of functional testing, we need to enforce that:
Maybe we should inject a test accessor that throws exceptions? |
Since we still do ADAL legacy cache operations, which are not partitioned, this will introduce a penalty and won't really work with the static cache idea. So I'm doing a bit of refactoring there. |
In Generally GetAccountsAsync should continue to work, even if it goes cross partition. |
I removed the partition key in GetAccountsAsync since it returns accounts for the whole app. For RemoveAccountInternal the partition key is always home account ID of the passed-in account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmaytak and @bgavrilMS
I've left a few questions where I didn't understand.
src/client/Microsoft.Identity.Client/Platforms/Android/AndroidTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Cache/Items/MsalAccessTokenCacheItem.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Platforms/Android/AndroidTokenCacheAccessor.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Platforms/Android/AndroidTokenCacheAccessor.cs
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Show resolved
Hide resolved
...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs
Show resolved
Hide resolved
tests/Microsoft.Identity.Test.Unit/CacheTests/CachePartitioningTests.cs
Outdated
Show resolved
Hide resolved
Added performance test results in the description. |
src/client/Microsoft.Identity.Client/Cache/ITokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Cache/ITokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Cache/ITokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Cache/ITokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Cache/ITokenCacheAccessor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments but otherwise looks good, I agree with comments made by others
…l serialization is not configured
9d335c0
to
7952732
Compare
Fixes #2861.
Changes proposed in this request
CreateTokenCacheAccessor
method from specific proxy classes to the abstract base class.ITokenCacheAccessor
interface (removed some unused methods, updated method signatures).IsAppSubscribedToSerializationEvents
andIsAppSubscribedToSerializationEvents
.IsExpired
logic intoMsalAccessTokenCacheItem
class.HasAccessOrRefreshTokens
logic into the accessor classes.Testing
Performance impact
IsMultiTenant
flag represents a scenario when tokens are requested for different tenants or just for one tenant.False
, there are TokensPerPartition number of access tokens in each partition but only 1 refresh token, ID token, and account per partition.True
, because of tenant profiles, this means there will be also be TokensPerPartition number of ID tokens and account cache items per partition. (So 3x number of cache items than above. The number of access tokens is still the same as above regardless of the flag.)Summary: The results show that partitioned cache performs better than flat cache (in some cases 1000x) and allocates less memory because only the data in the partition is filtered through instead of the whole cache.