The biggest problem with your code is that it's not thread-safe. You can't just access dictionary without a lock for reading and only lock it for writing. The problem is that while your code is reading, another thread could be writing to the dictionary and that means the reading could return bogus data/throw exception/whatever.
If you're trying to mimic double-checked locking, then that only works because the read there is atomic. But
TryGetValue is not atomic, so this approach doesn't work here.
You could make the code thread-safe by using
ReaderWriterLockSlim. You would enter the lock in the upgradeable mode first, then call
TryGetValue and if you find out you need to write, upgrade it to write mode. Though this requires the user to use the same
ReaderWriterLockSlim too and it only makes sense if the lock is often used in the read mode (so that the readers will work concurrently with your method; two threads in upgradeable mode can't work concurrently).
But this is a pretty complicated solution with uncertain benefits. I think a better option is just to
lock the whole method. And in that case, making the method thread-safe makes even less sense (see also below).
can I rely on the fact that locking on a dictionary to safely add/remove elements is a common convention
I don't think you should.
It seems your code is supposed to be used like this:
dictionary = "forty two";
var result = dictionary.GetOrAdd(42, () => "42");
Code like this would make me very nervous, because it looks like the call to
GetOrAdd() is missing a lock.
Thread-safe code that doesn't need a lock (like
ConcurrentDictionary) is great. Code that requires a lock always is still good: that's a simple rule, it's easy to remember, follow and check. Code that requires a lock only sometimes is bad, because it means the developer has to remember which methods are thread-safe and which aren't. And that's likely going to lead to bugs ("
dictionary.GetOrAdd() doesn't need a lock, so
dictionary.Add() doesn't either, right?").
Another issue is that sometimes you don't want to lock on the same object. It's not unusual to have two fields that need to be kept synchronized, so both share the same lock; your code doesn't allow that. Another possibility is that the user code is using another locking mechanism (
SpinLock, …) and your code won't work with that either.
The same method on
Func<TKey, TValue>, not just
Func<TValue>. This is done to avoid closure for code like:
dictionary.GetOrAdd(key, () => someFunction(key));
Instead, it's written as:
dictionary.GetOrAdd(key, k => someFunction(k));
which avoids allocating a temporary object for the closure. If performance/GC pressure is not a concern for you, you can ignore this.