Another bad lock pattern


A while ago I asked Dr. GUI to post an MSDN article about the perils of using lock(typeof(Foo)), you can still find that article here.


But recently I’ve started seeing another pattern that’s just as bad:



class MyClass
{
    private static String myLock = “MyLock“;


    public void Foo() 
   {
     lock(myLock) { … }
   }
}


This is bad because string literals are normally interned, meaning that there is one instance of any given string literal for the entire program.  The exact same object represents the literal in all running appdomains, on all threads.  So if someone else comes along and locks a literal named “MyLock” his literal will interfere with yours.


The thing I recommend is:


private static Object myLock = new Object();


That’s the safest.

Comments (13)

  1. Are you regretting your decission to enable any object to be used as a mutex yet? ;>

  2. Ian Ringrose says:

    When I implement a singleton in C#, I often use typeof(class), is this a real problem. I do not see how it can give a dead lock. After all one else should be using my class in a lock statement. I agree with using a separate lock object if the locking is any more complex then this.

    class A

    (

    private A()

    {

    // some complex code…

    }

    public static A Get()

    {

    if (msOneAnyOnly == null)

    {

    lock(typeof(A))

    {

    if (msOneAnyOnly == null)

    {

    msOneAnyOnly = new A();

    }

    }

    }

    return msOneAnyOnly;

    }

    private A msOneAnyOnly;

    )

  3. Rico Mariani says:

    There are three problems with lock(typeof(A)):

    First: type objects are agile, so the same type object is used in all appdomains. That means if your code is running in two different appdomains they will share the same lock which is probably a bad idea.

    Second: type objects are public, so anyone anywhere can do lock(typeof(A)) which could totally mess your algorithm up.

    Third: typeof() isn’t free, you’ll have to drag in a bunch of reflection code.

    So, avoiding lock(typeof(X)) results in faster, simpler, and more correct code. In contrast I can’t think of any benefits to using lock(typeof(X)) under any circumstances.

  4. Rico Mariani says:

    Clarification: In your particular example the first problem doesn’t matter much. Really the third is the big issue. It’s much cheaper to have a static variable that holds the lock explicitly.

  5. Wesner Moise says:

    In an NGEN app, doesn’t using a static initializer introduce new additional checks before every function call?

    Thus, in fixing the lock, we slowed the class down.

  6. Rico Mariani says:

    I think typically not. If you put the test in the constructor for instance, then you mostly don’t have to worry about instance methods being called having not initialized the statics because if you have an instance then you necessarily have run the constructor.

    There are complicated rules for this but it ends up not costing too much. Also this is only an issue for domain nuetral code.

    In any case, the cost of typeof() is greater than just a single test… even if there was an extra test in every method that used the lock, it would still be faster.

  7. Rico Mariani says:

    Actually, I think going into the exact details of this process would make an interesting blog/article. First I’ll see if someone has already done a good job on this topic.

  8. The “Request is not available in this context” exception is one of the most common errors

  9. As we have found in deploying 2 projects to the new release of IIS in the past month that there were

  10. As we have found in deploying 2 projects to the new release of IIS in the past month that there were

  11. The “Request is not available in this context” exception is one of the more common errors you may receive