Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ protected virtual void Dispose(bool disposing) { }
~MemoryCache() { }
public void Remove(object key) { }
public bool TryGetValue(object key, out object result) { throw null; }
public void Clear() { }
}
public partial class MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ public class MemoryCache : IMemoryCache
internal readonly ILogger _logger;

private readonly MemoryCacheOptions _options;
private readonly ConcurrentDictionary<object, CacheEntry> _entries;

private ConcurrentDictionary<object, CacheEntry> _entries;
Comment thread
stephentoub marked this conversation as resolved.
Outdated
private long _cacheSize;
private bool _disposed;
private DateTimeOffset _lastExpirationScan;
Expand Down Expand Up @@ -260,8 +260,8 @@ public bool TryGetValue(object key, out object result)
public void Remove(object key)
{
ValidateCacheKey(key);

CheckDisposed();

if (_entries.TryRemove(key, out CacheEntry entry))
{
if (_options.SizeLimit.HasValue)
Expand All @@ -276,6 +276,24 @@ public void Remove(object key)
StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow);
}

/// <summary>
/// Removes all keys and values from the cache.
/// </summary>
public void Clear()
{
CheckDisposed();

// the following two operations are not atomic change as a whole, but an alternative would be to introduce a global lock for every access to _entries and _cacheSize
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get away with this by introducing a new class that contained both the dictionary and the size? Then we can atomically replace the pointer with a new instance?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case, yes. In other, where we add/remove item from the cache and update the size afterwards, not.

if (_entries.TryRemove(key, out CacheEntry entry))
{
if (_options.SizeLimit.HasValue)
{
Interlocked.Add(ref _cacheSize, -entry.Size.Value);
}

entryAdded = _entries.TryUpdate(entry.Key, entry, priorEntry);
if (entryAdded)
{
if (_options.SizeLimit.HasValue)
{
// The prior entry was removed, decrease the by the prior entry's size
Interlocked.Add(ref _cacheSize, -priorEntry.Size.Value);

I am not sure if it's worth it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In those other cases you can atomically read the pointer, store it in a local variable and modify that. If Clear gets called concurrently, it will either update the pointer before or after the other mutation. Thus, we should always have a coherent set of entries and cache size.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eerhardt well, it took me a while to get back to this PR. PTAL at my recent commit.

ConcurrentDictionary<object, CacheEntry> oldEntries = Interlocked.Exchange(ref _entries, new ConcurrentDictionary<object, CacheEntry>());
Interlocked.Exchange(ref _cacheSize, 0);

foreach (var entry in oldEntries)
{
entry.Value.SetExpired(EvictionReason.Removed);
entry.Value.InvokeEvictionCallbacks();
}
}

private void RemoveEntry(CacheEntry entry)
{
if (EntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry)))
Expand Down Expand Up @@ -317,7 +335,8 @@ private static void ScanForExpiredItems(MemoryCache cache)
{
DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow;

foreach (KeyValuePair<object, CacheEntry> item in cache._entries)
ConcurrentDictionary<object, CacheEntry> entries = cache._entries; // Clear() can update the reference in the meantime
foreach (KeyValuePair<object, CacheEntry> item in entries)
{
CacheEntry entry = item.Value;

Expand Down Expand Up @@ -402,7 +421,8 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry

// Sort items by expired & priority status
DateTimeOffset now = _options.Clock.UtcNow;
foreach (KeyValuePair<object, CacheEntry> item in _entries)
ConcurrentDictionary<object, CacheEntry> entries = _entries; // Clear() can update the reference in the meantime
foreach (KeyValuePair<object, CacheEntry> item in entries)
{
CacheEntry entry = item.Value;
if (entry.CheckExpired(now))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,5 +444,19 @@ public void NoCompactionWhenNoMaximumEntriesCountSpecified()
// There should be 6 items in the cache
Assert.Equal(6, cache.Count);
}

[Fact]
public void ClearZeroesTheSize()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
Assert.Equal(0, cache.Size);

cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
Assert.Equal(5, cache.Size);

cache.Clear();
Assert.Equal(0, cache.Size);
Assert.Equal(0, cache.Count);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,29 @@ public void RemoveRemoves()
Assert.Null(result);
}

[Fact]
public void ClearClears()
{
var cache = (MemoryCache)CreateCache();
var obj = new object();
string[] keys = new string[] { "key1", "key2", "key3", "key4" };

foreach (var key in keys)
Comment thread
adamsitnik marked this conversation as resolved.
Outdated
{
var result = cache.Set(key, obj);
Assert.Same(obj, result);
Assert.Same(obj, cache.Get(key));
}

cache.Clear();

Assert.Equal(0, cache.Count);
foreach (var key in keys)
Comment thread
adamsitnik marked this conversation as resolved.
Outdated
{
Assert.Null(cache.Get(key));
}
}

[Fact]
public void RemoveRemovesAndInvokesCallback()
{
Expand Down Expand Up @@ -397,6 +420,38 @@ public void RemoveRemovesAndInvokesCallback()
Assert.Null(result);
}

[Fact]
public void ClearClearsAndInvokesCallback()
{
var cache = (MemoryCache)CreateCache();
var value = new object();
string key = "myKey";
var callbackInvoked = new ManualResetEvent(false);

var options = new MemoryCacheEntryOptions();
options.PostEvictionCallbacks.Add(new PostEvictionCallbackRegistration()
{
EvictionCallback = (subkey, subValue, reason, state) =>
{
Assert.Equal(key, subkey);
Assert.Same(value, subValue);
Assert.Equal(EvictionReason.Removed, reason);
var localCallbackInvoked = (ManualResetEvent)state;
localCallbackInvoked.Set();
},
State = callbackInvoked
});
var result = cache.Set(key, value, options);
Assert.Same(value, result);

cache.Clear();
Assert.Equal(0, cache.Count);
Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback");

result = cache.Get(key);
Assert.Null(result);
}

[Fact]
public void RemoveAndReAddFromCallbackWorks()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,30 @@ public void RemoveItemDisposesTokenRegistration()
Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback");
}

[Fact]
public void ClearingCacheDisposesTokenRegistration()
{
var cache = (MemoryCache)CreateCache();
string key = "myKey";
var value = new object();
var callbackInvoked = new ManualResetEvent(false);
var expirationToken = new TestExpirationToken() { ActiveChangeCallbacks = true };
cache.Set(key, value, new MemoryCacheEntryOptions()
.AddExpirationToken(expirationToken)
.RegisterPostEvictionCallback((subkey, subValue, reason, state) =>
{
// TODO: Verify params
Comment thread
adamsitnik marked this conversation as resolved.
Outdated
var localCallbackInvoked = (ManualResetEvent)state;
localCallbackInvoked.Set();
}, state: callbackInvoked));
cache.Clear();

Assert.Equal(0, cache.Count);
Assert.NotNull(expirationToken.Registration);
Assert.True(expirationToken.Registration.Disposed);
Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback");
}

[Fact]
public void AddExpiredTokenPreventsCaching()
{
Expand Down