[RedisCache] Allow injection of ConnectionMultiplexer

[epignosisx]

Currently the RedisCache creates the ConnectionMultiplexer and does not allow access to it. This prevents the application from reusing the ConnectionMultiplexer for other operations like PubSub.

Granted, the application could create another ConnectionMultiplexer, but the recommendation is to have only one per application.

I was thinking if it would be possible to have a separate constructor for RedisCache that would receive the ConnectionMultiplexer. I understand that so far Microsoft.Extensions.Caching.Redis did not leak any implementation details about its usage of StackExchange.Redis. However, I noticed that Microsoft.AspNetCore.DataProtection.Redis leaks StackExchange.Redis types here. So, I'm unclear about how you feel about keeping StackExchange.Redis as an implementation detail.

Proposed API

namespace Microsoft.Extensions.Caching.StackExchangeRedis
{
    public class RedisCacheOptions
    {
+       Func<IConnectionMultiplexer> ConnectionMultiplexerFactory { get; set; }     
    }

[epignosisx]

Just noticed there is an open pull request enabling this scenario. But did not find an issue discussing the proposed change, so I'll keep this one open for discussion.

[muratg]

Closing aspnet/Caching#318 as it entails to a breaking change. Keeping this bug open for any alternatives or for a potential major release change.

[israellot]

I believe that injecting IConnectionMultiplexer as a dependency is the path to go.

I share @epignosisx concerns about StackExchange.Redis being a implementation detail or not.
The pragmatic way to address it would be have a service interface and ** StackExchange.Redis ** being used in a adaptor that implements that interface. But that would possibly mean too many levels of generalization though.

[epignosisx]

@muratg could you share where does the team stand on whether StackExchange.Redis should be kept as an implementation detail or not?

Answering this question is critical to make progress on this issue. As @israellot pointed out the alternatives are more abstractions that probably will be undesirable.

[muratg]

cc @davidfowl

[davidfowl]

Yes we should allow for this. Not sure about injecting the IConnectionMultiplexer as a dependency though we should have an option that allows setting a Func<IConnectionMultiplexer>

[JadynWong]

I know the application has created a ConnectionMultiplexer.
Why the application must be create another ConnectionMultiplexer?

[muratg]

Tentatively putting this in 2.2.0. We'll need to identify what the best way to achieve this is.

[schneiBoy]

FYI this issue is referenced here:

https://stackoverflow.com/questions/49403973/net-core-idistributedcache-redis-sentinel-support-for-master-slave/49931794#49931794

[shravan2x]

@muratg Any update to this?

[ekarlso]

Over a year old and nothing new ?

[muratg]

This is not currently in scope. We'll revisit in a future release.

cc @DamianEdwards @davidfowl

[effyteva]

+1
Perhaps a much easier solution would be to allow getting the current Multiplexer? This way Microsoft.Extensions.Caching.Redis would create the Multiplexer instance, and any external code would use the same instance.

[shravan2x]

@effyteva That would become a problem if other libraries did the same thing - everyone would create their own multiplexer and expect others to use theirs.

[kinosang]

For Data Protection we can use .PersistKeysToStackExchangeRedis(ConnectionMultiplexer, RedisKey), how about adding an overloading such as AddStackExchangeRedisCache(ConnectionMultiplexer, InstanceName)?

[hjkl950217]

I also encountered the same problem, for some redis data, I need to use IConnectionMultiplexer

[bidianqing]

@muratg Any update to this?

[muratg]

I'm no longer working with the ASP.NET team, so paging @anurse. @DamianEdwards and @davidfowl here.

[analogrelay]

We don't have plans to do this for 3.0, but it is on our backlog to consider for a future version.

[bidianqing]

https://github.com/dotnet-architecture/eShopOnContainers/blob/dev/src/Services/Basket/Basket.API/Startup.cs#L112

[bragma]

Hi, it seems this feature won't be available anytime soon. Is there a workaround for this? I need to use the redis cache both via IDistributedCache and with native redis commands. Thanks!

[analogrelay]

@bragma I don't disagree at all that it would be a good plan for us to allow injecting it, but can you provide more context on why you require that your native redis commands and the redis cache use the same instance of the ConnectionMultiplexer?

[kinosang]

@anurse I think that's why there's ConnectionMultiplexer.

• https://stackexchange.github.io/StackExchange.Redis/Basics.html

Because the ConnectionMultiplexer does a lot, it is designed to be shared and reused between callers. You should not create a ConnectionMultiplexer per operation. It is fully thread-safe and ready for this usage.

[epignosisx]

@bragma the workaround I have ended up using is getting a hold of the ConnectionMultiplexer via private reflection 😨 . This is basically what I have:

    public static class RedisCacheExtensions
    {
        /// <summary>
        /// The ASP.NET Core's RedisCache does not expose the ConnectionMultiplexer required for more advanced Redis scenarios
        /// and it is recommended to have just one ConnectionMultiplexer.
        /// We are left with no option but to use reflection to get a hold of the connection.
        /// </summary>
        public static async Task<ConnectionMultiplexer> GetConnectionAsync(this RedisCache cache, CancellationToken cancellationToken = default)
        {
            //ensure connection is established
            await((Task)typeof(RedisCache).InvokeMember("ConnectAsync", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.InvokeMethod, null, cache, new object[] { cancellationToken }));

            //get connection multiplexer
            var fi = typeof(RedisCache).GetField("_connection", BindingFlags.Instance | BindingFlags.NonPublic);
            var connection = (ConnectionMultiplexer)fi.GetValue(cache);
            return connection;
        }
    }

Usage:

    public class RedisHealthCheck : IHealthCheck
    {
        private readonly RedisCache _cache;

        public RedisHealthCheck(IDistributedCache cache)
        {
            _cache = (RedisCache)cache;
        }

        public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default)
        {
            try
            {
                var connection = await _cache.GetConnectionAsync(cancellationToken);
                await connection.GetDatabase()
                    .PingAsync();

                return HealthCheckResult.Healthy();
            }
            catch (Exception ex)
            {
                return new HealthCheckResult(context.Registration.FailureStatus, exception: ex);
            }
        }
    }

[analogrelay]

I think that's why there's ConnectionMultiplexer.

Yep, I understand the purpose. Just trying to understand what issues you're seeing with using two separate instances to help prioritize. Is there extra load in your system? (memory usage, cpu, etc.)

We'll definitely be investigating this, but I just want to understand how this issue is negatively impacting applications right now in order to help us prioritize.

[bragma]

@bragma I don't disagree at all that it would be a good plan for us to allow injecting it, but can you provide more context on why you require that your native redis commands and the redis cache use the same instance of the ConnectionMultiplexer?

Well, the context is more or less the same as others requesting for it. I need to stuff some data in the cache preserving compatibility with other services reading it via IDistributedCache and also use all other nice Redis data types for other purposes. Of course I could just ditch the IDistributedCache implementation and use only the Redis package replicating the data format, but any change in the implementation of distributed cache for redis would require me to change my code.
About using 2 different instances, I have no proof it will cause problems, yet it seems a waste of resources to me. I am no expert but 2 different contexts each ignorant of the other will at lease cause double connections/threads.
I also agree it is not a priority, that's why I am asking for a kind of workaround instead of an implementation. I am not expert enough to make up mine, so I am asking:

1. Is it possible and what's the correct way to create a singleton ConnectionMultiplexer and use it in both RedisCache and directly?
2. If not, what's the bast way to create a privatec ConnectionMultiplexer and a RedisCache without having both interfere?

Thanks a lot!

[analogrelay]

About using 2 different instances, I have no proof it will cause problems, yet it seems a waste of resources to me. I am no expert but 2 different contexts each ignorant of the other will at lease cause double connections/threads.

This isn't necessarily wrong, but it would probably be best to have actual data to know the impact of that.

I don't have a specific workaround for you unfortunately. The main issue here is that the cache doesn't give you a way to specify the multiplexer, which isn't easy to work around. Honestly, I would investigate the impact of two separate ConnectionMultiplexer instances is on your actual app and see if that's a sufficient workaround here.

[bragma]

I was wondering an idea, which may be totally naive and stupid, so I am exposing it for comments. Suppose RedisCache is changed by adding something like:

        public IDatabase GetRedisDatabase()
        {
            Connect();
            return _cache;
        }

        public async Task<IDatabase> GetRedisDatabaseAsync()
        {
            await ConnectAsync();
            return _cache;
        }

