GetOrSetAsync fails to write value to redis when any redis extensions are enabled

[ErikPilsits-RJW]

I'm trying a basic redis backed setup. SetAsync works, but GetOrSetAsync is not setting the value into redis. It is setting the value into memory however, and does return the expected value.

If I remove the redis extensions, then it works and does set the value into redis. However I can't really use redis without them, since I have scaled app services in azure. Note I need to remove both redis extensions, having either one will cause the problem.

Here's a basic reproducer I mocked up in linqpad.

var redisConnection = ConnectionMultiplexer.Connect("-azure-cnxn-string-");

var c = new CacheStack(
    new ICacheLayer[]
    {
        new MemoryCacheLayer(),
        new RedisCacheLayer(redisConnection, 0)
    },
    new ICacheExtension[]
    {
        new AutoCleanupExtension(TimeSpan.FromMinutes(5)),
        new RedisLockExtension(redisConnection, 0),
        new RedisRemoteEvictionExtension(redisConnection)
    });

// works
var result1 = c.SetAsync("setasync", "hello world", TimeSpan.FromHours(24)).GetAwaiter().GetResult();
// doesn't work
var result2 = c.GetOrSetAsync<string>("getorsetasync", old => Task.FromResult("hello world"), new CacheSettings(TimeSpan.FromMinutes(2))).GetAwaiter().GetResult();
result1.Dump();
result2.Dump();

[ErikPilsits-RJW]

Looking at the lock extension, seems the issue might be here. I think you're using the same cache key for the lock as the value, so it removes the value when it removes the lock. Still looking at why the remote eviction might also cause an issue.

var hasLock = await Database.StringSetAsync(cacheKey, RedisValue.EmptyString, expiry: LockTimeout, when: When.NotExists);

			if (hasLock)
			{
				try
				{
					var cacheEntry = await valueProvider();
					await Subscriber.PublishAsync(RedisChannel, cacheKey, CommandFlags.FireAndForget);
					return cacheEntry;
				}
				finally
				{
					await Database.KeyDeleteAsync(cacheKey, CommandFlags.FireAndForget);
				}

[ErikPilsits-RJW]

I think I confirmed the lock issue. If I set the lock extension to use a different redis database, then it works.

[ErikPilsits-RJW]

I think I've got the eviction issue narrowed down, though not totally sure. Either way, the Subscribe method doesn't take an async handler, it's a sync Action.

StackExchange/StackExchange.Redis#639

There's an OnMessage version of the subscriber that does take an async handler however, and could be used in place.

https://stackexchange.github.io/StackExchange.Redis/Basics#using-redis-pubsub

[ErikPilsits-RJW]

Figured it out. It's not the async handler, though maybe that should be fixed also?

The issue is the remote eviction itself. I actually had another instance of my app running on azure, and so it was subscribed as well. The remove eviction on the azure instance calls EvictAsync on the ICacheStack which evicts all the layers, including redis! I think the intention is that remote eviction should only evict the local caches.

[Turnerj]

Great work chasing up all of this - saves a lot of time debugging the issues!

Either way, the Subscribe method doesn't take an async handler, it's a sync Action.
...
There's an OnMessage version of the subscriber that does take an async handler however, and could be used in place.

Hmmm, I wonder if that may have been one of the causes of issues in my tests.

Figured it out. It's not the async handler, though maybe that should be fixed also?

Yeah, that should definitely be fixed. I find it strange that there is no compiler error or anything for how I currently have it set up.

The issue is the remote eviction itself. I actually had another instance of my app running on azure, and so it was subscribed as well. The remove eviction on the azure instance calls EvictAsync on the ICacheStack which evicts all the layers, including redis!

Wow - that is really bad, can't believe I missed that when writing it. Definitely need more tests around that scenario.

I think the intention is that remote eviction should only evict the local caches.

Yeah. When calling EvictAsync on an ICacheStack, that should evict it from everything BUT when a Redis remote eviction triggers it, it definitely should NOT evict from itself through a second consumer otherwise it will destroy the new value cached in Redis.

Gonna need to work out how I can best co-ordinate that logic. Likely it will be changing the following to not ask the cache stack to evict it directly - maybe will need an EvictLocalAsync method though that may cause other issues.

CacheTower/src/CacheTower.Extensions.Redis/RedisRemoteEvictionExtension.cs

Lines 64 to 67 in a3f9c98

	if (shouldEvictLocally)
	{
	await cacheStack.EvictAsync(cacheKey);
	}

[epilsits]

(Still me)
Maybe allow the user to register the layers they want evicted with the remote extension when constructed. That way the extension doesn't need to worry about the logic and what a user considers a distributed cache layer vs local. Also gives different apps which may share the cache a bit more flexibility.

[epilsits]

I think that issue with the lock extension still exists. The lock uses the same cache key as the value, so deleting the lock deletes the value, or on subsequent tries where the value might exist, would always fail to get a lock.

[epilsits]

Perhaps also take a look at incorporating redlock.net into this extension for the lock taking.

[Turnerj]

Thanks for reminding me about the locking issue! The fix is simple enough, allow an option to configure a lock key format - this is the same logic that Redlock.NET does.

Regarding using Redlock.NET entirely, it seems a little heavy for what it does and with the changes I've done, locking should work correctly now. If I do have more issues with locking etc in the future, I may reconsider it though.

[ErikPilsits-RJW]

Noticed the fixes have all been merged, awesome! Are you planning a nuget release this week?

[Turnerj]

Should be - just checking a few final things, want to make sure CI builds are stable.

[Turnerj]

I've released 0.7.0 with the Redis fixes. Thanks again for raising the issues, if you encounter any others, let me know!