7

We had a discussion at work regarding locking and what exactly happens. The code that triggered this discussion is:

        string name = (string)context.Cache[key];

        if (String.IsNullOrEmpty(name)){

            lock (typeof(string)){
                name = (string)context.Cache[key];
                //.. other code to get the name and then store in the cache
            }
        }

I see this as straight-forward: look for a value in the cache, if it's not there then get a lock so as nothing else interrupts whilst the code gets the name and stores it in the cache.

Our discussion focused on whether (typeof(string)) is the best way of doing things, and what exactly is does.

My question is what exactly does lock(typeof(string)) do? Does it create a local string to be used a lock or is it creating something with a wider scope and therefore potentially un-safe.

MSDN lock statement

Daniel Hollinrake
  • 1,768
  • 2
  • 19
  • 39
  • 3
    No, it locks on the whole type - [and it can be dangerous](http://stackoverflow.com/questions/1603008/why-is-lock-typeof-mytype-a-problem). If it just created a random string to lock on, how would other locks know to lock on the same instance of that string? I would recommend you just make a dummy object to lock on like `private object _lockOnThis = new object()`. – vcsjones Oct 30 '13 at 15:46
  • I think typeof(SomeType) will always return the same Type instance for the same type. So you would use a globally available instance as a lock which is not the best idea IMHO. – helb Oct 30 '13 at 15:50
  • @vcsjones I'm currently removing all instances of this way of locking but was interested in what exactly it did. – Daniel Hollinrake Oct 30 '13 at 16:07

3 Answers3

8

My question is what exactly does lock(typeof(string)) do?

It locks on the Type object referred to by the reference that the typeof operator returns.

That means any code which does the same thing anywhere within the same process (or at least the same AppDomain) will share the same lock. Sounds like a bad idea to me.

I would suggest you create an object just for locking on:

private static readonly object CacheLock = new object();

...

lock (CacheLock)
{
    ...
}

That way you can easily see what's going to lock on that object.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • What would the benefit of locking on a dedicated object be over locking on the principal shared resource? Say if this cache is just an object that is declared in the same fashion as your `CacheLock`. – Michael J. Gray Oct 30 '13 at 15:50
  • @MichaelJ.Gray: It means you can easily check what's locking on that object. Do you know whether any code within .NET itself locks on the cache? I don't. – Jon Skeet Oct 30 '13 at 15:52
  • I guess I am a little confused with your comment. How is it easier to check if something is locking on `CacheLock` rather than the cache you are going to be accessing (not typeof(Cache) mind you)? If I have a `lock (this.cache) { .. }` it should be more correct, I believe. I am uncertain as to why the trend has begun with making something like `lock (this.cacheLocker) { .. }` against single objects; I can understand the `cacheLocker` for complex processes where types are internally thread safe and expose public thread safe methods. – Michael J. Gray Oct 30 '13 at 15:53
  • @MichaelJ.Gray: `CacheLock` is private. I can easily see all the code which can see it at all. Lots of code *could* access the `Cache` - anything which can use `context.Cache`, for starters... – Jon Skeet Oct 30 '13 at 15:56
  • Fair enough. I suppose it is applicable to lock the resource (nearly) only when your shared resource is not private and has the ability to be accessed outside of its parent class. – Michael J. Gray Oct 30 '13 at 16:02
  • 1
    @MichaelJ.Gray: Well don't forget that if you do anything *with* the resource, that might lock on `this`, too. Personally I wish we didn't have the ability to lock on arbitrary objects... – Jon Skeet Oct 30 '13 at 16:03
  • Ah, yes. That is very true. If the shared resource is locking on `this`, as was common a long time ago, before the SyncRoot practice, you would run into some unnecessary contention and possibly deadlocks. Good point! I don't believe locking on `this` is a recommended practice anymore, but I could be wrong. I do agree though, it would be cool to have a lock statement that locks on everything inside its scope instead of letting you specify arbitrary objects; but then it would be annoying to make special locks for complicated concurrent execution plans. – Michael J. Gray Oct 30 '13 at 16:08
  • @MichaelJ.Gray: That's not what I was suggesting at all - I was suggesting that you should *only* be able to lock on specific types (e.g. make `Monitor` a non-static class...) – Jon Skeet Oct 30 '13 at 16:10
  • My apologies. I thought of an idea and my mind ran with it. I get what you mean now. Thanks :) – Michael J. Gray Oct 30 '13 at 19:18
2

If you lock on a Type it will mean that you have a mutual access exclusion based on that instance of the Type. The implications are that two threads in the application doing this will inadvertently block each other or cause unforeseen deadlocks.

Remember, typeof(someType) just returns a Type instance.

It is typically a best practice to dedicate an object to locking a complex process, such as declaring a readonly object in your class. If the lock just needs to go around some access to a private variable, say a collection, then locking that collection is quite fine.

Michael J. Gray
  • 9,784
  • 6
  • 38
  • 67
1

As shown on the page you link to:

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

lock (typeof (MyType)) is a problem if MyType is publicly accessible.

Community
  • 1
  • 1
CodeCaster
  • 147,647
  • 23
  • 218
  • 272