[c#] Locking pattern for proper use of .NET MemoryCache

I assume this code has concurrency issues:

Actually, it's quite possibly fine, though with a possible improvement.

Now, in general the pattern where we have multiple threads setting a shared value on first use, to not lock on the value being obtained and set can be:

1. Disastrous - other code will assume only one instance exists.
2. Disastrous - the code that obtains the instance is not can only tolerate one (or perhaps a certain small number) concurrent operations.
3. Disastrous - the means of storage is not thread-safe (e.g. have two threads adding to a dictionary and you can get all sorts of nasty errors).
4. Sub-optimal - the overall performance is worse than if locking had ensured only one thread did the work of obtaining the value.
5. Optimal - the cost of having multiple threads do redundant work is less than the cost of preventing it, especially since that can only happen during a relatively brief period.

However, considering here that MemoryCache may evict entries then:

1. If it's disastrous to have more than one instance then MemoryCache is the wrong approach.
2. If you must prevent simultaneous creation, you should do so at the point of creation.
3. MemoryCache is thread-safe in terms of access to that object, so that is not a concern here.

Both of these possibilities have to be thought about of course, though the only time having two instances of the same string existing can be a problem is if you're doing very particular optimisations that don't apply here*.

So, we're left with the possibilities:

1. It is cheaper to avoid the cost of duplicate calls to SomeHeavyAndExpensiveCalculation().
2. It is cheaper not to avoid the cost of duplicate calls to SomeHeavyAndExpensiveCalculation().

And working that out can be difficult (indeed, the sort of thing where it's worth profiling rather than assuming you can work it out). It's worth considering here though that most obvious ways of locking on insert will prevent all additions to the cache, including those that are unrelated.

This means that if we had 50 threads trying to set 50 different values, then we'll have to make all 50 threads wait on each other, even though they weren't even going to do the same calculation.

As such, you're probably better off with the code you have, than with code that avoids the race-condition, and if the race-condition is a problem, you quite likely either need to handle that somewhere else, or need a different caching strategy than one that expels old entries†.

The one thing I would change is I'd replace the call to Set() with one to AddOrGetExisting(). From the above it should be clear that it probably isn't necessary, but it would allow the newly obtained item to be collected, reducing overall memory use and allowing a higher ratio of low generation to high generation collections.

So yeah, you could use double-locking to prevent concurrency, but either the concurrency isn't actually a problem, or your storing the values in the wrong way, or double-locking on the store would not be the best way to solve it.

*If you know only one each of a set of strings exists, you can optimise equality comparisons, which is about the only time having two copies of a string can be incorrect rather than just sub-optimal, but you'd want to be doing very different types of caching for that to make sense. E.g. the sort XmlReader does internally.

†Quite likely either one that stores indefinitely, or one that makes use of weak references so it will only expel entries if there are no existing uses.