IMemoryCache.Set() silently fails when Size >= SizeLimit

[sellotape]

When adding an item via IMemoryCache.Set() (the extension method), and the number of items in the cache is already at MemoryCacheOptions.SizeLimit, the item is not added, but the add also fails silently, and there appears to be no way to readily know this.

The only indication of failure appears to be that CacheEntry.EvictionReason is set to Capacity, but a) this is only set on disposal of the ICacheEntry, b) using .Set() does not expose the ICacheEntry anyway, and c) even if trying to use e.g. IMemoryCache.CreateEntry() instead, CacheEntry and EvictionReason are internal.

To Reproduce

[Test]
public void OverCapacity_IsHandled_Eg_ByCompaction()
{
    var cacheOptions = new MemoryCacheOptions { SizeLimit = 2, CompactionPercentage = 0.5D };
    var entryOptions = new MemoryCacheEntryOptions { SlidingExpiration = TimeSpan.FromMinutes(1.0), Size = 1 };

    var value1 = new object();
    var value2 = new object();
    var value3 = new object();

    using (var memoryCache = new MemoryCache(cacheOptions))
    {
        object added1 = memoryCache.Set("key1", value1, entryOptions);
        Assert.That(added1, Is.Not.Null.And.EqualTo(value1));
        Assert.That(memoryCache.Get("key1"), Is.Not.Null);

        object added2 = memoryCache.Set("key2", value2, entryOptions);
        Assert.That(added2, Is.Not.Null.And.EqualTo(value2));
        Assert.That(memoryCache.Get("key2"), Is.Not.Null);

        object added3 = memoryCache.Set("key3", value3, entryOptions); //// Might expect this to make some noise as it failed...
        Assert.That(added3, Is.Not.Null.And.EqualTo(value3)); //// <-- ...and this doesn't indicate an issue either
        Assert.That(memoryCache.Get("key3"), Is.Not.Null); //// <-- Assertion fails; the item is not in the cache.
    }
}

Expected behavior

In most- to least-preferred order:

• call OvercapacityCompaction(), add the new item, succeed. This will take more execution time, but it's probably what the caller wants to achieve anyway, and the frequency of this can be regulated with MemoryCacheOptions.CompactionPercentage.
• throw an exception. The caller will probably then still want to remove LRUs, then add the new value, so it doesn't seem like a good option when MemoryCache could assume responsibility for this instead.

Additional context

Issue 1087 alludes to the same issue, but it seems to not be getting much attention and I thought it needed a stronger title.

I am using Microsoft.Extensions.Caching 2.2.0

Some workarounds I considered:

• Calling .Compact() before .Set(), but this scans every item in the cache regardless of whether any compaction is actually needed, so is very inefficient. Also .Compact() is available on MemoryCache, not IMemoryCache, so is not available to the latter when injected, without assuming knowledge of the former.
• Comparing .Size to MemoryCacheOptions.SizeLimit just before using .Set(), and then only calling .Compact() if needed, but again neither is available on IMemoryCache and .Size is internal anyway. In my case I know each item is of size 1, so I could use .Count as equivalent, but this is not always the case.
• Inheriting MemoryCache to create e.g. AutoCompactingMemoryCache to get around the limitations above, but while MemoryCache is not sealed, nothing of note is virtual to allow this, and the use of extension methods makes this even harder.

I eventually worked around this by creating an AutoCompactingMemoryCache inheriting MemoryCache and re-implementing IMemoryCache.CreateEntry() explicitly, with the known assumption of .Size == .Count, but the assumption, while it works for me, is not the general case.

[Tratcher]

This is by design.

• When using a cache you always have to cope with the possibility that an item can be removed at any time. Failing to add it is only an extreme example of that.
• Blocking the caller while running the LRU eviction could lead to significant contention/performance problems.
• Throwing would make the cache even harder to use.

Note the EvictionReason is available if you register a PostEvictionCallback.

[analogrelay]

Agreed. It's always possible to change this behavior by implementing your own IMemoryCache, as you've done. We don't currently have plans to change this behavior in our default cache.

[sellotape]

I agree with some of that; what I disagree with is that it's okay for a method called Set() to succeed or fail without any indication of the outcome**. The method is called "Set", not "TrySet" or "SetIfSpaceAllows"; it's reasonable for a caller to believe that it will actually set something.

--

** Yes I know I could register a post-eviction callback, but to do this for potentially millions of items, resulting in million of ignored calls, seems wasteful and unnecessary, just to establish whether an item made it into the cache in the first place.