Don't Lock Type Objects!

Collapse
This topic is closed.
X
X
 
  • Time
  • Show
Clear All
new posts
  • Nashat Wanly

    #1

    Don't Lock Type Objects!

    http://msdn.microsoft.com/library/de...ui06032003.asp

    Don't Lock Type Objects!

    Why Lock(typeof(Cla ssName)) or SyncLock GetType(ClassNa me) Is Bad
    Rico Mariani, performance architect for the Microsoft® .NET runtime
    and longtime Microsoft developer, mentioned to Dr. GUI in an e-mail
    conversation recently that a fairly common practice (and one that's,
    unfortunately, described in some of our documentation right now,
    although we'll be fixing that) is actually quite problematic. He asked
    if Dr. GUI could help get the word out that programmers shouldn't do
    this. The good doctor was delighted to oblige.

    What is this fairly common practice? Well, it's getting a lock on a
    type object. In C#, it's lock(typeof(Cla ssName)) where ClassName is
    the name of some class; in Microsoft® Visual Basic® .NET, it's
    SyncLock GetType(ClassNa me).

    A bit of background: The lock/SyncLock statement is used in
    multithreaded programming to create critical sections, or brief
    sections of your code where only one thread can execute at a time.
    (You might need this if you had to update more than one field in your
    object simultaneously— you'd want to make sure that another thread
    didn't try to update the object at the same time!) This statement
    locks the unique monitor object associated with the object you
    specify, waiting if another thread has the monitor already. Once it
    locks the monitor, no other thread can lock it until your thread
    releases the lock, which happens automatically at the end of enclosed
    block. A common usage is to lock the this/Me reference so that only
    your thread can modify the object you're using—or better yet, to lock
    the specific object you're about to modify. Locking the smallest
    possible object is good because it helps to avoid needless waiting.

    GetType and typeof return a reference to the type object for that
    type. The type object, of type System.Type, contains methods that
    allow you to reflect on the type, meaning you can find its fields and
    methods, and even access fields and call methods. You can even create
    an instance of the object once you have a reference to the type object
    (and you can get a reference to a type object by name if you use the
    Type.GetType shared/static method).

    So the type object is pretty handy. But some programmers have taken to
    using it as a "cheap" way of getting a proxy for a static/Shared
    object you can put a lock on. (And, unfortunately, we document this in
    both the C# and Visual Basic .NET documentation, implying that it's a
    recommended practice.) In this case, the docs are wrong (and we'll be
    correcting them). This practice is not acceptable, let alone
    recommended.

    Here's why: Since there's one type object for all instances of a
    class, it would appear that locking it would provide a lock equivalent
    to locking a static object contained in your class. You would lock all
    instances of the class, wait until other threads were done accessing
    any part of any instance, then lock access so you could access static
    members safely and without another thread interfering.

    And it does work, at least most of the time. But there are problems
    with it: First, getting the type object is actually a fairly slow
    process (although most programmers would guess that it's extremely
    fast); second, other threads in ANY CLASS and even different programs
    running in the same application domain have access to the type object,
    so it's possible that they'll lock the type object on you, blocking
    your execution entirely and causing you to hang.

    The basic problem here is that you don't own the type object, and you
    don't know who else could access it. In general, it's a very bad idea
    to rely on locking an object you didn't create and don't know who else
    might be accessing. Doing so invites deadlock. The safest way is to
    only lock private objects.

    But wait; it's even worse than all that. As it turns out, type objects
    are sometimes shared across application domains (but not across
    processes) in current versions of the .NET runtime. (This is generally
    okay since they're immutable.) That means that it's possible for
    ANOTHER APPLICATION running even in a different application domain
    (but in the same process) to deadlock your application by getting a
    lock on a type object you want to lock and never releasing it. And it
    would be easy to get access to that type object because the object has
    a name—the fully qualified name of the type! Remember that
    lock/SyncLock blocks (that's a polite word for hangs) until it can
    obtain a lock. It's obviously really quite bad to rely on a lock that
    another program or component can lock and cause you to deadlock.

    Even if the type objects were unique to your application domain, this
    would still be a bad practice because any code could get access to the
    type object for a public type and cause a deadlock. This is especially
    a problem when you use components in your application that you didn't
    write. (Even lock(this)/SyncLock Me can have this problem, since
    someone else can lock you. Although if that happens, it's likely to be
    easier to find than the deadlock caused by locking a type object,
    since your object isn't globally available across application
    domains.)

    So what should you do instead? Well, it's pretty easy: just declare
    and create an object to use as a lock, then use it, not the type
    object, to do your locking. Usually, to duplicate the intended
    semantics of the bad code, you'll want this object to be
    static/Shared—and it really, of course, should be private! In general,
    you could change the following bad code:

    // C#
    lock(typeof(Foo )) { // BAD CODE! NO! NO! NO!
    // statements;
    }

    ' VB .NET
    SyncLock GetType(MyClass ) ' BAD CODE! NO! NO! NO!
    ' statements
    End SyncLock

    …into the following good code:

    // C#
    lock(somePrivat eStaticObject) { // Good code!
    // statements;
    }

    ' VB .NET
    SyncLock GetType(somePri vateStaticObjec t) ' Good code!
    ' statements
    End SyncLock

    Of course, you'll have to either have a private static object to lock
    already (if you're using the lock to modify static objects, you may in
    fact already have one!) or you'll have to create one. (Make it private
    to keep other classes from locking your object.) Do NOT attempt to
    lock a field that's not a reference (object) type, such as
    int/Integer. You'll get a compiler error. If you don't have a private
    static object to lock, you may need to create a dummy object:

    // C#
    Class MyClass {
    private static Object somePrivateStat icObject = new Object();
    // methods of class go here--can lock somePrivateStat icObject
    }

    ' VB .NET
    Class MyClass
    Private Shared somePrivateStat icObject As New Object
    ' methods of class go here--can lock somePrivateStat icObject
    End Class

    You'll want to analyze each case separately to make sure you've got it
    right, but in general, the above technique should work.

    Note a couple of things: First, no code outside of your class can lock
    MyClass.somePri vateStaticObjec t, so you've eliminated many
    opportunities for deadlock. And deadlocks are among the nastiest bugs
    to find, so eliminating opportunities for them is a very good thing.

    Second, you know that there's exactly one copy of
    MyClass.somePri vateStaticObjec t for your application and exactly one
    for each other application on the system that is running, so there's
    no interplay across applications in the same application domain. Dr.
    GUI hopes you can see why this code is much more reliable and robust
    than the bad code.

    To summarize, don't lock type objects. You don't know where they've
    been. Doing so is slow and can expose you to possible deadlock
    situations. It's a bad programming practice. Instead, lock a static
    object in your object.
Working...