RedisRemoteEvictionExtension evicts key on SetAsync

[mgoodfellow]

Hi,

Thanks for the great library.

Maybe I'm missing something but I have come across an issue that is confusing me:

Consider:

var layers = new ICacheLayer[]
                {
                    new RedisCacheLayer(cacheLayerMultiplexer, databaseIndex: 1),
                };

var myCacheStack = new CacheStack(
                layers,
                new ICacheExtension[]
                    {
                        new RedisRemoteEvictionExtension(
                            cacheLayerMultiplexer,
                            new ICacheLayer[]{})),    // Do not evict any layers for this example
                    });

// Add something to cache stack
await myCacheStack.SetAsync("hello:world", 123, TimeSpan.FromMinutes(5));

// Check redis, no key found

var result = await myCacheStack.GetAsync<int>("hello:world");

if (result == null)
{
      Console.WriteLine("No cache entry");
}

I wasn't sure on the setup, originally I also had a MemoryCacheLayer as well, and I figured you would pass layersToEvict as:

layersToEvict = layers.Where(x => x.GetType() != typeof(RedisCacheLayer)).ToArray();

And pass this into the RedisRemoteEvictionExtension - however, whatever method I use to configure this extension, it seems to evict the key I add to redis, from redis, the moment its added.

Let me know if I have misunderstood something here! Thanks!

[Turnerj]

Hey! Thanks for raising this issue and will look into it in further detail tomorrow.

One quick thing though, you've got two different cache keys in your example. Is that just a typo in your example?

[mgoodfellow]

Sorry, that's a typo from me copying and pasting between my example code and here, I thought "hello:world" was more internet. Very good spot though :) Will edit now

Just to confirm that removing the RedisRemoteEvictionExtension from the configuration leads to the expected result of both the key appearing in redis on set, and the call to GetAsync returning not null.

Thanks!

[mgoodfellow]

Apologies, I just rebuilt a new test in an isolated environment to be completely sure and there isn't an issue.

I run a server farm with many services, and one of the components had:

   var layers = new ICacheLayer[]
                {
                    new RedisCacheLayer(cacheLayerMultiplexer, databaseIndex: 1),
                };

      var myCacheStack = new CacheStack(
                layers,
                new ICacheExtension[]
                    {
                        new RedisRemoteEvictionExtension(
                            cacheLayerMultiplexer,
                            layers),
                    });

i.e. it wasn't ignoring the redis layer like in my first example.

As my test program was running against the test environment, every time I added a key, another service was the one removing it.

Anyway, this leads to an interesting issue, the RedisRemoteEvictionExtension should really never be allowed to run against the RedisCacheLayer otherwise it will self purge in a server farm scenario.

I wonder if the extension shouldn't protect against this?

A user could be fooled into thinking there isn't an issue when running one instance of a service - in this case, the RedisRemoteEvictionExtension protects against purging the RedisCacheLayer (if it was passed in) , however if they were to then run another instance, the behaviour of the cache changes, and the other instance will "self" purge from the RedisCacheLayer

[Turnerj]

First, great work finding that - was scratching my head about how that even happens.

Second, I like that idea. One thing difficult with the idea is RedisCacheLayer is in a separate library to the Redis extensions so I can't check by looping then checking cacheLayer is RedisCacheLayer. I have been thinking about moving the types back into a single library (which would allow this) but the path to move users to it might be a bit of a pain (plus the library and package name may need updating).

Let me think about it a bit more and will look at the best way forward.

As an aside to this, how else has your experience been with the library? Any suggestions from a use standpoint?

[Turnerj]

This goes for documentation too - any suggestions there from your experience using the library would be helpful!

[mgoodfellow]

As an aside to this, how else has your experience been with the library? Any suggestions from a use standpoint?

I stumbled across the library in a blog post a while ago actually - at the moment we use it in a relatively anemic way - we only use GetAsync and SetAsync.

Using the GetOrSetAsync functionality has been on my todo list for an embarrassingly long time (coming up a year actually). Although there is the functionality for providing a context, the complexity comes from actually leveraging this in a clean way.

