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

Solution 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.

Solution 2

I'd say it's a really bad idea, personally. That isn't what strings are for.

(Personally I dislike the fact that every object has a monitor in the first place, but that's a slightly different concern.)

If you want an object which represents a lock which can be shared between different instances, why not create a specific type for that? You can given the lock a name easily enough for diagnostic purposes, but locking is really not the purpose of a string. Something like this:

public sealed class Lock
{
    private readonly string name;

    public string Name { get { return name; } }

    public Lock(string name)
    {
        if (name == null)
        {
            throw new ArgumentNullException("name");
        }
        this.name = name;
    }
}

Given the way that strings are sometimes interned and sometimes not (in a way which can occasionally be difficult to discern by simple inspection), you could easily end up with accidentally shared locks where you didn't intend them.

Solution 3

Locking on strings can be problematic, because interned strings are essentially global.

Interned strings are per process, so they are even shared among different AppDomains. Same goes for type objects (so don't lock on typeof(x)) either.

Solution 4

I had a similar issue not long ago where I was looking for a good way to lock a section of code based on a string value. Here's what we have in place at the moment, that solves the problem of interned strings and has the granularity we want.

The main idea is to maintain a static ConcurrentDictionary of sync objects with a string key. When a thread enters the method, it immediately establishes a lock and attempts to add the sync object to the concurrent dictionary. If we can add to the concurrent dictionary, it means that no other threads have a lock based on our string key and we can continue our work. Otherwise, we'll use the sync object from the concurrent dictionary to establish a second lock, which will wait for the running thread to finish processing. When the second lock is released, we can attempt to add the current thread's sync object to the dictionary again.

One word of caution: the threads aren't queued- so if multiple threads with the same string key are competing simultaneously for a lock, there are no guarantees about the order in which they will be processed.

Feel free to critique if you think I've overlooked something.

public class Foo
{
    private static ConcurrentDictionary<string, object> _lockDictionary = new ConcurrentDictionary<string, object>();

    public void DoSomethingThreadCriticalByString(string lockString)
    {
        object thisThreadSyncObject = new object();

        lock (thisThreadSyncObject)
        {
            try
            {
                for (; ; )
                {
                   object runningThreadSyncObject = _lockDictionary.GetOrAdd(lockString, thisThreadSyncObject);
                   if (runningThreadSyncObject == thisThreadSyncObject)
                       break;

                    lock (runningThreadSyncObject)
                    {
                        // Wait for the currently processing thread to finish and try inserting into the dictionary again.
                    }
                }

                // Do your work here.

            }
            finally
            {
                // Remove the key from the lock dictionary
                object dummy;
                _lockDictionary.TryRemove(lockString, out dummy);
            }
        }
    }
}

View more solutions

Comments

[Brian Gideon]

I point out some of the quirks of using a string as the lock target in my answer here.

[Neil]

May I suggest a slight change? lock(_locks.GetOrAdd(_s, s => new object())) This makes it so you only create one object per string, instead of creating one every time you call LockMethod().

[Oliver]

Concerning the problems with unequal string instances mentioned both by JonSkeet and BrianGideon, I'd suggest using lock(_locks.GetOrAdd(String.Intern(_s), s => new object())), although interning might still be turned off through CompilationRelaxations.NoStringInterning and then this won't work, either. Maybe, using the string's hashcode as a key in the dictionary would help here?!

[Zaid Masud]

@Oliver interning is a problem when you are locking on a string (someone else can lock on the same interned string instance). Here we lock on a new object instance, not on the string instance. No one else has access to this new object instance.

[HooYao]

how to safely delete the keyed lock object?

[Zaid Masud]

Thanks ... would a static ConcurrentDictionary<string, object> to replace the HashSet<string> that creates a new lock object for every string be a good approach?

[Jon Skeet]

@ZaidMasud: You'd still have a static (global) set of locks, which doesn't sound like a good design to me. I very rarely find that statics for mutable state ends up being a good thing.

[Zaid Masud]

I'm not sure if I'm following this completely... if it's a private static Dictionary the lock objects would be contained to that class only. Also I'm not sure what the new lock type would look like, unless you mean new object instance as @HenkHolterman is suggesting.

[Jon Skeet]

@HenkHolterman: I very much meant create a specific type, e.g. a class called "Lock".

[Jon Skeet]

@ZaidMasud: It's still static, which makes it a pain for testing etc. It's a global variable, even if it's not globally visbile... The Lock class wouldn't need to have any extra members at all, unless you wanted to give it a name property (and override ToString) - the point is it would make it extremely clear to anyone reading the code what a variable of type Lock was for.

[leppie]

Sometimes per-process lock variables are what you are looking for ;p

[Zaid Masud]

@JonSkeet I would suggest a code sample of the suggested Lock type in the answer.

[Zaid Masud]

+1 for the approach and idea... although unless I'm missing something the instances of Lock objects would still need to be stored statically somewhere so as I understand it advantage of the new type is pretty much for readability.

[Zaid Masud]

Accepted as I thought this answer most comprehensively addressed the specific questions.

[Jon Skeet]

@ZaidMasud: There are two entirely separate concerns here: a) a global collection of locks (avoid if possible); b) what to lock on (not strings; object would be okay, Lock would be better)

