Is it OK to use a string as a lock 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.

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.

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.