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

[Feature Request] Enable apps to decide what to do when the L2 cache fails (Retry) #1042

Closed
jmprieur opened this issue Mar 4, 2021 · 3 comments · Fixed by #1046
Closed
Labels
enhancement New feature or request fixed
Milestone

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Mar 4, 2021

Is your feature request related to a problem? Please describe.
Today, when the L2 cache fails, an error is logged, but given the L1 cache was able to proceed, we don't re-throw (think of Redis cache connection error)

Describe the solution you'd like

Have a new property in the MsalDistributedTokenCacheAdapterOptions, so that the application can handle the failure ?

 /// <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;}

Here is an example of how this would be hooked from the Startup.cs:

services.Configure<MsalDistributedTokenCacheAdapterOptions>(options =>
{
options.L1CacheSizeLimit = 10 * 1024 * 1024; // 10 Mb
options.OnL2CacheFailure = (ex) =>
{
  if (ex is StackExchange.Redis.RedisConnectionException)
  {
    // Attempt to act on the redis cache if at all possible?
    // Put here your reconnected code
    return true; // Retry
  } 
  return false;  // Don't retry.
 };
});

Describe alternatives you've considered
We could also decide of the behavior when it fails again on retry (we could be silent instead of throwing)

@jmprieur jmprieur added the enhancement New feature or request label Mar 4, 2021
@jmprieur jmprieur added this to the 1.8.0 milestone Mar 4, 2021
@jmprieur
Copy link
Collaborator Author

jmprieur commented Mar 4, 2021

Advice from Henrik:

  • Look on stack overflow about the Redis connection issues; etc ..
  • @henrik-me will make sure we have the right people to help

@jmprieur jmprieur changed the title [Feature Request] Enable apps to decide what to do when the L2 cache fails [Feature Request] Enable apps to decide what to do when the L2 cache fails (Retry) Mar 5, 2021
@jmprieur jmprieur linked a pull request Mar 5, 2021 that will close this issue
@jmprieur jmprieur reopened this Mar 8, 2021
@jmprieur jmprieur added the fixed label Mar 8, 2021
@jmprieur
Copy link
Collaborator Author

jmprieur commented Mar 8, 2021

Re opening until it's released.

@jennyf19
Copy link
Collaborator

Included in 1.8.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants