AddOrGetExisting does not keep expiration into account

Question

I am using (.NET 4.5) MemoryCache, combined with SlidingExpiration.

I notice that the method .AddOrGetExisting() does not seem to keep the expiration in mind, whilst .Get() does.

Unit tests:

    [TestMethod]
    public void NonWorking()
    {
        var memCache = new MemoryCache("somekey");
        var cachePolicy = new CacheItemPolicy() { SlidingExpiration = TimeSpan.FromSeconds(1) };

        var cacheEntry = memCache.AddOrGetExisting("key1", "foo", cachePolicy);
        Assert.AreEqual(null, cacheEntry); // OK: AddOrGetExisting returns null, because it wasn't existing yet
        Thread.Sleep(1100);

        // Expecting null, since the existing item for key1 has expired by now.
        // It is, however, still "foo".
        Assert.AreEqual(null, memCache.AddOrGetExisting("key1", "bar", cachePolicy));

        // FYI: afterwards, memCache.Get("key1") still equals "foo"
    }

    [TestMethod]
    public void Working()
    {
        var memCache = new MemoryCache("somekey");
        var cachePolicy = new CacheItemPolicy() { SlidingExpiration = TimeSpan.FromSeconds(1) };

        var cacheEntry = memCache.AddOrGetExisting("key1", "foo", cachePolicy);
        Assert.AreEqual(null, cacheEntry); // OK: AddOrGetExisting returns null, because it wasn't existing yet
        Thread.Sleep(1100);
        Assert.AreEqual(null, memCache.Get("key1"));
    }

Question:

Is this the expected behaviour for .AddOrGetExisting()?

I could fall back to .Get() and then, if null, .Add().
However, consequently I would have to implement my own locking to ensure thread-safety.

Answer 1

The problem lies in the second call of AddOrGetExisting. When calling the Method it creates a new Instance of MemoryCacheEntry SourceCode of .net Framework. In this entry it sets the Expiration to UtcNow + 1 Second, making your Thread.Sleep useless. (See this Line of the Constructor)

I dont know why it doesnt use the policy of the existing entry to determine the timeout though. This line should use existingEntry instead of entry i guess. Maybe this is a bug?

Here is the code in the Framework that produces the "misbehaviour"

existingEntry = _entries[key] as MemoryCacheEntry;
// has it expired?
if (existingEntry != null && /* THERE => */ entry.UtcAbsExp <= DateTime.UtcNow) {
    toBeReleasedEntry = existingEntry;
    toBeReleasedEntry.State = EntryState.RemovingFromCache;
    existingEntry = null;
}

existingEntry is the "old entry" that should expire, entry is the new one with a not expired UtcAbsExp value.

Answer 2

If you extend the thread sleep far enough, the expected behaviour happens. I assume the MemoryCache's Timer mechanism expires the element.

I haven't dug enough through the code to work out what the default timer interval is, but I suspect it may be 10 seconds. Using 9 seconds still failed, 15 seconds passed - for me.

So the impact of this bug in AddOrGetExisting depends on how frequently you're calling AddOrGetExisting. If you're calling it consistently slower than the sliding expiry window, but faster than the MemoryCache timer is checking for expiries, then you'll get the worst case where you always keep the oldest value, even though it has expired. If the old and new values aren't equivalent, then you'll have incorrect behaviour in your code.

If you know there will be periods where you don't access the value for long enough that the timer will expire the item, and you can tolerate possible cases above where the item should have expired but didn't, then you can probably live with this issue.