-
-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RedisRemoteEvictionExtension evicts key on SetAsync #163
Comments
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? |
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 Thanks! |
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:
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 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 |
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 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? |
This goes for documentation too - any suggestions there from your experience using the library would be helpful! |
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 Using the 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. 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.
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 Overall love the library, and I like the fact you have taken so much care designing it, and making sure it's fully tested. |
In fact, now I'm thinking about it, I think 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. |
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): This PR introduces this issue while solving others! |
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
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
The "context factory" here is a function that is called every time the "getter" is called in CacheTower/src/CacheTower/CacheStack{TContext}.cs Lines 42 to 46 in 3ac8eb7
I think this would help work around that issue.
Hmmmmm. I kinda understand what your saying here but yeah, not sure what a fix would be for that.
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 With only using Redis though, you won't actually need the
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.
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
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 For the most part, you can avoid needing a context at all. I'll probably need to add an extra 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! |
Yeah - that issue definitely exposes the problem in your use case with |
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.
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:
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 Now we could have a specific context for something:
then later:
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. |
This is awesome news, thank you!
Our current usage of it is purely without the context:
|
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 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 What are your thoughts on this? |
That would be perfect. This then enables any IoC container to be implemented in conjunction with this library. You managed to describe it a lot more succinctly than I did, I think I need another coffee in the morning before trying to outline an idea :) |
Awesome! I'll bring this out to a separate issue to track and hopefully have something like I've described above merged in the next few days. It will still be a little bit longer before a new release is out with the change as there are some more serialization bits I need to tackle before then. |
Amazing, thank you! All my excuses about not using background refresh will have gone and I will move that back forward in my issue tracker ;) |
That's great to hear! The feedback you've provided will definitely help the library be better. For every person like yourself that raises these quality-of-life issues with the library, there are likely many others that don't and simply use something else. |
Forgot to close this one - with v0.12.0, there is now specific |
Hi,
Thanks for the great library.
Maybe I'm missing something but I have come across an issue that is confusing me:
Consider:
I wasn't sure on the setup, originally I also had a MemoryCacheLayer as well, and I figured you would pass layersToEvict as:
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!
The text was updated successfully, but these errors were encountered: