No, it's not safe because you're locking only when writing. You prevent multiple writings to occur simultaneously but:
- Caller might call
KeyExists() in the middle of
Insert(), in this case:
data[partition][key] might be not
null but still empty.
data[partition] might be in an intermediate invalid internal state.
- If you add a
Delete() function then reading may go through the same indeterminate states.
- When calling
Get() you can go through the same indeterminate states as above, plus if you add
Delete() then you might be in the middle of that (also
Delete() might be called after
KeyExists() and before
That said let's see few other improvments you can make:
ContainsKey() and then searching the value in the dictionary is pretty inefficient because search is done twice. There is a
TryGetValue() method for this:
When you have an
else section you may want to keep the
if condition positive, negation is a small
! symbol which may be unnoticed.
dictionary[key] = value you do not need to call
ContainsKey(), that check is already done efficiently inside the setter.
In code (C#7, if earlier version you can't use
out var and
sub has to be declared before):
public static void Insert(string partition, string key, object value)
if (data.TryGetValue(partition, out var sub))
sub[key] = value;
sub = new Dictionary<string, object>();
Now add locking also in the functions to read from the dictionary and you're almost done.
You do not validate parameters. I don't know if for your usage scenarios a null value (or an empty string) for
values are allowed or not. If not then you must validate your inputs. Something as simple as:
if (partition == null)
throw new ArgumentNullException(nameof(partition));
if (partition.Length == 0)
throw new ArgumentException("Partition cannot be an empty string", nameof(partition));
What about a string made of spaces? In that case you might change second check to:
throw new ArgumentException("Partition cannot be...", nameof(partition));
Now you can consider if
Monitor (what's used by
lock) is the most appropriate synchronization mechanism for your cache. If you expect multiple concurrent reads and (after an initial period) very few writes then a
ReadWriterLockSlim may give you much much better performance. Profile.
I left these points to the end but they're actually the first things you should consider.
What about testing? Static methods are a nightmare for testing (if you want to replace them with something else at calling point). You may use a default singleton instance (not the perfect thing to do but better than nothing).
Your class has nothing of a cache. It's a sort of
MultiMap but it has nothing to do with a cache. What I'd do? Design a not thread-safe
MultiMultiMap<TKeyOuter, TKeyInner, TValue> (eventually considering to implement
ILookup) which I might reuse somewhere else (even if the standard
MultiMap<TKey, TValue> is probably much more common).
Now I can design a thread-safe access class around it (call it cache if you want).
Now you should start putting some caching logic inside this class, in this moment you have everything at calling point (because
CacheService has too few responsibilities). What about:
- A method which accepts a
Func<object> instead of object itself?
- A method to clear the cache.
- A method to set an expiration for each item.
- A method to limit the memory size used for caching.
Then you might want to abstract cache storage (memory in this case) from cache interface. What if objects you're constructing are pretty huge and expensive to build? Maybe switching to disk storage is still better than re-build them (for example if they come from network data...). Move interface to a base class (or...an interface) and derive this memory cache from that.
Did you consider to use System.Runtime.Caching.MemoryCache instead?