CODEWITHSUNDEEP
c#locking
Erik Funkenbusch
Erik Funkenbusch

Reputation: 93454

Locking on an object

I often see code like that which is shown here, ie where an object is allocated and then used as a "lock object".

It seems to me that you could use any object for this, including the event itself as the lock object. Why allocate a new object that does nothing? My understanding is that calling lock() on an object doesn't actually alter the object itself, nor does it actually lock it from being used, it's simply used as a placeholder for multiple lock statements to anchor on.

public class Shape : IDrawingObject, IShape
{
    // Create an event for each interface event
    event EventHandler PreDrawEvent;
    event EventHandler PostDrawEvent;

    object objectLock = new Object();

    // Explicit interface implementation required.
    // Associate IDrawingObject's event with
    // PreDrawEvent
    event EventHandler IDrawingObject.OnDraw
    {
        add
        {
            lock (objectLock)
            {
                PreDrawEvent += value;
            }
        }
        remove
        {
            lock (objectLock)
            {
                PreDrawEvent -= value;
            }
        }
    }
}

So my question is, is this really a good thing to do?

Upvotes: 5

Views: 2773

Answers (3)

Marc Gravell
Marc Gravell

Reputation: 1063944

including the event itself

No, you can't do that. An "event" is actually just some accessor methods. Assuming you mean the backing delegate, that would be very bad - delegates are immutable: every time you add/remove a subscriber, you get a different delegate.

Actually, the 4.0 compiler now does this with lock-free code using Interlocked - it may be worth following this approach instead.

In your example, the objectLock ensures that all callers (to that instance) are locking against the same object, which is important - but without the ugliness of locking on this (which is how the C# compiler used to work).

--

Update: your example shows code that is necessary before C# 4.0, access to a field-like-event inside the type talked directly to the field: the normal field-like-event locking was not respected. This was changed in C# 4.0; you can now (in C# 4.0) safely re-write this as:

public class Shape : IDrawingObject, IShape
{
    // Create an event for each interface event
    event EventHandler PreDrawEvent;
    event EventHandler PostDrawEvent;

    event EventHandler IDrawingObject.OnDraw
    {
        add { PreDrawEvent += value; }
        remove { PreDrawEvent -= value; }
    }
}

All the correct behaviour is then followed.

Upvotes: 7

Hans Passant
Hans Passant

Reputation: 942227

Any private reference type member will do the job. As long as it is private and never gets reassigned. Which knocks a delegate object out of the running, you definitely don't want to see a lock fail to do its job simply because client code that you don't control assigns an event handler. Extremely hard to debug.

Using private members that do another job is not something that scales well. If you find out you need to lock another region of code while refactoring or debugging, you'll need to find another private member. Which is where things can turn sour quickly: you might pick the same private member again. Deadlock knocking on the door.

This doesn't happen if you dedicate a lock object to a specific set of shared variables that need to be protected. Allows you to give it a good name too.

Upvotes: 2

Darin Dimitrov
Darin Dimitrov

Reputation: 1039398

It is recommended to lock on a private static field because this ensures that multiple threads trying to access the lock in parallel will be blocked. Locking on the instance of the class itself (lock(this)) or some instance field could be problematic because if two threads call the method on two different instances of the object they will be able to simultaneously enter the lock statement.

Upvotes: 1

Related Questions