VahidN / EFSecondLevelCache.Core

Entity Framework Core Second Level Caching Library
Apache License 2.0
326 stars 51 forks source link

Cache not invalidating after .Remove #36

Closed jasenf closed 5 years ago

jasenf commented 5 years ago

Summary of the issue

When running this test, the object is put in the cache successfully, but after being deleted, the following query is still retrieving the value from the cache.

I see the InvalidateCacheDependencies being called within SaveChanges properly, and I see dependenciesCacheManager.Remove(rootCacheKey) being called as well. (where rootCacheKey is being set to "SC.Core.Entities.Analytic"

 // Create a new object w/ some test data..
 SC.Core.Entities.Analytic a = new Analytic();
 a.ApplicationID = 4171;
 a.EmailsSent = 1;

// Save it
_context.Analytics.Add(a);
_context.SaveChanges();

// Retrieve it.   DB Call is made and object is cached.
 var ab = _context.Analytics.Cacheable(debugger).FirstOrDefault(x=>x.ID==a.ID);

// Delete it from DB
 _context.Analytics.Remove(a);
_context.SaveChanges();

// Test retrieving from cache.  Variable is INCORRECTLY set.  Object is still in cache.
var ac = _context.Analytics.Cacheable(debugger).FirstOrDefault(x => x.ID == a.ID);

 // Test retrieving directly.  Result is appropriately null.
var ad = _context.Analytics.FirstOrDefault(x => x.ID == a.ID);

Is there some other set up piece that I am missing?

VahidN commented 5 years ago

True. Because The ToSql() method (which will be used to calculate the hash of the query or the cache key automatically) doesn't see the x => x.ID == a.ID predicate. It will be evaluated where the Cacheable(debugger) method is located (It doesn't see anything after it). This one should work:

var ac = _context.Analytics.Where(x => x.ID == a.ID).Cacheable(debugger).FirstOrDefault();

Or you can provide a saltKey parameter for the Cacheable method to change its final hash based on the a.ID value.

jasenf commented 5 years ago

Interesting. I see what you mean, didn't quite understand how this operated.

So, theoretically, would it be a good enhancement for this library to throw an exception if someone tries to use Cacheable() without any predicate set up? I can see this as a very nuanced issue where a simple mis-ordering or Linq causes big issues.

Thanks a ton for your quick response.

jasenf commented 5 years ago

Updated code is still returning a value. The code below, var ac should be null, like ae is. But it's not. debugger is showing a cache hit.

var debugger = new EFCacheDebugInfo();

// Create a new object
Analytic a = new Analytic();
a.ApplicationID = 4171;
a.EmailsSent = 1;

// Save it
_context.Analytics.Add(a);
_context.SaveChanges();

// Retrieve it.   DB Call is made and object is cached.
var ab = _context.Analytics.Where(x=>x.ID==a.ID).Cacheable(debugger).FirstOrDefault();

// This one should return
var ae = _context.Analytics.Where(x=>x.ID==a.ID).Cacheable(debugger).FirstOrDefault();

// Delete it from DB
_context.Analytics.Remove(a);
_context.SaveChanges();

// Test retrieving from cache.  Variable is INCORRECTLY set.  Object is still in cache.
var ac = _context.Analytics.Where(x => x.ID == a.ID).Cacheable(debugger).FirstOrDefault();

// Test retrieving directly.  Result is appropriately null.
var ad = _context.Analytics.Where(x => x.ID == a.ID).FirstOrDefault();
VahidN commented 5 years ago

Use a different var debugger = new EFCacheDebugInfo(); per each query or check out the logged SQL like this test which is exactly based on this issue.

jasenf commented 5 years ago

Ok, done. The keyhash is the same for all queries, which I kind of expected. The issue seems to be the item not being removed during the .Remove > .SaveChanges process. My override looks like this:

 public override int SaveChanges()
        {
            var changedEntityNames = this.GetChangedEntityNames();

            this.ChangeTracker.AutoDetectChangesEnabled = false; // for performance reasons, to avoid calling DetectChanges() again.
            var result = base.SaveChanges();
            this.ChangeTracker.AutoDetectChangesEnabled = true;

            this.GetService<IEFCacheServiceProvider>().InvalidateCacheDependencies(changedEntityNames);

            return result;
        }
jasenf commented 5 years ago

Follow up... first, thanks for that code example, it seems to actually be having a variation of the same issue. When I run that code, this line:

https://github.com/VahidN/EFSecondLevelCache.Core/blob/master/src/Tests/EFSecondLevelCache.Core.AspNetCoreSample/Controllers/HomeController.cs#L110

should return null, as you removed the produce in the preceding lines. However, it is not. It is returning an entity. Worse, I think, its returning the wrong item.

image

VahidN commented 5 years ago

should return null, as you removed the produce in the preceding lines. However, it is not. It is returning an entity. Worse, I think, its returning the wrong item.

Yes! Because the cache key won't be calculated for anything after the Cacheable(debugger) method (.FirstOrDefault(x=>x.ID==a.ID) here). So its cache key is the same as the 1st query which has a different ID.

jasenf commented 5 years ago

Haha.. sorry, was looking at the wrong result. My confusion in your sample is because the sample never actually loads the entity into the cache in the proper way. If you inject the following code into your test, then thirdQueryResult no longer returns null:

// 1st query, reading from db
var debugInfo1 = new EFCacheDebugInfo();
var firstQueryResult = _context.Products
      .Cacheable(debugInfo1)
       .FirstOrDefault(p => p.ProductId == product.ProductId);

// ADD THIS CODE -- breaks thirdQueryResult
var debugInfo4 = new EFCacheDebugInfo();
var fourthQueryResult = _context.Products.Where(p => p.ProductId == product.ProductId)
                .Cacheable(debugInfo4)
                .FirstOrDefault();

 // Delete it from db, invalidates the cache on SaveChanges
 _context.Products.Remove(product);
_context.SaveChanges();
VahidN commented 5 years ago

Fixed it via #https://github.com/VahidN/EFSecondLevelCache.Core/commit/a4046261345f03265970bc2abb5012ae4fed9547. Give it a try.

jasenf commented 5 years ago

Seems to work. Thanks! Any chance this was a new bug? I'm curious because if delete's have essentially never been working for anyone?

VahidN commented 5 years ago

Yes. There was an issue about it before.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related problems.