And an interface like this is added:

    public interface IDistributedRedisCache : IDistributedCache
    {
        IDatabase GetRedisDatabase();
        Task<IDatabase> GetRedisDatabaseAsync();
    }

This would allow to create RedisCache as a singleton for a IDistributedRedisCache and IDistributedCache would still work, I suppose.

It seems like a minor change to me. I am missing anything?

Thanks a lot!

[epignosisx]

I like the idea of RedisCache exposing public methods to get the inner workings, however:

• For ultimate flexibility expose the ConnectionMultiplexer instead of IDatabase.

• The additional interface IDistributedRedisCache is leaking StackExchange.Redis types, so it is not much of an abstraction. Something like ServiceStack.Redis would not be able to implement it. Probably better to leave this interface as an application concern to facilitate unit testing instead of having it in the framework.

• Still I think constructor injection is needed. I can imagine use cases needing to connect to Redis during startup and then just passing the initialized ConnectionMultiplexer to RedisCache during DI configuration.

[davidfowl]

@napalm684 I've updated the original issue with an API proposal. We'll review the API, you can prepare a PR in the meantime or wait for final approval.

[NapalmCodes]

@davidfowl I guess I'm a little confused what the factory provides when the property is already private on that class if I recall correctly. Is there an example of this I could reference?

[davidfowl]

@davidfowl I guess I'm a little confused what the factory provides when the property is already private on that class if I recall correctly. Is there an example of this I could reference?

The options could have a ConnectionMultiplexer instance instead of a Func<ConnectionMultiplexer> (if that's what people need). I assume the use case for this is to potentially reduce the number of connections to redis in the application, so we'd also need a way to know if we should be making a connection or not.

[jjxtra]

Putting the burden on the application to create and pass a connection multiplexer in the options would add un-needed complexity. Now the client has to know how to create this type properly and connect, which can be error prone if not done right.

Being able to inject IConnectionMultiplexer into a constructor automatically should be all that is needed, since the Microsoft frameworks already have this created under the hood. The public interface of the nuget packages need not change, since they already reference the stack exchange nuget.

[davidfowl]

They can inject the multiplexer trivially if that's what they want to do and apply it to the options. We generally don't like assuming the services that are in the container should be automagically consumed by components, especially when they are optional. The API above is the standard pattern introduced in all extensions and ASP.NET Core APIs for configurability.

[NapalmCodes]

Is there a process to take this? I would like to make a PR if someone hasn't already.

[davidfowl]

We're gonna discuss the API next week and then you can send the PR

[NapalmCodes]

Awesome will you post the result of the discussion here and then I can submit?

[NapalmCodes]

So I took a stab in this PR #31879 . I would like some feedback (whether I did this properly - first timer here) and am willing to assist in whatever way necessary to apply feedback.

[speterson-zoll]

We would benefit from this fix. Can someone review PR #31879?

[NapalmCodes]

@davidfowl any word on the api design discussion? If things didn't change my PR is still out there. Otherwise let's talk about updates/feedback? Thanks!

[pranavkm]

Additional API feedback provided in the PR

[NapalmCodes]

Additional API feedback provided in the PR

I commented in the PR @pranavkm let's talk about a few things please.

[NapalmCodes]

With the merge of PR #32266 can this be closed?

[JunTaoLuo]

Yup, thanks for the contribution @napalm684 !

[RehanSaeed]

I've registered IConnectionMultiplexer in IoC. The Hot Chocolate GraphQL server allows you to resolve IConnectionMultiplexer from IoC like so:

servoces.AddSingleton<IConnectionMultiplexer>(ConnectionMultiplexer.Connect("redis:6379"));
builder.AddRedisQueryStorage(x => x.GetApplicationServices().GetRequiredService<IConnectionMultiplexer>().GetDatabase());
builder.AddRedisSubscriptions(x => x.GetRequiredService<IConnectionMultiplexer>());

I've also raised an issue in Xabril health checks to do something similar in Xabaril/AspNetCore.Diagnostics.HealthChecks#750:

services
    .AddHealthChecks()
    .AddRedis(sp => sp.GetRequiredService<IConnectionMultiplexer>());

Can this be a supported scenario for the IDistributedCache also? Setting up Redis in three different ways is not ideal.