Memory cache implementation

Question

I have created an open source memory cache called cachew and can be found here: cachew. I would like your help to make it better.

Regarding the class Cache or its dependencies:

1. I am thinking of removing timeoutStyle and timeout from the Cache constructor and providing default values that can be changed via public properties. How do you like that?
2. How do you like my abstraction LockManager for ReaderWriterLockSlim?
3. Do you think my timer in Cache should be configurable? Through a property in cache, change internal to public or by another method?
4. Do you think it is a bad practice to start a timer in a constructor? If so, what would you do?
5. Would you consider the Cache threadsafe? Could it be better implemented?

public enum TimeoutStyle
{
 FixedTimeout,
 RenewTimoutOnQuery
}
public class Cache : ICache
{
 private readonly ITimer expirationTimer;
 private readonly IInternalCache internalCache;
 private readonly LockManager lockManager = new LockManager();
 public Cache(TimeoutStyle timeoutStyle, TimeSpan timeout) :
 this(new InternalCache(timeoutStyle, timeout), new SystemTimer(5000))
 {
 }
 internal Cache(IInternalCache iternalCache, ITimer expirationTimer)
 {
 if (iternalCache == null) throw new ArgumentNullException("iternalCache");
 if (expirationTimer == null) throw new ArgumentNullException("expirationTimer");
 this.internalCache = iternalCache;
 this.expirationTimer = expirationTimer;
 this.expirationTimer.Elapsed += ExpirationTimerElapsed;
 this.expirationTimer.Start();
 }
 public object Get<T>(CacheKey key, Func<T> func)
 {
 using (lockManager.EnterRead())
 {
 object existingValue;
 if (internalCache.TryGetValue(key, out existingValue))
 return existingValue;
 }
 using (lockManager.EnterWrite())
 {
 object existingValue;
 if (internalCache.TryGetValue(key, out existingValue))
 return existingValue;
 var newValue = func();
 internalCache.Add(key, newValue);
 return newValue;
 }
 }
 private void ExpirationTimerElapsed(object sender, EventArgs e)
 {
 using (lockManager.EnterWrite())
 {
 internalCache.RemoveExpiredItems();
 }
 }
}

• RubberDuck: "There are two hard things in computer science. Na mining things, cache invalidation, and off by one errors." -- Welcome to Code Review!
• Snowbody: What happens if the timer event fires before the constructor is done? Yes I know the .Start() is the last line of the constructor, but the compiler or JIT may reorder things.
• Łukasz Zwierko: @Snowbody - "but the compiler or JIT may reorder things", I don't think it's actually true. There are certain things that compiler / JIT can optimize, but I'm pretty sure that this is not the case. Having said that, I don't like it either. Consider a situation: you create inherited cache class that does sth else in the constructor, after the timer starts. With very short timeouts you might run into situation when callback is executed before constructor finishes. It may not be a real-life case, but that's a danger of starting threads form ctor.
• Jakob: @Snowbody Like Heslacher pointed out I could start the timer when the first item is added?
• Jakob: @ŁukaszZwierko Like Heslacher pointed out I could start the timer when the first item is added? I should maybe also have the Cache class sealed.

Answer 1

I am thinking of removing timeoutStyle and timeout from the Cache constructor and providing default values that can be changed via public properties. How do you like that?

Does it make sense to change these properties in the middle of using the cache? Is it a desirable feature? If yes, go ahead. If not, and you just want to provide default values to make it easier to construct the class, then add a default constructor that calls the existing one.

I don't know enough C# to comment on the rest, I hope you'll get good reviews!

• Jakob: Thanks @janos, good idea. Id does not make sense to change these values. An extra constructor with default values sounds good.

Answer 2

First I thought, nice cache, nothing can be added to it but then I stumbled over this Get() method which should be a GetOrAdd() method instead.

public object Get<T>(CacheKey key, Func<T> func)
{
 using (lockManager.EnterRead())
 {
 object existingValue;
 if (internalCache.TryGetValue(key, out existingValue))
 return existingValue;
 }
 using (lockManager.EnterWrite())
 {
 object existingValue;
 if (internalCache.TryGetValue(key, out existingValue))
 return existingValue;
 var newValue = func();
 internalCache.Add(key, newValue);
 return newValue;
 }
}

