From 417ba4d0413a8839e001ba75caa992684462ddd3 Mon Sep 17 00:00:00 2001 From: Dennis Doomen Date: Wed, 6 Feb 2019 13:44:42 +0100 Subject: [PATCH 1/4] Removed the superfluous INode interface --- Src/FluidCaching/FluidCache.cs | 8 ++++---- Src/FluidCaching/IIndexManagement.cs | 4 ++-- Src/FluidCaching/INode.cs | 12 ------------ Src/FluidCaching/Index.cs | 24 ++++++++++++------------ Src/FluidCaching/LifespanManager.cs | 4 ++-- Src/FluidCaching/Node.cs | 2 +- 6 files changed, 21 insertions(+), 33 deletions(-) delete mode 100644 Src/FluidCaching/INode.cs diff --git a/Src/FluidCaching/FluidCache.cs b/Src/FluidCaching/FluidCache.cs index 7096aa4..75e2244 100644 --- a/Src/FluidCaching/FluidCache.cs +++ b/Src/FluidCaching/FluidCache.cs @@ -102,14 +102,14 @@ public void Add(T item) /// /// AddAsNode an item to the cache /// - internal INode AddAsNode(T item) + internal Node AddAsNode(T item) { if (item == null) { return null; } - INode node = FindExistingNode(item); + Node node = FindExistingNode(item); // dupl is used to prevent total count from growing when item is already in indexes (only new Nodes) bool isDuplicate = (node != null) && (node.Value == item); @@ -143,9 +143,9 @@ internal INode AddAsNode(T item) return node; } - private INode FindExistingNode(T item) + private Node FindExistingNode(T item) { - INode node = null; + Node node = null; foreach (KeyValuePair> keyValue in indexList) { if ((node = keyValue.Value.FindItem(item)) != null) diff --git a/Src/FluidCaching/IIndexManagement.cs b/Src/FluidCaching/IIndexManagement.cs index f72253d..cdcbca1 100644 --- a/Src/FluidCaching/IIndexManagement.cs +++ b/Src/FluidCaching/IIndexManagement.cs @@ -6,8 +6,8 @@ namespace FluidCaching internal interface IIndexManagement where T : class { void ClearIndex(); - bool AddItem(INode item); - INode FindItem(T item); + bool AddItem(Node item); + Node FindItem(T item); int RebuildIndex(); } } \ No newline at end of file diff --git a/Src/FluidCaching/INode.cs b/Src/FluidCaching/INode.cs deleted file mode 100644 index 5ec4920..0000000 --- a/Src/FluidCaching/INode.cs +++ /dev/null @@ -1,12 +0,0 @@ -namespace FluidCaching -{ - /// - /// This interface exposes the public part of a LifespanMgr.Node - /// - internal interface INode where T : class - { - T Value { get; } - void Touch(); - void RemoveFromCache(); - } -} \ No newline at end of file diff --git a/Src/FluidCaching/Index.cs b/Src/FluidCaching/Index.cs index 7512f2a..2b81c36 100644 --- a/Src/FluidCaching/Index.cs +++ b/Src/FluidCaching/Index.cs @@ -13,7 +13,7 @@ internal class Index : IIndex, IIndexManagement where T : c { private readonly FluidCache owner; private readonly LifespanManager lifespanManager; - private Dictionary>> index; + private Dictionary>> index; private readonly GetKey _getKey; private readonly ItemCreator loadItem; private readonly IEqualityComparer keyEqualityComparer; @@ -49,7 +49,7 @@ public Index( /// the object value associated with key, or null if not found or could not be loaded public async Task GetItem(TKey key, ItemCreator createItem = null) { - INode node = FindExistingNodeByKey(key); + Node node = FindExistingNodeByKey(key); node?.Touch(); ItemCreator creator = createItem ?? loadItem; @@ -83,7 +83,7 @@ public async Task GetItem(TKey key, ItemCreator createItem = null) /// public void Remove(TKey key) { - INode node = FindExistingNodeByKey(key); + Node node = FindExistingNodeByKey(key); if (node != null) { lock (this) @@ -100,15 +100,15 @@ public void Remove(TKey key) } /// try to find this item in the index and return Node - public INode FindItem(T item) + public Node FindItem(T item) { return FindExistingNodeByKey(_getKey(item)); } - private INode FindExistingNodeByKey(TKey key) + private Node FindExistingNodeByKey(TKey key) { - WeakReference> reference; - INode node; + WeakReference> reference; + Node node; if (index.TryGetValue(key, out reference) && reference.TryGetTarget(out node)) { lifespanManager.Stats.RegisterHit(); @@ -132,16 +132,16 @@ public void ClearIndex() /// /// Returns true if the item could be added to the index, or false otherwise. /// - public bool AddItem(INode item) + public bool AddItem(Node item) { lock (this) { TKey key = _getKey(item.Value); - INode node; + Node node; if (!index.ContainsKey(key) || !index[key].TryGetTarget(out node) || node.Value == null) { - index[key] = new WeakReference>(item, trackResurrection: false); + index[key] = new WeakReference>(item, trackResurrection: false); return true; } @@ -156,9 +156,9 @@ public int RebuildIndex() { // Create a new ConcurrentDictionary, this way there is no need for locking the index itself var keyValues = lifespanManager - .ToDictionary(item => _getKey(item.Value), item => new WeakReference>(item)); + .ToDictionary(item => _getKey(item.Value), item => new WeakReference>(item)); - index = new Dictionary>>(keyValues, keyEqualityComparer); + index = new Dictionary>>(keyValues, keyEqualityComparer); return index.Count; } } diff --git a/Src/FluidCaching/LifespanManager.cs b/Src/FluidCaching/LifespanManager.cs index 034b60e..3338427 100644 --- a/Src/FluidCaching/LifespanManager.cs +++ b/Src/FluidCaching/LifespanManager.cs @@ -5,7 +5,7 @@ namespace FluidCaching { - internal class LifespanManager : IEnumerable> where T : class + internal class LifespanManager : IEnumerable> where T : class { /// /// The number of bags which should be enough to store the requested capacity of items. The heuristic is that each @@ -255,7 +255,7 @@ IEnumerator IEnumerable.GetEnumerator() } /// Create item enumerator - public IEnumerator> GetEnumerator() + public IEnumerator> GetEnumerator() { for (int bagNumber = CurrentBagIndex; bagNumber >= OldestBagIndex; --bagNumber) { diff --git a/Src/FluidCaching/Node.cs b/Src/FluidCaching/Node.cs index 9125c43..3493efa 100644 --- a/Src/FluidCaching/Node.cs +++ b/Src/FluidCaching/Node.cs @@ -5,7 +5,7 @@ namespace FluidCaching /// /// Represents a node in a linked list of items. /// - internal class Node : INode where T : class + internal class Node where T : class { private readonly LifespanManager manager; From 1d944c5c1a040e82686128c21bf29c9c6bb6a4ea Mon Sep 17 00:00:00 2001 From: Dennis Doomen Date: Wed, 6 Feb 2019 14:54:31 +0100 Subject: [PATCH 2/4] Uses a dedicated object to lock on instead of using this. --- FluidCaching.sln.DotSettings | 1 + Src/FluidCaching/FluidCache.cs | 5 +++-- Src/FluidCaching/Index.cs | 13 +++++++------ Src/FluidCaching/LifespanManager.cs | 13 +++++++------ Src/FluidCaching/Node.cs | 5 +++-- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/FluidCaching.sln.DotSettings b/FluidCaching.sln.DotSettings index ec16fbb..69d6e9d 100644 --- a/FluidCaching.sln.DotSettings +++ b/FluidCaching.sln.DotSettings @@ -26,6 +26,7 @@ True True True + True True True True diff --git a/Src/FluidCaching/FluidCache.cs b/Src/FluidCaching/FluidCache.cs index 75e2244..a3adc8f 100644 --- a/Src/FluidCaching/FluidCache.cs +++ b/Src/FluidCaching/FluidCache.cs @@ -34,7 +34,8 @@ class FluidCache where T : class { private readonly Dictionary> indexList = new Dictionary>(); private readonly LifespanManager lifeSpan; - + private readonly object syncObject = new object(); + /// Constructor /// the normal item limit for cache (Count may exeed capacity due to minAge) /// the minimium time after an access before an item becomes eligible for removal, during this time @@ -125,7 +126,7 @@ internal Node AddAsNode(T item) } } - lock (this) + lock (syncObject) { if (!isDuplicate) { diff --git a/Src/FluidCaching/Index.cs b/Src/FluidCaching/Index.cs index 2b81c36..33a7533 100644 --- a/Src/FluidCaching/Index.cs +++ b/Src/FluidCaching/Index.cs @@ -17,7 +17,8 @@ internal class Index : IIndex, IIndexManagement where T : c private readonly GetKey _getKey; private readonly ItemCreator loadItem; private readonly IEqualityComparer keyEqualityComparer; - + private readonly object syncObject = new object(); + /// constructor /// parent of index /// @@ -64,7 +65,7 @@ public async Task GetItem(TKey key, ItemCreator createItem = null) T value = await task; - lock (this) + lock (syncObject) { node = FindExistingNodeByKey(key); if (node?.Value == null) @@ -86,7 +87,7 @@ public void Remove(TKey key) Node node = FindExistingNodeByKey(key); if (node != null) { - lock (this) + lock (syncObject) { node = FindExistingNodeByKey(key); if (node != null) @@ -121,7 +122,7 @@ private Node FindExistingNodeByKey(TKey key) /// Remove all items from index public void ClearIndex() { - lock (this) + lock (syncObject) { index.Clear(); } @@ -134,7 +135,7 @@ public void ClearIndex() /// public bool AddItem(Node item) { - lock (this) + lock (syncObject) { TKey key = _getKey(item.Value); @@ -152,7 +153,7 @@ public bool AddItem(Node item) /// removes all items from index and reloads each item (this gets rid of dead nodes) public int RebuildIndex() { - lock (this) + lock (syncObject) { // Create a new ConcurrentDictionary, this way there is no need for locking the index itself var keyValues = lifespanManager diff --git a/Src/FluidCaching/LifespanManager.cs b/Src/FluidCaching/LifespanManager.cs index 3338427..099b0ec 100644 --- a/Src/FluidCaching/LifespanManager.cs +++ b/Src/FluidCaching/LifespanManager.cs @@ -40,6 +40,7 @@ internal class LifespanManager : IEnumerable> where T : class internal int itemsInCurrentBag; private int currentBagIndex; private int oldestBagIndex; + private readonly object syncObject = new object(); public LifespanManager(FluidCache owner, int capacity, TimeSpan minAge, TimeSpan maxAge, GetNow getNow) { @@ -74,7 +75,7 @@ public void CheckValidity() { // NOTE: Monitor.Enter(this) / Monitor.Exit(this) is the same as lock(this)... We are using Monitor.TryEnter() because it // does not wait for a lock, if lock is currently held then skip and let next Touch perform cleanup. - if (RequiresCleanup && Monitor.TryEnter(this)) + if (RequiresCleanup && Monitor.TryEnter(syncObject)) { try { @@ -93,7 +94,7 @@ public void CheckValidity() } finally { - Monitor.Exit(this); + Monitor.Exit(syncObject); } } } @@ -111,7 +112,7 @@ public void CheckValidity() /// private void CleanUp(DateTime now) { - lock (this) + lock (syncObject) { int itemsAboveCapacity = Stats.Current - Stats.Capacity; AgeBag bag = bags[OldestBagIndex]; @@ -209,7 +210,7 @@ public int CurrentBagIndex /// Remove all items from LifespanMgr and reset public void Clear() { - lock (this) + lock (syncObject) { bags.Empty(); @@ -223,7 +224,7 @@ public void Clear() /// ready a new current AgeBag for use and close the previous one private void OpenBag(int bagNumber) { - lock (this) + lock (syncObject) { DateTime now = getNow(); @@ -273,7 +274,7 @@ public IEnumerator> GetEnumerator() public Node AddToHead(Node node) { - lock (this) + lock (syncObject) { Node next = CurrentBag.First; CurrentBag.First = node; diff --git a/Src/FluidCaching/Node.cs b/Src/FluidCaching/Node.cs index 3493efa..f527800 100644 --- a/Src/FluidCaching/Node.cs +++ b/Src/FluidCaching/Node.cs @@ -8,6 +8,7 @@ namespace FluidCaching internal class Node where T : class { private readonly LifespanManager manager; + private readonly object syncObject = new object(); /// constructor public Node(LifespanManager manager, T value) @@ -41,7 +42,7 @@ private void RegisterWithLifespanManager() { if (Bag == null) { - lock (this) + lock (syncObject) { if (Bag == null) { @@ -59,7 +60,7 @@ public void RemoveFromCache() { if ((Bag != null) && (Value != null)) { - lock (this) + lock (syncObject) { if ((Bag != null) && (Value != null)) { From 5a6707dcc3f7f9e2385167747418113cee5844b4 Mon Sep 17 00:00:00 2001 From: Dennis Doomen Date: Wed, 6 Feb 2019 15:39:57 +0100 Subject: [PATCH 3/4] Cleaned up some of the docs/naming --- Src/FluidCaching/FluidCache.cs | 14 +++++++++++--- Src/FluidCaching/IIndex.cs | 12 ++++++------ Src/FluidCaching/Index.cs | 6 ------ Src/FluidCaching/Node.cs | 2 +- Tests/FluidCaching.Specs/FluidCacheSpecs.cs | 4 ++-- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/Src/FluidCaching/FluidCache.cs b/Src/FluidCaching/FluidCache.cs index a3adc8f..01e8977 100644 --- a/Src/FluidCaching/FluidCache.cs +++ b/Src/FluidCaching/FluidCache.cs @@ -67,11 +67,19 @@ public IIndex GetIndex(string indexName) return indexList.TryGetValue(indexName, out index) ? index as IIndex : null; } - /// Retrieve a object by index name / key - public Task Get(string indexName, TKey key, ItemCreator item = null) + /// + /// Gets an object associated with from the index identified by + /// or tries to create a new one using the + /// (optional) factory method provided by + /// + /// + /// Returns the object associated with the key or null if no such object exists and + /// the was null or returned a null. + /// + public Task Get(string indexName, TKey key, ItemCreator createItem = null) { IIndex index = GetIndex(indexName); - return index?.GetItem(key, item); + return index?.GetItem(key, createItem); } /// Adds a new index to the cache diff --git a/Src/FluidCaching/IIndex.cs b/Src/FluidCaching/IIndex.cs index 49643dd..fa338d1 100644 --- a/Src/FluidCaching/IIndex.cs +++ b/Src/FluidCaching/IIndex.cs @@ -13,13 +13,13 @@ namespace FluidCaching interface IIndex where T : class { /// - /// Getter for index + /// Gets an object from the index based on the provided or tries to create a new one using the + /// (optional) factory method provided by /// - /// key to find (or load if needed) - /// - /// An optional delegate that is used to create the actual object if it doesn't exist in the cache. - /// - /// the object value associated with the cache + /// + /// Returns the object associated with the key or null if no such object exists and + /// the was null or returned a null. + /// Task GetItem(TKey key, ItemCreator createItem = null); /// Delete object that matches key from cache diff --git a/Src/FluidCaching/Index.cs b/Src/FluidCaching/Index.cs index 33a7533..71cf970 100644 --- a/Src/FluidCaching/Index.cs +++ b/Src/FluidCaching/Index.cs @@ -42,12 +42,6 @@ public Index( RebuildIndex(); } - /// Getter for index - /// key to find (or load if needed) - /// - /// An optional factory method for creating the item if it does not exist in the cache. - /// - /// the object value associated with key, or null if not found or could not be loaded public async Task GetItem(TKey key, ItemCreator createItem = null) { Node node = FindExistingNodeByKey(key); diff --git a/Src/FluidCaching/Node.cs b/Src/FluidCaching/Node.cs index f527800..0187487 100644 --- a/Src/FluidCaching/Node.cs +++ b/Src/FluidCaching/Node.cs @@ -46,7 +46,7 @@ private void RegisterWithLifespanManager() { if (Bag == null) { - // if node.AgeBag==null then the object is not currently managed by LifespanMgr so add it + // if Bag is null, then the object is not currently managed by the life span manager Next = manager.AddToHead(this); } } diff --git a/Tests/FluidCaching.Specs/FluidCacheSpecs.cs b/Tests/FluidCaching.Specs/FluidCacheSpecs.cs index a8cf6ff..779d5b2 100644 --- a/Tests/FluidCaching.Specs/FluidCacheSpecs.cs +++ b/Tests/FluidCaching.Specs/FluidCacheSpecs.cs @@ -233,12 +233,12 @@ public async Task Then_successive_requests_for_that_key_should_return_the_new_ob } } - public class When_an_item_doesnt_exist_in_the_cache_and_the_factory_returns_a_null_task : GivenWhenThen> + public class When_an_item_does_not_exist_in_the_cache_and_the_factory_returns_a_null_task : GivenWhenThen> { private IIndex indexById; private FluidCache cache; - public When_an_item_doesnt_exist_in_the_cache_and_the_factory_returns_a_null_task() + public When_an_item_does_not_exist_in_the_cache_and_the_factory_returns_a_null_task() { Given(() => { From c48b568da1998bc025e901c2011cebee21ed067f Mon Sep 17 00:00:00 2001 From: Dennis Doomen Date: Wed, 6 Feb 2019 15:40:47 +0100 Subject: [PATCH 4/4] Fixes GetItem from returning null during concurrent cache removals --- Src/FluidCaching/FluidCache.cs | 4 +-- Src/FluidCaching/Index.cs | 48 ++++++++++++++++++++++------------ Src/FluidCaching/Node.cs | 11 +++++--- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/Src/FluidCaching/FluidCache.cs b/Src/FluidCaching/FluidCache.cs index 01e8977..b1fc561 100644 --- a/Src/FluidCaching/FluidCache.cs +++ b/Src/FluidCaching/FluidCache.cs @@ -105,13 +105,13 @@ public IIndex AddIndex( /// public void Add(T item) { - AddAsNode(item); + TryAddAsNode(item); } /// /// AddAsNode an item to the cache /// - internal Node AddAsNode(T item) + internal Node TryAddAsNode(T item) { if (item == null) { diff --git a/Src/FluidCaching/Index.cs b/Src/FluidCaching/Index.cs index 71cf970..8c1ce28 100644 --- a/Src/FluidCaching/Index.cs +++ b/Src/FluidCaching/Index.cs @@ -44,11 +44,38 @@ public Index( public async Task GetItem(TKey key, ItemCreator createItem = null) { - Node node = FindExistingNodeByKey(key); - node?.Touch(); + T value = null; + + lock (syncObject) + { + Node node = FindExistingNodeByKey(key); + if (node != null) + { + value = node.Value; + + node.Touch(); + } + } + + if (value == null) + { + value = await TryCreate(key, createItem); + if (value != null) + { + Node newOrExistingNode = owner.TryAddAsNode(value); + value = newOrExistingNode.Value; + + lifespanManager.CheckValidity(); + } + } + return value; + } + + private async Task TryCreate(TKey key, ItemCreator createItem = null) + { ItemCreator creator = createItem ?? loadItem; - if ((node?.Value == null) && (creator != null)) + if (creator != null) { Task task = creator(key); if (task == null) @@ -57,21 +84,10 @@ public async Task GetItem(TKey key, ItemCreator createItem = null) "Expected a non-null Task. Did you intend to return a null-returning Task instead?"); } - T value = await task; - - lock (syncObject) - { - node = FindExistingNodeByKey(key); - if (node?.Value == null) - { - node = owner.AddAsNode(value); - } - - lifespanManager.CheckValidity(); - } + return await task; } - return node?.Value; + return null; } /// Delete object that matches key from cache diff --git a/Src/FluidCaching/Node.cs b/Src/FluidCaching/Node.cs index 0187487..f564c29 100644 --- a/Src/FluidCaching/Node.cs +++ b/Src/FluidCaching/Node.cs @@ -29,12 +29,15 @@ public Node(LifespanManager manager, T value) /// public void Touch() { - if ((Value != null) && (Bag != manager.CurrentBag)) + lock (syncObject) { - RegisterWithLifespanManager(); + if ((Value != null) && (Bag != manager.CurrentBag)) + { + RegisterWithLifespanManager(); - Bag = manager.CurrentBag; - Interlocked.Increment(ref manager.itemsInCurrentBag); + Bag = manager.CurrentBag; + Interlocked.Increment(ref manager.itemsInCurrentBag); + } } }