[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 https://github.com/aspnet/Caching/pull/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.

[analogrelay]

Also, it's non trivial to register a singleton implementation for two different service types (without getting two different singleton instances). It's doable but not trivial.

I think injecting the multiplexer in to the redis cache is a much more likely thing for us to do. We've done it in other places that use Redis.

If someone wanted to add something similar to the RedisCacheOptions, we'd accept that contribution. It's important we are careful to understand how it interacts with the existing Configuration and ConfigurationOptions settings though.

[jjxtra]

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.

Agreed. Looks like it's reflection ftw...

[jjxtra]

So is something as simple as adding "RegisterConnectionMultiplexer" in the options as a boolean (default false) which will add the connection multiplexer to services not backwards compatible?

[jjxtra]

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!

This would work great, we could cast IDistributedCache to IDistributedRedisCache :)

[skironDotNet]

I also like the idea of exposing IDatabase or at least make it public property of IDistributedCache? Basically I want to get list of keys, or check if key exists, and Redis allows this, but there is no way to write extension method for IDistributedCache because there is no access to underlining redis client.

Also @epignosisx provided a code snippet _cache = (RedisCache)cache; getting underlying cache manager via casting feels wrong, because today IDistributedCacheimplementation uses RedisCachebut if to change in the future, anybody who casting(RedisCache)IDistributedCache would be in trouble, thus I wish IDistributedCache exposed IDatabase for extnsibility

[bladepan]

I would like to share another motivation for this change. We have logging, metrics, retry and other decorators built using the IDatabase interface. We want RedisCache to use our factory methods to create these decorated IDatabase objects. Exposing IDatabase objects from RedisCache does not help here.

Allowing RedisCache to accept a connection factory provides most flexibility. You can control how many connection instances the application creates and how these connections are created.

[adamhathcock]

I guess this missed the 5.0 timeframe?

I'd like this to enable usage of a pool of multiplexers as recommended in https://stackexchange.github.io/StackExchange.Redis/Timeouts

[adamhathcock]

I created https://github.com/dotnet/extensions/pull/3473 to show what I'd like to see. Basically similar to the first fix but no caching on RedisCache

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[NapalmCodes]

This is needed badly. If you use Redis with OIDC as the ticket store IDistributedCache is required. However, if you then need to run on multiple machines in a non-sticky load balanced situation, then you are forced to use data protection sharing via PersistKeysToStackExchangeRedis which takes a ConnectionMultiplexer and is only accessible via private reflection as mentioned above. Can we at least get some parity/consistency? If Data Protection functionality for Redis cannot use IDistributedCache then we need a way to get the multiplexer out of it in order to follow best practices and use the connection as a singleton.

[jjxtra]

I just don't understand the hesitation to expose IConnectionMultiplexer or an IConnectionMultiplexerProvider to the service collection - as long as stack exchange redis is a dependent nuget package this seems safe. If for some reason a new redis provider or nuget was used, then there should be a compile error, easy to detect the problem at build time.

[davidfowl]

I think we'd make it an option instead, that seems simple enough. Make a formal API proposal and we can take it through the process.

[jjxtra]

I'm happy to do this, where do I submit the api proposal?

[alrz]

Linking to my comment in a dup https://github.com/dotnet/aspnetcore/issues/30342#issuecomment-783144405

I really do wish a full-blown API for this. IConnectionMultiplexer injection would do but it would be nice to make it a selfcontained API.

[davidfowl]

I'm happy to do this, where do I submit the api proposal?

The API proposal template is in the new issue template. The proposal should have an addition to RedisOptions that adds the connection multiplexer, or more likely a factory for one. The rest of the template should be straight forward.

[jjxtra]

The full blown api piece is tricky. The connection multiplexer and the database it exposes have a TON of methods. What most people want is just direct access to redis through the stackexchange api. So the lowest hanging fruit by far is to just provide an option to expose that directly.

[alrz]

Good thing about HttpClientFactory API that I mentined is that it manages all the instances by itself.

I suppose this still doesn't resolve excessive number of connections where redis can't handle. 8 workers in 4 servers each making a connection brings redis down.

[jjxtra]

I feel like this issue represents the feature. As far as an API, I would think just injecting an IConnectionMultiplexerProvider by default would not hurt anything. If someone never asks for it, they never know about it. No need to add an option to turn this off or on.

public interface IConnectionMultiplexerProvider
{
    // the implementation would do the right thing and ensure a singleton connection multiplexer is returned, or
    //  throw an exception if the connection multiplexer is null and not yet able to connect
    public IConnectionMultiplexer GetConnectionMultiplexer();
}

[davidfowl]

I don't think that should be the API. Why not just modify the existing options object like so:

public class RedisCacheOptions
{
+   Func<IConnectionMultiplexer> ConnectionMultiplexerFactory { get; set; }
}

[jjxtra]

How would this hook into the internal guts of the Microsoft and Stackexchange nugets? The whole point is for the provider/factory to be implemented by Microsoft and available if asked for. Then no configuration changes are needed. If the factory is never asked for or injected, it will never be created and behavior will remain same as it is today.

[davidfowl]

if the factory is null (which it will be by default) then it won't be used. Was that the question?