Why lock("myLock") can cause problems?

Question

I have read the following in the Microsoft's C# Reference:

lock("myLock") is a problem because any other code in the process using the same string, will share the same lock.

What does it mean exactly?

Is the following code not working as I expect? (I expect that ReadyCount is consistent)

public class Calculator
{
    public int ReadyCount { get; private set; }

    public void IncreaseReadyCount()
    {
        lock ("ReadyCount")
        {
            ReadyCount++;
        }
    }

    public void Calculate()
    {
        Parallel.ForEach(list, litItem =>
        {
            IncreaseReadyCount();
        });
    }
}

To be clear: if *all* you want to do is a thread-safe increment of a counter: `Interlocked.Increment` is your friend (as long as you also use `Thread.VolatileRead` or similar when you want to read it, or `Interlocked.CompareExchange` to be absolutely 100% sure) — Marc Gravell, May 17 '17 at 08:47

score 4 · Accepted Answer · answered May 17 '17 at 08:27

4

The question is: what is the scope of that lock? you can't tell; it is at least equivalent to static/global to your Calculator type, but it could also be coincidentally shared by any other code anywhere that happens to do a ldstr 'ReadyCount' (which gives back the interned version) and lock on it (or use Monitor, etc). Unlikely, but a risk.

More to the point, it just isn't obvious to the casual reader, which is a problem. If you intended it to be a static/global lock, then this is equivalent to your code, but more obvious and without the risk of unrelated code taking the lock by chance:

static readonly object readyCountLock = new object();
...
lock(readyCountLock) {...}

With this, at least it is obvious what it is doing.

Personally, though, I'd be tempted to just use Interlocked.Increment(ref _someField) :)

answered May 17 '17 at 08:27

Marc Gravell

1,026,079
266
2,566
2,900

If I understand correctly, the problem is that the string literal "ReadyCount" is stored in the application as a global static object and if I just use in another lock the same string, it will be the same oject instance. But this can cause only that my application could be a bit slower if I would use this string literal in other places for locking. So in my case (only one parallel code block in the whole applicaton) does not make a difference. I have already replaced it to a static readonly object, just want to fully understand. – dvjanm May 17 '17 at 22:47
@jannagy02 it could technically *never* become available - another thread could take that lock and never release it - unlikely though. Your lock could also be at the instance level (per `Calculator` instance) rather than global, though. – Marc Gravell May 17 '17 at 22:55

Evk · Answer 2 · 2017-05-17T08:40:10.533

3

In addition to what Marc said, in real life people ofren try to lock on strings that might not be interned, such as locking on some key from the database record. Locking on strings only (kind of) works if you lock on interned string. But consider this:

// not interned, but both strings represent "test"
string lock1 = new string(new char[] { 't', 'e', 's', 't' });
string lock2 = new string(new char[] { 't', 'e', 's', 't' });
Task.Run(() =>
{
    lock (lock1) {
        Console.WriteLine("1 entered");     
        Thread.Sleep(1000);
    }
});
Task.Run(() =>
{
    lock (lock2)
    {
        Console.WriteLine("2 entered");
        Thread.Sleep(1000);
    }
});

This code immediately executes both "protected" sections, because despite of both strings are "test" - they are different instances. So locking on constant string is danregrous because it's global and you never know which else code uses such "lock", and locking on string variable is dangerous because it might just not work at all.

To answer a comment about locking on ReadyCount.ToString(). That's exactly how people try to do this in real life (where ReadyCount is some property of database record or similar). I guess by ReadyCount you mean some number, not really a string (otherwise calling ToString makes no sense). And no, this is also bad, because:

int readyCount = 1;
string lock1 = readyCount.ToString();
string lock2 = readyCount.ToString();
bool same = Object.ReferenceEquals(lock1, lock2);
// nope, not the same, lock will not work

edited May 17 '17 at 08:40

answered May 17 '17 at 08:33

Evk

98,527
8
141
191

1

agreed; either you're *probably* locking too widely (interned), or you're not actually locking *at all* (different values at different times); fun either way! – Marc Gravell May 17 '17 at 08:34
So would "lock ("ReadyCount".ToString())" also be a good aproach? – dvjanm May 17 '17 at 08:36
@jannagy02 that wouldn't change anything - `string.ToString()` is implemented as `return this;` - however, if it **did** change the output, then you'd be locking a different instance *every time*, so... you wouldn't actually be doing anything useful in the lock (a lock is only useful if competing threads lock against the same instance) – Marc Gravell May 17 '17 at 08:37
@jannagy02 I've updated answer, assuming that by ReadyCount you meant integer. – Evk May 17 '17 at 08:40
@Evk Thank you for your efforts, it is very helpful to understand locking. – dvjanm May 17 '17 at 08:44
@jannagy02 if you need to lock on integer (or string), which of course happens in practice - you can use a dictionary of locks, keyed by integer (or string). Then you first get lock object corresponding to target value (like RedyCount) or create new one, then lock on that. Like in this question for example: http://stackoverflow.com/q/34754211/5311735 – Evk May 17 '17 at 08:51

Why lock("myLock") can cause problems?

2 Answers2