Now it makes sense to first EnterRead() to check if the key is found. But basically you can just remove the first part and handle the check in the EnterWrite() lock. This would make the method more dry like so

public object GetOrAdd<T>(CacheKey key, Func<T> func)
{
 using (lockManager.EnterWrite())
 {
 object existingValue;
 if (internalCache.TryGetValue(key, out existingValue))
 {
 return existingValue;
 }
 var newValue = func();
 internalCache.Add(key, newValue);
 return newValue;
 }
} 

and now my favourite place to start the timer is found. It doesn't make sense to start the timer if nothing is in the cache, so I would start the timer just after internalCache.Add(key, newValue);.

Edit:

Based on the comment from RobH

Your first recommendation would kill throughput. ReaderWriter locks can have multiple read locks active but only one write lock.

if performance and throughput matters much, you shouldn't use this suggestion but you should at least name the method GetOrAdd().

If it is possible to change the LockManager to use a UpgradeableReadLock then the performance wise EnterRead() could be removed.

See https://stackoverflow.com/questions/2494104/readerwriterlockslim-question

---

What I would like to see is a Count property of the IInternalCache interface, so you could check this property in the constructor and start the timer if Count > 0.

Starting the timer which has an intervall of 5000 ms, which is used in the public constructor, shouldn't be a problem if done in the constructor.

Speaking about the constructor, I would add some vertical spaces like so

internal Cache(IInternalCache iternalCache, ITimer expirationTimer)
{
 if (iternalCache == null) throw new ArgumentNullException("iternalCache");
 if (expirationTimer == null) throw new ArgumentNullException("expirationTimer");
 this.internalCache = iternalCache;
 this.expirationTimer = expirationTimer;
 this.expirationTimer.Elapsed += ExpirationTimerElapsed;
 this.expirationTimer.Start();
} 

this looks more structured and it is easier to se what belongs together.

---

I would like to encourage you to always use braces {} for single statement if clauses to make your code less error prone.

---

• RobH: Your first recommendation would kill throughput. ReaderWriter locks can have multiple read locks active but only one write lock. Congrats on the C# gold!
• Jakob: Nice ideas, especially on how to handle the timer. Like @RobH wrote, the idea about the ReaderWriter is going to affect performance so I think there is no alternative. Best regards.
• Heslacher: Edited answer. Thanks @RobH for the congrats.
• Jakob: @Heslacher the naming GetOrAdd is a good idea. I thought about using EnterUpgradeableReadLock and simplify the code but only one thread can EnterUpgradeableReadLock at a time so it would effect performance...

Answer 3

Q&A

I am thinking of removing timeoutStyle and timeout from the Cache constructor and providing default values that can be changed via public properties. How do you like that?

• This would add additional complexity that is unlikely to be useful. Normal usage of a cache is to be a long-lived instance that is task-scheduled periodically given a fixed schedule.
• What you can do is provide overloads for the consumer not to care about defining timeout values or strategy.

How do you like my abstraction LockManager for ReaderWriterLockSlim?

• I don't see an advantage in abstracting threading constructs. It adds unnecessary complexity. Your cache is as thread-safe as the weakest link in the exotic implementation of the lock manager. Stick with well known best practices.
• Besides, from the perspective of TDD, these threading constructs are no bottlenecks.

Do you think my timer in Cache should be configurable? Through a property in cache, change internal to public or by another method?

• As with your first question, don't add this complexity. It is not worth it.

Do you think it is a bad practice to start a timer in a constructor? If so, what would you do?

• I do think it is bad practice to do anything else but assigning state in constructors. Provide a method Initialise or Start. Make it idempotent and call it when a request to cache an item is made.

Would you consider the Cache threadsafe? Could it be better implemented?

• Thread-safety depends for a great part on the implementation of LockManager.
• Acquiring a read lock, releasing it and then requiring a write lock does not feel right. Possible food for race conditions. Instead go for what is suggested by others, an upgradable lock.
• Since this is a long-lived object with a timer and possibly storing lots of items, I would implement IDisposable and call it on application exit. You'd have a clean way of releasing resources.