Skip to content
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

L2 Cache retry initial commit #1046

Merged
merged 4 commits into from
Mar 7, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ protected override async Task RemoveKeyAsync(string cacheKey)
catch (Exception ex)
{
_logger.LogError($"[IdWebCache] Connection issue encountered with Distributed cache. Currently using In Memory cache only. Error message: {ex.Message} ");
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

Currently using In Memory cache only [](start = 101, length = 36)

Forgot to mention this last time...
Replace: Remove cache entry for (key) failed. Don't have to mention anything about the memory cache here.
(same comment a few other places)
#Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done & i agree...was more for testing too to see where we were.


In reply to: 587807465 [](ancestors = 587807465)


if (_distributedCacheOptions.OnL2CacheFailure != null && _distributedCacheOptions.OnL2CacheFailure(ex))
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

Good pattern to apply everywhere. Try/catch can be refactored into a method/generic which takes an action #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.


In reply to: 587808630 [](ancestors = 587808630)

{
_logger.LogDebug($"[IdWebCache] DistributedCache: Retry to remove cacheKey {cacheKey}. ");
await _distributedCache.RemoveAsync(cacheKey).ConfigureAwait(false);
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

await _distributedCache.RemoveAsync(cacheKey).ConfigureAwait(false); [](start = 20, length = 68)

have to catch this one as well #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. fixed.


In reply to: 587807756 [](ancestors = 587807756)

}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using Microsoft.Extensions.Caching.Distributed;

namespace Microsoft.Identity.Web.TokenCacheProviders.Distributed
Expand All @@ -18,6 +19,17 @@ public class MsalDistributedTokenCacheAdapterOptions : DistributedCacheEntryOpti
/// </summary>
public long L1CacheSizeLimit { get; set; } = 500;

/// <summary>
/// Callback offered to the app to be notified when the L2 cache fails.
/// This way the app is given the possibility to act on the L2 cache,
/// for instance, in the case of Redis, to reconnect. This is left to the application as it's
/// the only one that knows about the real implementation of the L2 cache.
/// The handler should return <c>true</c> if the cache should try again the operation, and
/// <c>false</c> otherwise. When <c>true</c> is passed and the retry fails, an exception
/// will be thrown.
/// </summary>
public Func<Exception, bool>? OnL2CacheFailure { get; set; }

/// <summary>
/// Value more than 0, less than 1, to set the In Memory (L1) cache
/// expiration time values relative to the Distributed (L2) cache.
Expand Down
13 changes: 13 additions & 0 deletions tests/WebAppCallsWebApiCallsGraph/Client/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.Extensions.Hosting;
using Microsoft.AspNetCore.Authentication.OpenIdConnect;
using Microsoft.Identity.Web.UI;
using Microsoft.Identity.Web.TokenCacheProviders.Distributed;

namespace WebApp_OpenIDConnect_DotNet
{
Expand Down Expand Up @@ -48,6 +49,18 @@ public void ConfigureServices(IServiceCollection services)
options.Configuration = Configuration.GetConnectionString("Redis");
options.InstanceName = "RedisDemos_"; //should be unique to the app
});
services.Configure<MsalDistributedTokenCacheAdapterOptions>(options =>
{
options.OnL2CacheFailure = (ex) =>
{
if (ex is StackExchange.Redis.RedisConnectionException)
{
// action: try to reconnect or something
return true; //try to do the cache operation again
}
return false;
};
});
#endif

services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
Expand Down