1

As I know it's recommended to put lock for custom event add/remove methods. Also when C# compiles event it refers to the internally created delagate field. I need to use not event but private delegate and public Add/Remove methods, should I also use locks in this case as shown in the code below? 

private EventHandler _delegate;
public void Add(EventHandler e)
{
    lock(_locker)
    {
        _delegate += e;
    }
}
public void Remove(EventHandler e)
{
    lock(_locker)
    {
        _delegate -= e;
    }
}

UPD after comments: why I still need (or I don't?) to use lock with following implementation where I use custom event's add/remove but delegate variable and not event variable inside:

 private EventHandler _delegate;
 public event EventHandler MyEvent
 {
    add
    {
        lock (_locker)
        {
            _delegate += value;
        }
    }
    remove
    {
        lock (_locker)
        {
            _delegate += value;
        }
    }
 }

UPD2: I do not think it's duplicated question because I asked not about standard obj.myEvent += onMyEventHandler thread safety (this is answered) but rather about add/remove accessors of event and about custom Add(), Remove() methods.

tytyryty
  • 741
  • 7
  • 17
  • 1
    The C# compiler already generates that code. It is much better than your version, the *lockTaken* argument of Monitor.Enter is a fair amount of rocket science. Just don't help. If you want to understand it then start by using a disassembler. – Hans Passant May 20 '16 at 21:48
  • Hans Passant, okay, but why C# compiler doesn't generate this code for custom event's add/remove and we still need to write lock there? – tytyryty May 20 '16 at 21:56
  • 1
    It is a custom event, the C# compiler is not supposed to meddle with it. You are telling the compiler to know how to do it better. It completely takes your word for it, now it is up to you do actually do it better. – Hans Passant May 20 '16 at 22:46
  • 1
    I wouldn't say that this question is a duplicate per se of the _many_ existing Q&As on Stack Overflow discussing thread safety and event implementations. However, that's only because it is impossible for anyone to answer this question without more information. Does your code need thread safety? Then you need to implement your event accessors in a thread-safe way (e.g. with `lock`), and this is true whether you just have `Add()` and `Remove()` methods or you use an actual C# `event`. But only you can decide whether your code needs to be thread-safe. – Peter Duniho May 21 '16 at 00:34
  • 1
    Personally, I would always implement the accessors in a thread-safe way (and typically, would use the compiler's implementation rather than writing my own), even if the current code base didn't need thread-safety, because eventually someone's going to need thread-safety and incorrectly assume it was already implemented in a thread-safe way, and then you're screwed. – Peter Duniho May 21 '16 at 00:35
  • Peter, thank you! I think your comment answers my question in a best way, as you said if I have my own event accessors add/remove or if I have my own methods Add(), Remove() for delegate combining, then I need to implement them in thread-safe way. – tytyryty Mar 30 '17 at 23:32
  • Still, I do not think it's duplicated question because I asked not about standard obj.myEvent += onMyEventHandler but rather about add/remove accessors and custom Add(), Remove() methods. – tytyryty Mar 30 '17 at 23:34

0 Answers0