Shared multiplexer configuration between different Redis clients

[serhatkiyak]

We are using the latest version of the Nuget package (https://www.nuget.org/packages/StackExchange.Redis.Extensions.Core/5.0.2). We have logic to manage multiple Redis client instances for sharding. This works well with the obsolete StackExchangeRedisCacheClient type, however we noticed an issue when we updated the package and switched to use RedisDefaultCacheClient.

If we create multiple instances of this type, the configuration of the last one initialized overrides the configuration of all the existing clients. Here's a sample code to repro this issue:

using System;
using StackExchange.Redis.Extensions.Core.Abstractions;
using StackExchange.Redis.Extensions.Core.Configuration;
using StackExchange.Redis.Extensions.Core.Implementations;
using StackExchange.Redis.Extensions.MsgPack;

namespace RedisBugRepro
{
	class Program
	{
		static void Main(string[] args)
		{
			var endpoint = "endpoint-p1.redis.cache.windows.net";
			var redisClient = GetRedisClient(endpoint);

			// Print first client details (correct)
			Console.WriteLine($"Expected endpoint: {endpoint}");
			Console.WriteLine($"Configuration: {redisClient.Database.Multiplexer.Configuration}");
			Console.WriteLine();

			var endpoint2 = "endpoint-p2.redis.cache.windows.net";
			var redisClient2 = GetRedisClient(endpoint2);

			// Print second client details (correct)
			Console.WriteLine($"Expected endpoint: {endpoint2}");
			Console.WriteLine($"Configuration: {redisClient2.Database.Multiplexer.Configuration}");
			Console.WriteLine();

			// Print first client details again (configurion gets modified!!!)
			Console.WriteLine($"Expected endpoint: {endpoint}");
			Console.WriteLine($"Configuration: {redisClient.Database.Multiplexer.Configuration}");
			Console.WriteLine();

			Console.WriteLine("Press to continue");
			Console.Read();
		}

		static IRedisDefaultCacheClient GetRedisClient(string endpoint)
		{
			var redisConfiguration = new RedisConfiguration
			{
				Hosts = new[]
				{
					new RedisHost { Host = endpoint, Port = 6380 }
				},
				Password = "",
				Ssl = true,
				AbortOnConnectFail = false,
				ConnectTimeout = 800,
				SyncTimeout = 800,
			};

			var connectionPoolManager = new RedisCacheConnectionPoolManager(redisConfiguration);
			var serializer = new MsgPackObjectSerializer();
			var redisCacheClient = new RedisCacheClient(connectionPoolManager, serializer, redisConfiguration);
			return new RedisDefaultCacheClient(redisCacheClient);
		}
	}
}

and here's the output to the console of this program:

Expected endpoint: endpoint-p1.redis.cache.windows.net
Configuration: endpoint-p1.redis.cache.windows.net:6380,syncTimeout=800,allowAdmin=False,connectTimeout=800,ssl=True,abortConnect=False

Expected endpoint: endpoint-p2.redis.cache.windows.net
Configuration: endpoint-p2.redis.cache.windows.net:6380,syncTimeout=800,allowAdmin=False,connectTimeout=800,ssl=True,abortConnect=False

Expected endpoint: endpoint-p1.redis.cache.windows.net
Configuration: endpoint-p2.redis.cache.windows.net:6380,syncTimeout=800,allowAdmin=False,connectTimeout=800,ssl=True,abortConnect=False

Press to continue

As you can see in the last section, the configuration field of the first object gets updated after a second object initialized with a different configuration.

[imperugo]

Hi, thanks for the accurate feedback. I think that the reason behind your problem is that the concurrent bag (used in order to cache the connections) is static.

see here: https://github.com/imperugo/StackExchange.Redis.Extensions/blob/master/src/StackExchange.Redis.Extensions.Core/Implementations/RedisCacheConnectionPoolManager.cs#L11

Tomorow I'll try you solution to have a confirmation of my idea (I'm in mobility right now). If I'm correct the idea could be to handle the cache outside of RedisCacheConnectionPoolManager (as an option). I need to check better, in the meantime, if you have problems on production, you could create 2 classes of RedisCacheConnectionPoolManager and use one for the first server and the other one for the second.

In any case tomorrow I'll check better. Thanks

[Exocomp]

@imperugo I had the same use case and ran into the same issue.

you could create 2 classes of RedisCacheConnectionPoolManager and use one for the first server and the other one for the second

If I follow you correctly, I created two instances of RedisCacheConnectionPoolManager and two instances of RedisCacheClient each using the respective RedisCacheConnectionPoolManager instance. However, I see the same behavior as described initially.

[Exocomp]

@imperugo The issue is what you highlighted with ConcurrentBag object (RedisCacheConnectionPoolManager) being static. If you make it an instance then it resolves the issue in my testing.

Any idea why it was set to static? I'm not familiar with the code and not sure if that would introduce a side effect I'm not aware of.

[imperugo]

It's static because is service cache. if it was an instance cache and you register the service as transient, it will be initialized every time to try to use the cache.

For scenarios like your I've created IRedisCacheConnectionPoolManager, so you can inject your logic.

[Exocomp]

@imperugo

it was an instance cache and you register the service as transient, it will be initialized every time to try to use the cache

I agree if it was registered as transient, I guess it didn't cross my mind as I thought one would only want to register it as singleton (just my thoughts and use cases). Any specific reason why you would want it transient (other than the normal use cases)?

The IRedisCacheConnectionPoolManager is one solution but perhaps there can be some built in design that could cater for multiple singleton instances. Reason being it would create the connection pool manager logic in one place (in this library) for most cases unless IRedisCacheConnectionPoolManager really is needed. Meaning that in my case the connection pool logic is fine so technically there is no need for me to fork off with IRedisCacheConnectionPoolManager just to make the connections field instance instead of static. Thoughts?

[imperugo]

Hi @Exocomp

Any specific reason why you would want it transient (other than the normal use cases)?

Just to prevent that for an error (or because normal people don't read the documentation) register the service as transient and then open a bug :)

From my point of view I just leave opened the opportunity to use your custom caching layer just creating your IRedisCacheConnectionPoolManager . It doesn't seem wrong to me but I understand your point.

Let me think about an elegant solution in order to match your need and the reason why I did this in this way.

[gri4]

Hi @imperugo, sorry to disturb. I have the same issue as @Exocomp.. Any updates on this, please?

[msmolka]

It's static because is service cache. if it was an instance cache and you register the service as transient, it will be initialized every time to try to use the cache.

@imperugo but isn't it idea behind transient. Transient means every tiem separate instance - no cache. If I want cache I use singleton.

[imperugo]

Use RedisCacheConnectionPoolManager is silly because you have to create a new pool each time you create an instance of it (new pool, new connctions and so on). To prevent this the bag is static.

The good part is that you have an interface and you can implement your own logic if you don't like this. Another good part is that you can send a PR with your implementation and hope that it will be merged.