alastairtree / LazyCache

An easy to use thread safe in-memory caching service with a simple developer friendly API for c#
https://nuget.org/packages/LazyCache
MIT License
1.72k stars 159 forks source link

CacheObject not getting overwritten. #18

Closed omnoms closed 7 years ago

omnoms commented 7 years ago

I have a simple API that stores a short JSON string with a GUID as a key, for quick counters on a page so that it follows the user along on different devices.

The problem I have is that despite the code updating the underlying database correctly, the cache-object isn't updated.

On the GET method I have the simple AddOrGet as per sample;

Func<Task<string>> cacheableAsyncFunc = () => GetByGuid(key);
string result = await cache.GetOrAddAsync(key.ToString(), cacheableAsyncFunc);

However, that isn't enough, clearly as when a POST/PUT comes along and updates/sets the value, it should update the cache as well.

At first I had only this line in the code;

cache.Add(inc.key.ToString(), inc.keyValue);

It seemed to work on a small scale when I just used postman, and hit these in sequence and quite slowly and deliberately. However, there seems to be a racing condition or something that if a GET request hits just shortly after the POST/PUT has happened, the Cache object still prevails despite being updated. So I tried to forcibly remove it, and have this line on the very first line in the POST/PUT method.

cache.Remove(inc.key.ToString());

That doesn't help however. If I spam postman on the GET method while reloading a page triggering a POST/PUT request, the GET method never returns the new value that is set in the POST/PUT method.

However, this is rarely reproducible on my own machine/dev environment, but much more reliably reproduced in an Azure Web App. No errors are logged, just that the cache is not updated.

Full Code

        [HttpGet]
        public async Task<HttpResponseMessage> Get(Guid key)
        {
            Func<Task<string>> cacheableAsyncFunc = () => GetByGuid(key);
            string result = await cache.GetOrAddAsync(key.ToString(), cacheableAsyncFunc);

            if (result != null)
                return new HttpResponseMessage { StatusCode = HttpStatusCode.OK, Content = new StringContent(result, Encoding.UTF8, "application/json") };
            else
                return new HttpResponseMessage { StatusCode = HttpStatusCode.OK };
        }
        private async Task<string> GetByGuid(Guid key)
        {
            using (var context = new CounterAPI.Models.CounterModelContainer(WebApiApplication.CounterDB))
            {
                Models.Counter cnt = await context.Counters.FirstOrDefaultAsync(c => c.key == key);
                string result = null;
                if (cnt != null)
                {
                    result = Regex.Unescape(cnt.keyValue);
                    return result;
                }
                return result;
            }
        }
        [HttpPost]
        public async Task<HttpResponseMessage> Post(Models.Counter inc)
        {
            if (!ModelState.IsValid || inc == null || inc.key == null || inc.keyValue == null) return new HttpResponseMessage { StatusCode = HttpStatusCode.BadRequest, ReasonPhrase = "Missing values" };
            cache.Remove(inc.key.ToString());
            using (var context = new CounterAPI.Models.CounterModelContainer(WebApiApplication.CounterDB))
            {
                Models.Counter cnt = await context.Counters.FirstOrDefaultAsync(c => c.key == inc.key);
                if(cnt == null) { context.Counters.AddOrUpdate(inc); }
                else { cnt.keyValue = inc.keyValue; }
                try
                {
                    await context.SaveChangesAsync();
                    cache.Add(inc.key.ToString(), inc.keyValue);
                }
#pragma warning disable CS0168 // Variable is declared but never used
                catch (Exception ex)
#pragma warning restore CS0168 // Variable is declared but never used
                {
                    return new HttpResponseMessage { StatusCode = HttpStatusCode.InternalServerError, ReasonPhrase = "Unable to add content" };
                }
                return new HttpResponseMessage { StatusCode = HttpStatusCode.Created };
            }
        }

After a POST, there remains a difference between ObjectCache and SQL DB entry;

image

omnoms commented 7 years ago

I'll also note that the Azure Web App isn't loadbalanced or scaled out to several VMs or some such either.

omnoms commented 7 years ago

I've now mitigated this on client side by ignoring the GET result that was stale as the values are less than what the client currently has. And it doesn't seem to happen all the time either... It's tricky to reproduce, but once it's reproduced it's sticking around for a while until it releases it and it all starts to work as intended for a while...

alastairtree commented 7 years ago

Any chance you could try and create a unit tests that repros the issue? If you look in the tests project there are tests like GetFromCacheTwiceAtSameTimeOnlyAddsOnce() that try and test race conditions. To summarise I suspect you are doing these operations:

Start a GetOrAdd(key, value1) Remove(key) Add(key, value2) finish first GetOrAdd(key, value1)

In this case it's not crystal clear in the general case that you want value1 or value2 to remain in the cache?

I cannot see anything obvious in your code and am wondering if this is an artifact of the way that the Microsoft MemoryCache class we depend on handles adding items after the delegate has been evaluated.

Either way a repeatable test would be a good way to establish if there is an issue.

omnoms commented 7 years ago

I had the same effect even without the Remove bit. I'll see if I can get a repeatable test out of this.

omnoms commented 7 years ago

I've created a simple unit-test and the race condition never happens in that. I think it would boil down to the concurrency/asynchronous client which has since been mitigated. The race-condition there has to be that sometimes a Get-request finishes off after a post-request, and with the mitigation of checking for stale values and other checks, that has been removed :)

Not repeatable in other words :)

Also, I just saw a 0.377ms request on the Get-method in the unit-test :) Love the speed.

alastairtree commented 7 years ago

Thanks, and let's reopen this if anything changes.