1

The code sample "How to: Define Event Accessor Methods" at

http://msdn.microsoft.com/en-us/library/dw1dtw0d.aspx

appears to mutate the internal pE without taking locks. (It doesn't look like Delegate::Combine does anything magical that would prevent issues.) It also does

void raise() {
   if (pE != nullptr)
      pE->Invoke();
}

which can be problematic if pE changes to null between the check and the Invoke(). I have two questions:

  1. Am I right in that the existing code is not thread-safe?

  2. Since I want a thread-safe version of the code, I was thinking of locking the add and remove functions. Is it premature optimization to use

    void raise() {
    MyDel^ handler = pE;
    if (handler != nullptr)
       handler->Invoke();
}

    or should I just lock that function too?

seeker
  • 1,136
  • 1
  • 13
  • 16
  • possible duplicate of [CA1047 'Make member raise private, public, or internal' and C++/CLI events](http://stackoverflow.com/questions/4336790/ca1047-make-member-raise-private-public-or-internal-and-c-cli-events) – Hans Passant Jul 20 '12 at 22:02

1 Answers1

3

All three accessors are thread-safe by default (raise includes a null-check, and uses a local variable to avoid the race condition) unlike the example in the page you linked.

When it comes to custom event implementations, you're right about needing to synchronize the add and remove accessors. Just put a mutex around the implementation. But there's no need to throw away type safety by calling Delegate::Combine and then casting, since operator + and - are overloaded for delegate handles. Or you can go lockless, as follows:

void add(MyDel^ p)
{
     MyDel^ old;
     MyDel^ new;
     do {
         old = pE;
         new = pE + p;
     } while (old != Interlocked::CompareExchange(pE, new, old));
 }

Define remove mutatis mutandis (new = pE - p;). And the code you gave for raise will be perfectly fine for a custom event implementation.

In summary, that MSDN sample is total garbage. And the simplest way to achieve thread-safety is with an auto-implemented event.

Community
  • 1
  • 1
Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Thanks :-) Just out of curiosity, is there a way for a listener to ensure that its attempt to `remove()` itself has been successful? – seeker Jul 22 '12 at 00:44
  • @seeker: The `do`-`while` loop won't exit until it is successfully removed. Unless you mean whether the listener was even on the subscriber list in the first place, in which case no the event accessors don't provide any way for you to return that information. But you certainly could make a separate method that checks whether `new.GetInvocationList()->Count != old.GetInvocationList()->Count` – Ben Voigt Jul 22 '12 at 03:02
  • Yeah, I meant the latter. I was wondering if there might be a problem with `raise()` picking up a really old value of `p`, but I guess the caller to `remove()` has to assume that it can receive an indefinite number of events after `remove()` returns. Still, it seems like we need to mark `p` as volatile for complete correctness. – seeker Jul 22 '12 at 20:29