So as an example for us, we leverage SimpleInjector as our DI, and NHibernate as our ORM. The DI will construct a DB session as required within a scope (e.g. a web request), but its actually controlled by the code (e.g. AsyncScopedLifestyle.BeginScope(context.Container)). So within the refresh function, you need to create a container scope, and then fetch what you need from a container. This ends up meaning that the context itself is the container, which in turn starts to feel like a dirty service locator pattern.

Then the next issue for us was that often you have a case of a repository which you want to cache the result from. This now means you have a repository, with an injected DB session (from the scope you are using it in) and then refresh function is creating a scope within that and has it's own injected session but fetched from the injected service locator, and things get really obtuse really fast.

Anyway, I'm sure there is a more graceful way of deal with these "out of band" updates, but I haven't had the time to try to come up with a more graceful design.

In short, it means we often just Get/Set as required without the cool refresh features.

We also originally were using multiple layers (Memory & Redis), but as it stands we decided just to use the Redis layer as that has the TTL built in and it always felt a tad dirty having a clean up function running, and in our use case where we lean on Redis (which is local to our VPC) I'm not sure having in memory was adding any extra performance.

This goes for documentation too - any suggestions there from your experience using the library would be helpful!

I think from this perspective adding the Redis setup examples would be good. The RedisLockExtension has RedisLockOptions and it isn't clear from a first glance what the configurable options are for. Also, with this RedisRemoteEvictionExtension I think it needs to be made clear that the layers you want to evict from will not be Redis in general.

Overall love the library, and I like the fact you have taken so much care designing it, and making sure it's fully tested.

[mgoodfellow]

In fact, now I'm thinking about it, I think GetOrSetAsync should have a version without background refresh. That would solve a lot of pain.

That would remove the need to perform a Get - Check - Set flow, and also remove the complexity of these background refreshes being run out of context/scopes.

[mgoodfellow]

