C# lock on string value

Question

Our codebase is full of locks such as:

lock("Something." + var)
{
 ....
}

This causes issues due to strings not meant to be used in locks in this way. We have a need to lock on dynamic strings, so this is the replacement based on the implementation of the lock object specified here: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock

public class StringLock : IDisposable
{
 private static readonly ConcurrentDictionary<string, object> LockObjects = new ();
 private string Key { get; }
 private object LockObject { get; }
 private readonly bool _lockWasTaken;
 public StringLock(string key)
 {
 Key = key;
 LockObject = LockObjects.GetOrAdd(Key, new object());
 _lockWasTaken = false;
 System.Threading.Monitor.Enter(LockObject, ref _lockWasTaken);
 }
 public void Dispose()
 {
 if (_lockWasTaken) System.Threading.Monitor.Exit(LockObject);
 LockObjects.TryRemove(Key, out _);
 }
}

Usage:

using (new StringLock("test"))
{
 // Locked
}

• Theodor Zoulias: Related StackOverflow question: How to dynamically lock strings, but remove the lock objects from memory?

Answer 1

Thread 1 and Thread 2 call into StringLock("test"). One of them wins, lets say Thread 1. Thread 1 goes into Monitor.Enter. Thread 2 waits. Thread 1 exits and removes the lock object from ConcurrentDictionary. Thread 2 enters the Monitor.Enter. Thread 3 calls into StringLock("test") and gets a new lock object, because not in the ConcurrentDictionary anymore, and can proceed while Thread 2 is still in the "lock".

The hard part here is knowing when it's safe to remove from LockObjects.

Could make a more complex lock object instead of object that contains a counter. Something like the WaitingWriteCount of the ReaderWriterLockSlim class. Then instead of using GetOrAdd on concurrent dictionary use AddOrUpdate to increment a count if object exist. Still could need grab the object again before releasing the lock and decrement the counter.

Another option could be the ConditionalWeakTable but strings could be a bad option for the key and would still need to have some mapping object between strings and the key.

Update

You could use the AddOrUpdate method to know when it's safe to remove the lockobject.

public class StringLock : IDisposable
{
 private static readonly ConcurrentDictionary<string, Tuple<object, object>> LockObjects = new ConcurrentDictionary<string, Tuple<object, object>>();
 private readonly Tuple<object, object> _lockTuple;
 private readonly string _key;
 private bool _lockTaken = false;
 public StringLock(string key)
 {
 _key = key;
 // Add a new tuple or if updating just replace the "holder" part of the tuple so all locks share common object but each call has own object for the call
 _lockTuple = LockObjects.AddOrUpdate(key, _ => Tuple.Create(new object(), new object()), (_, x) => Tuple.Create(x.Item1, new object()));
 Monitor.Enter(LockObject, ref _lockTaken);
 }
 private object LockObject => _lockTuple.Item1;
 public void Dispose()
 {
 if (_lockTaken)
 {
 Monitor.Exit(LockObject);
 }
 // Cast to IDictionary to do Atomic remove if the "Value" hasn't changed from the update statement
 // gist taken from https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/
 var dict = (IDictionary<string, Tuple<object, object>>)LockObjects;
 dict.Remove(new KeyValuePair<string, Tuple<object, object>>(_key, _lockTuple));
 }
 public override string ToString()
 {
 return _key;
 }
}

Here we are using a tuple's item1 to hold the object that is to be locked on with all threads. The tuple's item2 is just a unique object for each call. We use AddOrUpdate to change Item2 to a new object if the key exists in the ConcurrentDictionary. Then we can cast the ConcurrentDictionary to IDictionary and use the Atomic remove where it will only remove if both the key and value are the same and haven't been changed.

• Tom Gullen: Thanks makes sense, native lock is so cheap I wonder if wrapping the constructor in a lock would work?
• CharlesNRice: @TomGullen updated answer with some code that might be a better option. I can't think of reason why it wouldn't work. If there is a problem hopefully someone from CR will see it and post about it.
• CharlesNRice: Another thing to be aware of this will not play well with async code. The old lock way would tell you that can't have async but it won't tell you when using the using command. If need it to work with async then would need to switch from locks to semaphoreslim
• Theodor Zoulias: From the looks of it the StringLock class is buggy. It works under the assumption that the first thread that calls LockObjects.AddOrUpdate will also be the first thread that calls Monitor.Enter. This is not a given. The first thread might be preempted by the second thread, resulting in the second thread removing the key from the dictionary while the first thread has acquired the lock. The result of this race condition is the violation of the exclusive execution policy per key.
• CharlesNRice: True. Didn't think of that. Thanks for pointing it out. I agree probably slight chance it happens but still should have been accounted for.