should I lock 'event'?

Question

should I lock event in the following case:

event foo;

thread A: will call foo += handler;

thread B: will call foo -= handler;

should I lock foo?

Answer 1

Locking on foo is a bad idea, because the value will change each time. You should lock on a variable which doesn't change:

private readonly object eventLock = new object();
private EventHandler fooHandler;

public event EventHandler Foo
{
    add
    {
        lock (eventLock)
        {
            fooHandler += value;
        }
    }
    remove
    {
        lock (eventLock)
        {
            fooHandler -= value;
        }
    }
}

private void OnFoo(EventArgs e)
{
    EventHandler handler;
    lock (eventLock)
    {
        handler = fooHandler;
    }
    if (handler != null)
    {
        handler(this, e);
    }
}

Note that if you use a field-like event, like this:

public event EventHandler Foo;

then you'll automatically get a "lock(this)" on add/remove, although you'd have to manually add it when fetching the handler before calling it (assuming you want to make sure you read the most recently written value). Personally I'm not a fan of locking on "this", but you may not mind - and it certainly makes for simpler code.