I just had a chance to dig back, this breakage actually came about between 0.71 (didn't have the issue) and 0.8 (does):

#138

This PR introduces this issue while solving others!

[Turnerj]

Using the GetOrSetAsync functionality has been on my todo list for an embarrassingly long time (coming up a year actually).

I strongly recommend it! One of the biggest benefits is locking to prevent race conditions where multiple calls to the getter/value factory happen at once. This can be extended to locking web farm/cluster wide via the RedisLockExtension.

Although there is the functionality for providing a context, the complexity comes from actually leveraging this in a clean way.

So as an example for us, we leverage SimpleInjector as our DI, and NHibernate as our ORM. The DI will construct a DB session as required within a scope (e.g. a web request), but its actually controlled by the code (e.g. AsyncScopedLifestyle.BeginScope(context.Container)). So within the refresh function, you need to create a container scope, and then fetch what you need from a container. This ends up meaning that the context itself is the container, which in turn starts to feel like a dirty service locator pattern.

Ahhh, yes, I can see that being a bit weird! If I understand it right though, you might be able to use this:

CacheTower/src/CacheTower/ServiceCollectionExtensions.cs

Lines 45 to 50 in 3ac8eb7

	public static void AddCacheStack<TContext>(this IServiceCollection services, Func<TContext> contextFactory, ICacheLayer[] layers, ICacheExtension[] extensions)
	{
	services.AddSingleton<ICacheStack<TContext>, CacheStack<TContext>>(sp => {
	return new CacheStack<TContext>(contextFactory, layers, extensions);
	});
	}

The "context factory" here is a function that is called every time the "getter" is called in GetOrSetAsync:

CacheTower/src/CacheTower/CacheStack{TContext}.cs

Lines 42 to 46 in 3ac8eb7

	return await GetOrSetAsync<T>(cacheKey, async (old) =>
	{
	var context = ContextFactory();
	return await getter(old, context);
	}, settings);

I think this would help work around that issue.

Then the next issue for us was that often you have a case of a repository which you want to cache the result from. This now means you have a repository, with an injected DB session (from the scope you are using it in) and then refresh function is creating a scope within that and has it's own injected session but fetched from the injected service locator, and things get really obtuse really fast.

Hmmmmm. I kinda understand what your saying here but yeah, not sure what a fix would be for that.

We also originally were using multiple layers (Memory & Redis), but as it stands we decided just to use the Redis layer as that has the TTL built in and it always felt a tad dirty having a clean up function running, and in our use case where we lean on Redis (which is local to our VPC) I'm not sure having in memory was adding any extra performance.

Yeah - the best layer combinations for a system will really depend on what the system is doing, how its accessed and what is cached. What I've used as a rule-of-thumb before is frequently accessed data is best to still have locally in-memory simply to avoid even the marginal overhead of transferring data back-and-forth between Redis (even if it runs on the same machine).

With the in-memory cleanup - that is automatically handled when you use the AutoCleanupExtension. The reason for this (rather than building it internally) is that file-based and database caching both need it too so it seemed like a better ideal to have a central cleanup timer rather than maintaining one per cache layer.

With only using Redis though, you won't actually need the RedisRemoteEvictionExtension. That is designed to communicate to other web servers to clear the cache key from their local caches (eg. in-memory, file-based etc).

I think from this perspective adding the Redis setup examples would be good. The RedisLockExtension has RedisLockOptions and it isn't clear from a first glance what the configurable options are for. Also, with this RedisRemoteEvictionExtension I think it needs to be made clear that the layers you want to evict from will not be Redis in general.

That's great feedback - thanks for that! I'll open an issue to specifically track improving the documentation (both in the readme and inline) about using the Redis extensions.

Overall love the library, and I like the fact you have taken so much care designing it, and making sure it's fully tested.

Thanks! It started as a spur-of-the-moment thing after reading a blog post about multilayer caching and I wasn't really happy with the caching solutions I saw. While a lot of the core of what I want is here, there are still some big changes coming up including changes to the serialization (see #158), logging support (#150 ) and further tweaks to usability and performance. Thinking of adding a fluent-like interface for configuring a CacheStack for DI too.

In fact, now I'm thinking about it, I think GetOrSetAsync should have a version without background refresh. That would solve a lot of pain.

That would remove the need to perform a Get - Check - Set flow, and also remove the complexity of these background refreshes being run out of context/scopes.

Good news - that does already exist! Technically background refreshes only happen if the cache entry is stale. In the latest version of CacheTower, you would need to specify a staleAfter equal to or greater than the timeToLive. In the next version (to be released soon), you will be able to just specify a timeToLive (staleAfter will be null) and no background refreshing will happen at all.

For the most part, you can avoid needing a context at all. I'll probably need to add an extra AddCacheStack extension for DI services to make it easier to do but that isn't a big deal on my end.

Even if you don't now, I do recommend to look into background refreshing in the future. Being able to pre-emptively refresh a cache item without stalling the cache access can be very useful for high performance scenarios.

Hope that helps!

[Turnerj]

I just had a chance to dig back, this breakage actually came about between 0.71 (didn't have the issue) and 0.8 (does):

Yeah - that issue definitely exposes the problem in your use case with RedisCacheLayer being in the layers for RedisRemoteEvictionExtension.

[mgoodfellow]

The "context factory" here is a function that is called every time the "getter" is called in GetOrSetAsync:

We use SimpleInjector as our IoC which is extremely opinionated on the "correct" way to do things, but that means we don't use the Microsoft DI extensions for their service collection. Obviously I can make a registration similar to yours, but a simple factory doesn't cut it when we need additional DI in the "getter".

This is why I ended up with the entire container being "the context", and then the "getter" needing to manage scope.

return await _cacheStack.GetOrSetAsync<IEnumerable<int>>(
                key,
                async (old, context) =>
                {
// Eeek!
                    using (AsyncScopedLifestyle.BeginScope(context.Container))
                    {
                        var innerStatsClient = context.Container.GetInstance<IStatsClient>();
                        var innerSession = context.Container.GetInstance<ISession>();

                        using (innerStatsClient.StartTimer(
                            $"some.timer.key")
                        )
                        {
                            return await innerSession.Query<SomeData>().ToListAsync();
                        }
                    }
                },
                new CacheSettings(TimeSpan.FromDays(1), TimeSpan.FromMinutes(30)));

I think it would be good to plug into DI containers so that the factory can construct a complex context for the "getter".

We use an awesome library for backend job processing (Hangfire), and it allows different IoC containers to be plugged in for "job activation": https://github.com/devmondo/HangFire.SimpleInjector/blob/master/src/HangFire.SimpleInjector/SimpleInjectorJobActivator.cs

When you are bootstrapping the configuration:

         services.AddHangfire(
                x => x.UseStorage(
                        new MySqlStorage(
                            Configuration.GetConnectionString("HangfireConnection"),
                            new MySqlStorageOptions
                                {
                                    InvisibilityTimeout = TimeSpan.FromHours(2),
                                    JobExpirationCheckInterval = TimeSpan.FromHours(2),
                                    TablesPrefix = "Hangfire_"
                                }))
// The key bit is here, you can replace the activator
                    .UseActivator(new SimpleInjectorJobActivator(Container)));

The point being, you can manage the scope of the container while you construct objects. If I was to create a scope as part of Func<ContextFactory> it wouldn't be defined as to when it would be cleaned up, so that leads to needing the getter to setup the scope as required.

Now we could have a specific context for something:

public class SessionContext 
{
   public ISession Session {get;}
   // Some other things I need

   public SessionContext(ISession session) 
   {
      Session = session;
      // etc
   }
}

then later:

return await _cacheStack.GetOrSetAsync<IEnumerable<int>>(
                key,
                async (old, context) =>
                {
                        using (context.StatsClient.StartTimer($"some.timer.key"))
                        {
                            return await context.Session.Query<SomeData>().ToListAsync();
                        }
                    }
                },
                new CacheSettings(TimeSpan.FromDays(1), TimeSpan.FromMinutes(30)));

The "getter" would deal with the activator scope, then ask for the context to be "activated" / "constructed" via container and then it is available in the getter.

Apologies if I have gone off in the deep end, as I say I was toying with this June last year, and I haven't really sat down and fully formulated the idea, so sorry if its all a bit half-baked.

[mgoodfellow]

Good news - that does already exist! Technically background refreshes only happen if the cache entry is stale. In the latest version of CacheTower, you would need to specify a staleAfter equal to or greater than the timeToLive. In the next version (to be released soon), you will be able to just specify a timeToLive (staleAfter will be null) and no background refreshing will happen at all.

This is awesome news, thank you!

For the most part, you can avoid needing a context at all. I'll probably need to add an extra AddCacheStack extension for DI services to make it easier to do but that isn't a big deal on my end.

Our current usage of it is purely without the context:

container.RegisterInstance<ICacheStack>(myCacheStack);

[Turnerj]

Yeah, I'm familiar with Hangfire and that job activator - had to write my own custom one to support additional data being sent to Sentry. If I understand what you're saying correctly, basically with types like:

public interface ICacheContextActivator
{
    ICacheContextScope BeginScope();
}

public interface ICacheContextScope : IDisposable
{
    T Resolve<T>();
}

And the CacheStack<TContext>.GetOrSetAsync to internally go:

return await GetOrSetAsync<T>(cacheKey, async (old) =>
{
    using var scope = CacheContextActivator.BeginScope();
    var context = scope.Resolve<TContext>();
    return await getter(old, context);
}, settings);

Something like that sounds like it would solve it for you then right? The scope is properly disposed and within the scope the actual context type is resolved.

In theory I should be able to do something like this in a backwards compatible way too by having a custom implementation that resolves for a Func<TContext> like I currently support. Personally, it would be nicer if I could simplify it to one interface and do it in one method call but that won't make the disposable scope work properly.

What are your thoughts on this?