How does these lazy inline string-locks actually work, do they work, do they cause problem?

Question

So, I've been using strings as "uniquely identifiable mutexes", but I'm not sure if they're having the effect I intended them to have and now I'm having problems

I'm working in a multi-threaded application here and I'm facing weird behavior with my locks and I want to make sure that it's "Dumb locking" instead of "using dumb locks"

My locks look like this

private Stuff _stuff = null;
public Stuff Stuff {
    get {
        lock($"{this.SomeIdentifier}_stuff_initialization") {
            if(stuff == null) {
                stuff = new Stuff();
                stuff.DoLazyInitialization(this.SomeOtherLazyInitedStuff);
                return stuff;
            } else {
                return stuff;
            }
        }
    }
}

Too trash? Works? What should I be aware of when writing this type of code?

My guess will be that it is not working, but i can not back it up. I would rewrite it to use Lazy, ConcurrentDictionary or MemoryCache instead - which would be fairly easy. — Frank Nielsen, Dec 05 '19 at 14:35

score 3 · Accepted Answer · answered Dec 05 '19 at 14:37

3

There are problems using strings as mutexes for locking and a very good answer here:

Using string as a lock to do thread synchronization

In addition to that, there may be an issue with your logic. If the 'SomeIdentifier' is mutable, then you run into the possible race condition where:

Thread A calls get on object instance 1

Thread B updates SomeIdentifer on object instance 1

Thread A locks and instantiates stuff on object instance 1

Thread B calls get on object instance 1 (locks on a different string)

Thread A calls DoLazyInitialization

Thread B returns 'stuff' (as it is not null) prior to DoLazyInitialization completing.

answered Dec 05 '19 at 14:37

Paddy

33,309
15
79
114

1

So, the 'same thing but different instance' issue is a different thing and I don't think that locking is what you need for that. What you are looking for in that instance is some kind of concurrency check in your underlying data store - you won't be able to solve that via a lock I don't think. – Paddy Dec 05 '19 at 16:11
True, for a larger scale definitely; Currently there is 1 application accessing a lot of databases and it has been more than enough processing wise, the program is just a "glorified put this JSON in the RDBMS" no heavy duty; I also use these lame locks for memory caches, someone gave a good tip in the comments right above, I just didn't know those were a thing. – Felype Dec 05 '19 at 17:34
Large or small scale, I think the DB is the way to do this. Solving concurrency in your multi threaded C# code is a considerably more difficult thing to get completely right - good luck! – Paddy Dec 06 '19 at 09:27

Felype · Answer 2 · 2019-12-09T18:38:46.747

I have found out (the hard way) that actually locking with $ string is pretty bad, it does NOT work.

This is the code I used for testing:

object o = new object();
Parallel.For(0, 1000, (i) => {
    lock (o) {
        Console.WriteLine($"Hey {i}");
        Thread.Sleep(5000);
    }
});

It looks dumb because it is a silly test and what is expected happens: It locks the region correctly.

The following also works

var lid = "SomeId";
Parallel.For(0, 1000, (i) => {
    lock(lid) {
        Console.WriteLine($"Hey {i}");
        Thread.Sleep(5000);
    }
});

Parallel.For(0, 1000, (i) => {
    lock("SomeId") {
        Console.WriteLine($"Hey {i}");
        Thread.Sleep(5000);
    }
});

Now if you try to compose the string on the fly and use it as a locking mutex, it won't lock properly, it will generate many different instances of the same string and you'll get multiple different "locks" allowing different threads to access the resource.

This does not work

var lid = "SomeId";
Parallel.For(0, 1000, (i) => {
    lock($"{lid}_LOCKING") {
        Console.WriteLine($"Hey {i}");
        Thread.Sleep(5000);
    }
});

var lid = "SomeId";
Parallel.For(0, 1000, (i) => {
    lock(lid + "_LOCKING") {
        Console.WriteLine($"Hey {i}");
        Thread.Sleep(5000);
    }
});

The code below works, but I would avoid if the dynamic expression may generate way too many unique strings:

var lid = "SomeId"; Parallel.For(0, 1000, (i) => { lock(String.Intern($"{lid}_LOCKING")) { Console.WriteLine($"Hey {i}"); Thread.Sleep(5000); } });

I might have been lazy to test it this way before, but now I see it clearly it's a dumb way to lock. Do not lock with non-constant strings, even if the string components used in creating that string are immutable, it does not work;

You can solve this problem with `lock (String.Intern($"{lid}_LOCKING"))`. — Theodor Zoulias, Dec 06 '19 at 16:38

How does these lazy inline string-locks actually work, do they work, do they cause problem?

2 Answers2