handle neg expiry #1418
handle neg expiry #1418
Conversation
src/Microsoft.Identity.Web/TokenCacheProviders/InMemory/MsalMemoryTokenCacheProvider.cs
Show resolved
Hide resolved
| LoggerMessage.Define<string, string>( | ||
| LogLevel.Debug, | ||
| LoggingEventId.MemoryCacheNegativeExpiry, | ||
| "[MsIdWeb] {CacheType}: {Operation} The SuggestedCacheExpiry from MSAL was negative. "); |
There was a problem hiding this comment.
maybe the TokenCacheNotificationArgs.SuggestedCacheExpiry ?
| @@ -58,6 +58,12 @@ private static class Logger | |||
| LoggingEventId.MemoryCacheRead, | |||
| "[MsIdWeb] {CacheType}: {Operation} cacheKey {CacheKey} cache size {Size} "); | |||
|
|
|||
| private static readonly Action<ILogger, string, string, Exception?> s_l1CacheNegativeExpiry = | |||
| LoggerMessage.Define<string, string>( | |||
| LogLevel.Debug, | |||
There was a problem hiding this comment.
More like a LogLevel.Warning or Error?
There was a problem hiding this comment.
i don't think this error is actionable by the service.
There was a problem hiding this comment.
They should log a bug on MSAL. This field is only calculated on SaveMsalTokenResponse, so we assume there is a new access token incomming and no RTs. New ATs should always have a bit of time left on them.
| @@ -237,6 +237,11 @@ protected override async Task WriteCacheBytesAsync(string cacheKey, byte[] bytes | |||
| if (cacheSerializerHints != null && cacheSerializerHints?.SuggestedCacheExpiry != null) | |||
| { | |||
| cacheExpiry = cacheSerializerHints.SuggestedCacheExpiry.Value.UtcDateTime - DateTime.UtcNow; | |||
| if (cacheExpiry < TimeSpan.Zero) | |||
There was a problem hiding this comment.
Hmm, should you not delete the cache entry instead?
There was a problem hiding this comment.
we could but then the L1/L2 will be out of sync.
| // Assert | ||
| Assert.Equal(1, _testCacheAdapter._memoryCache.Count); | ||
| Assert.Single(L2Cache.dict); | ||
| await Task.Delay(1000).ConfigureAwait(false); |
There was a problem hiding this comment.
Generally we should avoid awaiting in tests. But I think in this case the adapter should just delete the entry itself or not set an expiry.
There was a problem hiding this comment.
what happens if MSAL expects that the entry was written and doesn't find it just after? I tried to have a void implementation of a serializer and that didn't work. MSAL crashed, it was expecting something to come back.
|
SonarCloud Quality Gate failed. |
| // Arrange | ||
| byte[] cache = new byte[3]; | ||
| AssertCacheValues(_testCacheAdapter); | ||
| Assert.Equal(0, _testCacheAdapter._memoryCache.Count); |
There was a problem hiding this comment.
nit: internal member naming is MemoryCache, not _memoryCache
#1419