[Brian Rasmussen]

@leppie certainly, but it may not be obvious that strings/type objects have this "feature".

[Zaid Masud]

By the way it seems that string interning is only an issue for literal strings. So if the strings are not being created as literals the whole concern is a bit overblown in my opinion.

[Brian Rasmussen]

Literal strings are interned by default, but you can intern any string if you want. The point here is that you can't tell if the string is interned or not by simply looking at the usage of a string reference. IMO there's no benefit in locking on a string and given that it may cause hard to find problems in some situation it is better to just avoid it.

[Oleks]

Wouldn't 1) use of explicit this.name = String.Intern(name); and 2) use of e.g. a const prefix for the name this.name = String.Intern(Prefix + name); be an improvement for the Lock class?

[Jon Skeet]

@Alex: No, I really don't think so - you'd basically have a global namespace for the lock... how do you know what other code is going to use? Yes, interning would ensure that you get the lock for that name, but where's the rigour in making sure that only the appropriate code uses it?

[user1005462]

interest, is it described somewhere or someone tested it?

[Brian Rasmussen]

@user1005462 it is easy to verify in the debugger as the same instance will be returned across app domains.

[traveler3468]

Thanks @JonSkeet I was having an issue when i was using multiple threads that we accessing the same string but with different values on each thread, in my case a connection string, long story short sometimes the string would be a value of a different connectionstring even though it was not set to it, using the above lock like class worked! thank you!

[mdarefull]

For future readers, the reason I found to lock on strings is that there might be scenarios where we don't want to lock all the threads but a set of related threads identified by such string. Ex.: Fetching an item from a cache, while requesting it from an API if it is not located. I'd like that only one request is made to the API to retrieve the item while all the other related threads wait for it to be available. At the same time, I don't want to block other threads from requesting other items. So, I lock on the item key. I used the same approach while refreshing OAuth access tokens.

[HooYao]

Removing the lock object looks not right, if a 2nd thread is holding and waiting for this particular lock object, and then a 3rd steps in, there is no object in the dictionary, then the 3rd thread will add a new object for the same key, and hold a different lock object from the 2nd thread, then the mutual exclusion is broken.

[Bill Ravdin]

Removing the lock object is necessary because otherwise no other threads would be able to execute. After the lock object is removed, the outer lock will be released. Then any threads waiting on the inner lock will continue execution. The first thread that adds its lock object to the dictionary will continue and the rest will wait on the inner lock again.

[Taha Ali]

I suggest this answer for more depth - stackoverflow.com/a/19375402/9350845