Safely firing an event, Part 3


Take another look at Safely firing an event.  According to Grant, there’s another issue here.  The JITter may optimize away locals in situations where you think want them for thread safety.  So the recommendation to make a local copy is not as valuable as we had hoped.

 

Grant’s recommendation is to mark the event volatile:

 

    public volatile event D SomeEvent;

 

    void FireSomeEvent()

    {

        D temp = SomeEvent;

        if (temp != null)

        {

            temp();

        }

    }

 

This problem just keeps getting worse & worse!

 

My conclusion: Use the empty delegate pattern, that I described in Safely firing an event, Part 2:

 

    public event D SomeEvent = delegate { };

 

    void FireSomeEvent()

    {

        SomeEvent();

    }

 

It’s simpler, cleaner, less headache.

 

If I was going to design the language over from scratch, I would change the semantics of delegate invocation to say that if it’s empty it’s just a nop, instead of a crash.

 

 

Comments (15)

  1. Thomas Eyde says:

    Why can’t you just change it? What would there be to break? I can only see that a change would render all those null checks redundant. Now you even have a new runtime, so the change should be even smoother.

    What does the event look like behind the scenes, anyway?

    I think it’s amazing that this design flaw ever happen. The C# team brags about how much they think of everything and so on. C# is also about being OO, isn’t it? The event mechanism doesn’t feel very OO. The fact that the object owns the event, but still has to check on it wether the listener are still there? Didn’t nobody’s stomach react to that?

    I wonder what’s wrong whith how Java implements events, since you (as in they) chose to do it different.

  2. Matthew W. Jackson says:

    The reason for the design is because not all delegates have a void return type. Some may even have out parameters.

    What would the result be of invoking a null delegate that returned a value?

    However, one solution that I’ve thought of is to use an EventCollection class (similar to the one provided by Component) that provides a dictionary lookup for events. If you use the new EventHandler<T> for ALL of your events, the EventCollection could have a generic method:

    Invoke<T>(object key, T e) where T : EventArgs

    This method could check that the event is in the dictionary and invoke it. Thread-synchronization or marshalling could be built in to this collection.

    I’m not even sure that EventHandler<T> would need to be used, but that certainly is a clean way to define your events (assuming you’re not worried about CLS-compliance).

  3. Thomas Eyde says:

    I think your point is irrelevant. The event mechanism is based on delegates, but it didn’t have to. When it does, it could have the restriction on what types of delegates it accepts, like all delegates should be void. That is also recommended, because return values are hard to handle in events.

    I also did not propose a null delegate. What I propose, or want, is that when I invoke an event, it should then invoke all its listeners. It may happen that all listeners equals no listeners, but that is really not of my concern.

    When I don’t know who my listeners are, why should I care when there are none? Isn’t that the whole idea with events? That any object can subscribe to my event without me knowing who they are.

    The event collection is how the event mechanism should be in the first place.

    I also have a great dislike that I can do a += to something which equals to null, and that when I do -= to something, it suddenly becomes null.

    No other object in the C# space behaves like this. In fact, I think it’s impossible to create such a beast outside of the compiler.

    += should map to AddListener(), -= should map to RemoveListener(). The whatever declared with the event keyword should be the event collection and should never be null.

  4. David Levine says:

    Jay,

    Even if the compiler did not optimize away locals there may still be a problem on machines with a weak memory model. I believe the keyword volatile provides a memory barrier so that helps but it does not prevent a race condition.

    Unless I am missing something, AFAIK the only way to provide thread-safe code is to use synchronization between the threads. You can take control over adding, removing, and invoking the listener list and use locks to ensure the list is stable. This also provides the memory barriers. For example…

    private delegate void SomeSignature(string foo);

    private event SomeSignature _event = null;

    private object _myLock = new object();

    public event SomeSignature SomeSignatureEvent

    {

    add

    {

    lock (_myLock)

    {

    _event += value;

    }

    }

    remove

    {

    lock (_myLock)

    {

    _event -= value;

    }

    }

    }

    private void FireSomeSignatureEvent(string msg)

    {

    lock (_myLock)

    {

    if (_event != null)

    _event(msg);

    }

    }

  5. Hasani Blackwell says:

    When thread saftey is of concern, in some scenarios, I just make a private static method and in a the static constructor of the class that declares the event, wire it to the private static method, eliminating the need for (does event == null) checks.

  6. jaybaz [MS] says:

    Folks: You’ve got some pretty strong feedback on this topic. I’ve sent it to the appropriate folks for them to ponder. Hopefully they’ll think up something great.

  7. Matthew W. Jackson says:

    Well, I do agree that perhaps delegates were not the best thing to use for events.

    Or, perhaps, the decision to remove Unicast delegates was a bit short-sighted (not to mention it’s odd that the class structure still suggests that there are two kinds of delegates). Here’s my idea for a design:

    For delegates that act as function-pointers (things like comparisons, starting threads, signal callbacks, etc.), a Unicast delegate that can point to zero or one method would be nice. Having more than one function in the invocation list isn’t really what is needed in these cases. In a unicast delegate, a check for null should always be necessary if a return value is involved, and the null-reference exception would be be thrown as currently is the case.

    Multicast delegates, which events would still be based on, would not allow any out parameters or return values, since they really don’t make sense. Since there would be no return values, a multi-cast delegate that points to no functions could be invoked with no problem.

    To me, I don’t agree with the decision to drop unicast delegates. The argument was that having two types of delegates could lead to confusion. Maybe unicast delegates could have been called delegates and multicast delegates could have been called events. The keywords of the languages could have been designed to reflect this.

    To me, it always seems odd that a delegate such as a thread-start method or a comparison can point to two methods.

  8. Matthew W. Jackson says:

    Oh, by the way…

    I’ve been playing around with some code, and at first I thought that some of these problems could possibly be solved very elegantly if we could write generic wrapper classes with a delegate as a type parameter.

    I was thinking about writing a ThreadSafeDelegate<T> where T : Delegate, but the Delegate constraint cannot be used at all. I started coding away without this constraint, performing runtime checks on the type of T, but I still couldn’t get an elegant solution.

    I also tried this technique to make WeakDelegate<T> where T : Delegate, but I had those same problems. One of the problems is invocation.

    However, if you follow the event model, and don’t care about delegates in general, this can be very easily done with a TreadSafeEvent<T> where T : EventArgs and WeakEvent<T> where T : EventArgs that simply raise EventHandler<T> events.

    I will see about implementing weak events and thread-safe events into my EventCollection class. I’m excited about the possibilities here.

  9. jaybaz [MS] says:

    Matthew,

    That sounds pretty cool. I’m looking forward to seeing the result.

    A couple times I’ve run in to situations where I wanted to constrain to delegates, but it can’t be done. I made a suggestion to the langauge team to add support constraining to:

    – ‘delegate’ – Delegate types, but not System.Delegate

    – [] – array types, but not System.Array

    – enum – enum types, but not System.Enum

    (Imagine these being similar to ‘class’ and ‘struct’ constraints.)

    The response was that they weren’t really useful. These didn’t enable anything you couldn’t do otherwise.

    For example, even if you constrain to a delegate, you can’t safely invoke the delegate, because you can’t supply a method with the correct signature. (You could provide Invoke(params[] object args), but it’s not typesafe.)

    If you can find a situation in which extending the constraint system actually was useful, let me know & I’ll forward it to the language designers.

  10. Matthew W. Jackson says:

    I think that the "enum" constraint could be very important and useful, but only if they add generic type-safe versions of the static methods on Enum, such as Parse. I believe that this has been suggested, and if it ever gets implemented an enum constraint may be necessary. I don’t quite understand why System.Enum is not allowed as a constraint, except the possible question of what happens when you actually use System.Enum itself and not a derived type as a parameter.

    Plus, using an Enum constraint should theoretically let you use all of the bitwise operators, since they’re available for all the underlying types of enums.

    The only place I’ve thought of where this would be useful is in writing a helper class "Flags<T> where T : enum" that allowed you to get/set/toggle bits by an indexer..But after thinking about it the great performance benefits of bit-set enums would be completely destroyed by the increased working-set size of a program using such a generic type.

    As for arrays, I’m failing to see how T : [] would be better than just using T with a T[] member/parameter, but I think I’m probably missing something.

    Delegates would be really nice, too, but like you said, I’ve found no way other than dynamic-invocation to make good use of them. If the runtime/compiler could get around this, and somehow magically let generics based on delegates insert dynamic method signatures based on the signature of the type parameter, then I could put this to use.

  11. jaybaz [MS] says:

    Matthew:

    Constraining to ‘System.Enum’ or any of the other special classes could allow you to actually create an instance of one of them (or so I’m told). Obviously that should be stopped! That’s why we would use the keyword syntax for the constratint (like ‘where T: struct’ instead of ‘where T: System.ValueType’).

    When thinking about the performance impact of bit flags vs. arrays of bool, remember that bit-flags are essentially unaligned. Depending on specific details of the context in which your application runs, the performance comparison can change quite significantly. (I’ve always wanted to compare booleans in 1-, 2-, and 4-byte storage on x86 to see if there are situations where the larger storage could be faster.)

    This just reinforces the principle of coding for clarity, and taking real perforamnce measurements before compromising clarity for performance.

  12. Matthew W. Jackson says:

    I really doin’t see how you could create an instance of System.Enum if it was a constraint, but then again I didn’t design the framework. How is this any different than constraining on an interface or an abstract class?

    Is it because ValueType and Enum are themselves reference types? Does it have something to do with serialization? Does IL enforce these rules, or is it simply a C#/VB/C++ thing?

  13. Matthew W. Jackson says:

    And yeah, as for bit flags, I was thinking about memory-usage performance over actual execution speed. Any time I use a bit-flag, it’s because I’m going to be storing a lot of these sets of booleans, and I can’t justify taking up a byte of storage for something that takes a bit. And with fast processors, small caches, and slow memory, this tends to work out in the long run. It’s just that all of the bitwise operators on the enums seem inelegant and redundant (and it smells), so I considered a generic wrapper that wrapped an enum and turned it into an IDictionary<T,bool> where T:enum.

    The reason I would consider this sort of wrapper inefficient is that T and bool are both value types, so for every type of T wrapped, native code will be JIT-ted, which would probably outweigh the memory saved by cramming a bunch of bools into a flags enum. But I can’t really test this unless I can get an enum constraint, and I can’t argue for an enum constraint until I’ve tested this.

    If there was an Enum.Parse<T> where T:enum, there would already be a good reason to have enum constraints, so I could play with other uses of enum constraints.

    By the way, this line of thinking begs the question—does each enum type result in a separate JIT-ted set of code? In other words, do List<ConsoleColor> and List<ConsoleKey> share the same code, since they’re both technically List<Int32>? How can I test this? This would make a big difference in regards to this discussion.

  14. Juval Lowy says:

    How about this solutions:

    public event D SomeEvent;

    void FireSomeEvent()

    {

    D temp = null;

    temp += SomeEvent;

    if (temp != null)

    temp();

    }

    or:

    public event D SomeEvent;

    void FireSomeEvent()

    {

    D temp = delegate{};

    temp += SomeEvent;

    if (temp.GetInvocationList().Lenght >=2)

    temp();

    }

  15. Matthew W. Jackson says:

    Good Idea Juval. If this does indeed prevent the problem (which I think it does, if I had to wager), then you can simplify this even further:

    public event D SomeEvent;

    void FireSomeEvent() {

    D handler = null + SomeEvent;

    if(handler != null) handler();

    }

    I’d try to avoid using "delgate{}" on each firing of the event…actually I’d avoid it alltogether (although it’s certainly elegant).

    The problem with throwing delegate{} all over the place is that you end up generating a TON of auto-generated methods that will have to be loaded and jitted by the runtime. I’m not sure what kind of overhead an empty static method on a class creates for the runtime, but it certainly bloats the metadata. Anonymous methods are great when they do something, but empty anonymous methods seem wasteful to me. For example, if you have two such events, you will end up with something like a method "<.ctor>b__0()" and a "<.ctor>b__1()".

    I can’t argue with the elegance of delegate{}, but I’d certainly prefer them to share an empty method somewhere.

    Hopefully, Juval’s idea will eliminate the need for anything as drastic as delegate{}. And it certainly beats "volatile", which I try to avoid at all costs since few programmers understand it (and it’s almost always unclear why a programmer is using it)