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()))
{
...
}
}
}
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.