Is it OK to use a string as a lock object?

Question

I need to make a critical section in an area on the basis of a finite set of strings. I want the lock to be shared for the same string instance, (somewhat similar to String.Intern approach).

I am considering the following implementation:

public class Foo {     private readonly string _s;     private static readonly HashSet<string> _locks = new HashSet<string>();      public Foo(string s)     {         _s = s;         _locks.Add(s);     }      public void LockMethod()     {         lock(_locks.Single(l => l == _s))         {             ...         }     } } 

Are there any problems with this approach? Is it OK to lock on a string object in this way, and are there any thread safety issues in using the HashSet<string>?

Is it better to, for example, create a Dictionary<string, object> that creates a new lock object for each string instance?

---

Final Implementation

Based on the suggestions I went with the following implementation:

public class Foo {     private readonly string _s;     private static readonly ConcurrentDictionary<string, object> _locks = new ConcurrentDictionary<string, object>();      public Foo(string s)     {         _s = s;     }      public void LockMethod()     {         lock(_locks.GetOrAdd(_s, _ => new object()))         {             ...         }     } } 

Answer 1

Locking on strings is discouraged, the main reason is that (because of string-interning) some other code could lock on the same string instance without you knowing this. Creating a potential for deadlock situations.

Now this is probably a far fetched scenario in most concrete situations. It's more a general rule for libraries.

But on the other hand, what is the perceived benefit of strings?

So, point for point:

Are there any problems with this approach?

Yes, but mostly theoretical.

Is it OK to lock on a string object in this way, and are there any thread safety issues in using the HashSet?

The HashSet<> is not involved in the thread-safety as long as the threads only read concurrently.

Is it better to, for example, create a Dictionary that creates a new lock object for each string instance?

Yes. Just to be on the safe side. In a large system the main aim for avoiding deadlock is to keep the lock-objects as local and private as possible. Only a limited amount of code should be able to access them.