Multi-threaded concerns when coding event accessors

Question

I'm looking for additional information about why Microsoft recommends locking on the object when coding event accessor syntax. Microsoft's code example is shown below and the recommendation is linked to.

I understand the general concept about locking over a section of code to control multi-threaded access to it, however I'm looking for the specific reason why those concerns come into play when coding custom event accessor logic in context of Microsoft's example shown.

The following example shows how to implement custom add and remove event accessors. Although you can substitute any code inside the accessors, we recommend that you lock the event before you add or remove a new event handler method.

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

~ via https://msdn.microsoft.com/en-us/library/bb882534.aspx?f=255&MSPPError=-2147217396

Could the event handle be removed at the same time that the event is raised? Would guess that that may result in a null reference exception. — C. Knight, Feb 22 '16 at 17:30
I'm not 100% sure, but I assume it is because an event is basically a multicast delegate. As such, when you add/remove a handler, a *new* delegate is actually created. So, it is actually a two-step operation: generating the new delegate, and assigning to the existing variable. That introduces a window for race conditions if multiple threads are (un)subscribing at the same time. — Glorin Oakenfoot, Feb 22 '16 at 17:37
A further note, I would think that locking on the event you are modifying would be bad form for the above reasons. Would seem more sensible to have a dedicated lock object. — Glorin Oakenfoot, Feb 22 '16 at 17:38
@C.Knight - That is a possibility, though it doesn't have anything to do with the above logic. You should always assign the event to a local variable, *then* check it for null before attempting to invoke it. — Glorin Oakenfoot, Feb 22 '16 at 17:41
+= is not atomic. It's a 3-step process which leaves your PreDrawEvent in an invalid state until all 3 steps are completed: (1) copy existing PreDrawEvent into a register, (2) append to event in the register, (3) copy newly appended list back to the variable... Answered by [exactly the guy you'd expect](http://stackoverflow.com/a/443433/4747123) here... — bri, Feb 22 '16 at 17:47
Doesn't PreDrawEvent get changed, making it useless as a lock object? — Denise Skidmore, Mar 07 '17 at 16:42

score 2 · Accepted Answer · answered Feb 22 '16 at 17:55

Only the author of the MSDN article can provide you a definitive answer as to the wording of the article.

However: it seems to me that the primary reason for the advice is that code almost always uses the compiler-provided event accessor methods. These have always been intended to be 100% thread safe, and with a recent change in the compiler (I think as of C# 4, but I don't recall for sure), they actually are.

The reasons for making the default implementation thread-safe is, I think, self-explanatory: doing so involves reasonably low cost, and the need for thread-safety in event accessor methods is frequent enough that forcing developers to implement their own accessors every time they need thread safety would be unreasonable.

So, given that the default implementation is thread-safe, this means that consumers of events (who often will not have ready access to the source code of the event) are going to be in the habit of assuming that event accessors are always thread-safe. Violating this assumption can lead to code with bugs in it.

Bottom line: if you are 100% sure your event will only ever be accessed in a single thread, or at least in a thread-safe way, you can get away without adding explicit thread-safety to the accessor methods. The problem is that arriving at this 100% certainty is of dubious validity; it's nearly impossible to predict how a particular piece of code will be used, especially the farther into the future one is talking about.

Code can live for a surprisingly long time. Best to make sure it can handle what's thrown at it, and especially when future clients of the code have every reason to assume the code can handle it.

As an aside: while the MSDN shows locking on the event field itself, this seems problematic to me. The moment the field has been updated, any currently held lock is going to not prevent subsequently executing code from entering the lock, even if the lock itself hasn't been exited. There could be field visibility issues on some platforms, due to misordering of reads and writes to the field; this could lead to two subsequently executing threads seeing different values for the lock, and then entering the protected section concurrently.

Never mind the more general problem of using publicly available values for locking. There is some debate on that particular topic, but it is my preference to only ever use private values for locking. I.e. don't lock using the event field's current value (because it's changeable) and don't lock using this (because it's public).

Multi-threaded concerns when coding event accessors

1 Answers1