Events get a little overhaul in C# 4, Part II: Semantic Changes and +=/-=
Last time, we saw that the field-like event accessor code that the C# compiler emits has changed. And it’s better in a few ways because we’ve banished the locking code that we used to emit.
There are a few questions that came up in the comments, and two of them in particular are probably as straightforward as can be when you’re thinking about code that the compiler’s generating. First, is the code bigger? And second, is it slower? In this case, yes and no. Yes, the IL is 17 bytes bigger per accessor (not per instance), which will make your assembly that much bigger. Which is not fantastic news, but hey, 17 bytes. And no, the code is not slower. In fact, it’s faster! In tests that I’ve performed, when the delegate operations are as simple as possible the accessors appear to be about 20% to 25% faster. That’s because there’s no lock.
But neither of these issues are cause for concern or celebration. No, the big problem is that this change is an observable semantic difference between C# 3 and C# 4, and there is existing code that it could break. You see, you used to be able to protect against any change to your events over some block of code simply by putting the code in a “lock(this)”:
class OldClass
{
public event EventHandler E;
public void UseE()
{
lock (this)
{
// Safe in C# 3, unsafe in C# 4!
E = (EventHandler)Delegate.Combine(E, new EventHandler(SecretHandler));
}
}
private void SecretHandler(object sender, EventArgs e) { }
}
This code used to be effective, because inside the lock, we could be sure that no other thread would be changing the value of E between our read and our write. So this could not screw up E. Now, uh oh, it can! But just a minute, who writes code like this? Anyone who is going to call Delegate.Combine is probably going to do so with the + operator in C#:
class OldClass
{
public event EventHandler E;
public void UseE()
{
lock (this)
{
// Still safe in C# 3, unsafe in C# 4!
E = E + SecretHandler;
}
}
private void SecretHandler(object sender, EventArgs e) { }
}
That calls Delegate.Combine in the same exact way. And actually, I suspect that anyone who writes that code is really going to write the following:
class OldClass
{
public event EventHandler E;
public void UseE()
{
lock (this)
{
E += SecretHandler;
}
}
private void SecretHandler(object sender, EventArgs e) { }
}
That code also calls Delegate.Combine, and we broke it too! This might be surprising to you. Look at it, the thing on the left of the += is an event, so surely this calls the event accessor, right? Nope! Let me take a quick detour here and explain to you how the binding of += works in C#. There are two possibilities: (1) either there is an actual + operator, such as with ints, and x += y binds to “x = x + y” except that x is only evaluated once. This is the compound assignment operator; or (2) the thing on the left hand side is an event, and x.E += y binds to “x.add_E(y)”. This is the event accessor operator, and in fact this is the only way to bind to an event accessor. So what do we have in the snippet above? Well, the extra bit of detail that you need to decide is the following rule about field-like events in C#: outside of the class or struct that defines a field-like event E, binding to the name E resolves to the event itself, on which the only legal operation is calling an accessor; inside the class or struct that defines a field-like event E, binding to the name E resolves to the private delegate field.
So in that last bit of code there, in “E += SecretHandler,” E is actually the delegate E, which has a +, not the event! So the binding you get is to Delegate.Combine (via +), not to the add accessor. And therefore, the explicit lock is the only thing that makes this safe in C# 3. Oh my! What have we done, and how can we fix it?
Well, it turns out that the binding in that last snippet of code is very surprising to most C# developers who have encountered it. It’s so surprising that I would say that in 99% of the cases, the developer thinks they are calling the accessor and so they don’t even put the lock in:
class OldClass
{
public event EventHandler E;
public void UseE()
{
// Unsafe in C# 3!
E += SecretHandler;
}
private void SecretHandler(object sender, EventArgs e) { }
}
But you cannot call the event accessor from inside the class that defines it. There’s a simple fix, though. Now we let you call the add accessor this way!
I hope I haven’t lost you, since this is actually quite subtle and you probably were unaware of the binding rule I outlined above. But that’s the rule that we had in C# 3. In C# 4, however, the new rule is that inside your class with field-like event E, the name E binds to the backing delegate field (so you can call it, for instance), except when it’s the left hand side of a += or –=, in which case it becomes the event.
This change fixes the code in C# 3, which you might not have even known was broken. Heck, even if you were among the small fraction of people who knew that events took care of some synchronization for you, you still probably didn’t know this was broken. I know this because there is a constant slow trickle of people who email us having discovered this the hard way. And we now we fixed them.
But we also fixed, to some degree, the problem with the new C# 4. If you wrote “E += Handler” in your class, now you get the benefit of our new synchronization whether or not you put the “lock(this)” there. Which is great, because you don’t want to write that synchronization yourself.
Ok, ok, there’s more. But I’ll leave it at that until Part III.
Comments
- Anonymous
March 08, 2010
What is the reasoning for the different behavior from outside vs. inside the class? Maybe I'm not quite awake yet, but I can't quite see what this would be useful for. It's not intuitively obvious that is the behavior and would only lead to subtle bugs (which you've alluded to in your article). Anyone who really needed the delegate directly could do so by making a backing field and using the add and remove accessors of the event, which is what I've done on occasion because I didn't realize it was treated differently from inside the class - in hindsight with this article in mind, I'm glad I did it this way.Regarding the change - do you know if there will be an FxCop rule added to check it? I'm working on a very large code base so it would be hard to track down manually and this change could lead to some very subtle bugs. - Anonymous
March 09, 2010
You might have checked to see if an event was null before you called it..? You can't do that from outside the class, as an event only has add and remove, no direct read or write access to the actual delegate holding the callback. - Anonymous
March 09, 2010
What is the reasoning for the different behavior from outside vs. inside the class? Maybe I'm not quite awake yet, but I can't quite see what this would be useful for. It's not intuitively obvious that is the behavior and would only lead to subtle bugs (which you've alluded to in your article). Anyone who really needed the delegate directly could do so by making a backing field and using the add and remove accessors of the event, which is what I've done on occasion because I didn't realize it was treated differently from inside the class - in hindsight with this article in mind, I'm glad I did it this way.If events weren't treated as fields from within the class, you could never raise an event in the first place, since event accessors are only add (+=) and remove (-=). The reason why you can raise an event by writing "Click(e, args)" from within the class is precisely because "Click" is treated as a field of a delegate type, which then has operator().Same reasoning also applies for comparisons such as "Click == null" etc. - Anonymous
March 09, 2010
If events weren't treated as fields from within the class, you could never raise an event in the first place, since event accessors are only add (+=) and remove (-=). The reason why you can raise an event by writing "Click(e, args)" from within the class is precisely because "Click" is treated as a field of a delegate type, which then has operator().Do'oh! Thanks for waking me up! Not quite sure how I missed that ... - Anonymous
March 09, 2010
Pavel, there actually is a "raise" accessor in IL and C++/CLI, but for some reason it isn't exposed in C#. - Anonymous
March 13, 2010
Now if only you guys would break the strong reference issue with events so they didn't create all sorts of memory leaks, forcing developers to manage the lifetimes of their objects - which takes away some of the fun of a managed language where I'm not supposed to have to worry about memory... - Anonymous
March 14, 2010
I am amazed how much energy you guys spend to fix details. Congratulations. But I would prefer to get weakly referenced events instead. I am sure that would fix many more problems of real existing code (memory leaks!) then the presently discussed, which most people even know about.Or how about if an event could be called even if there is not event handler attached to it ? Now everyone has to write countless lines of code like:if (myEvent!=null){ myEvent();}It is not intuitive that I can't just call myEvent() and even less intuitive that if a class subscribes to an event, that this will prevent the class from being memory collected. I am sure there are thousands of developers who get this one wrong and don't know it. It happens even to me sometimes.The idea of memory collection is that I don't need to do the house keeping of destroying objects. But as soon I use an event, I am forced to do that house keeping. - Anonymous
March 14, 2010
@Jason: I'm sorry, but you're never supposed NOT to worry about the memory.The garbage collector HELPS you cleaning up your memory, but it's not the holy grail. You should always detach your event handlers before disposing of an object!The GC only cleans up objects that have zero pointers to them. And as long as the event has a pointer to the object, it's stuck. - Anonymous
March 14, 2010
@Jason & KristofHave you guys not heard of anonymous delegates? This is exactly what they're for. They create weak references that are then cleaned up without the need for explicitly removing them. - Anonymous
March 15, 2010
@Steve: Won't the anonymous delegate still keep alive every object that it refers to? For example:obj.Event += delegate { handler.HandleObjEvent(); };This will still keep a strong reference to the object "handler", right? Just as if we had written:obj.Event += handler.HandleObjEvent;In fact, as I understand it, the anonymous delegate is worse because you can't remove it later. Am I missing something? - Anonymous
March 15, 2010
Steve, A quick google shows that unless I'm missing something, or it's changed in later versions, anonymous delegates hold strong references unless you go to the work of explicitly holding weak references. I hope I'm wrong, but that's what I saw, although I haven't verfied it. Here's one of several articles about this.http://stackoverflow.com/questions/371109/garbage-collection-when-using-anonymous-delegates-for-event-handling. - Anonymous
March 15, 2010
I think the obvious lesson here is don't use .net events. When the Observer pattern is present in ap roblem, it is cheaper, easier, and obviously safer, to implement the required behavior yourself. You can test-drive everything and /know/ it works as it should rather than just /hope/ it will continue to do so going forward. - Anonymous
March 15, 2010
@Max: With very similar arguments you can justify to not use almost any abstraction, be it events, anonymous methods, the yield statement, the garbage collector or even the C# compiler (you could write IL or even machine code). All of these abstractions have their corner cases.The important message here is that MS did recognize that one such corner case was a problem for a sufficiently large minority of the developers and reduced the probability for most devs to ever run into this one. - Anonymous
March 16, 2010
At first when I saw the title of this blog my first response was 'oh oh'. But I'm glad to see that this would never come into play for me. The bigger concern is the leaks cause by unsubscribed event handlers. I don't use dynamic so I'm able to put unsubscribe logic in my Dispose code quite easily.However not only do dynamic delegates create an issue in always having a handle, but the Silverlight asynchronous call handlers as well. I tried...and there is no way to clean up the binding of the event handler to the event. So either my client suffers .... or worse yet ... my WCF server begins to suffer.And...boy to I hate the "do the old Microsoft fix ... reboot" comments. Yet that is the only fix! - Anonymous
March 16, 2010
Is using lock(this) really a safe way to have thread-safe events in C# 3.0? I've been using a lock object and a separate private handler. - Anonymous
March 16, 2010
@Chris: No, lock(this) was a bad idea: see the previous blog post in this series. Your solution is better. (The C# 4.0 solution is perhaps even better since there's no lock at all.) - Anonymous
March 16, 2010
thanks for this. can implement this on my project. - Anonymous
March 16, 2010
@RalphI definitely read some documentation about the implementation of weak references for the lambda expression syntax on events.so: myClass.MyEvent += (s, e) => { };generates a weak reference and so myClass can be collected and finalized.I've just done a test and this bears out. I applied the syntax above and saw the finalizers called. I did the same using the traditional syntax (i.e. not using anonymous delegates) and the finalizers were not called, indicating they were not correctly collected as expected. - Anonymous
March 17, 2010
@Steve AdeyYour test appears to be constructed backwards. Although myClass holds a reference to an event handler (whether weak or strong) this reference does not affect the lifetime of myClass itself.On the other hand, so long as myClass is kept alive by some reference, then any objects referenced by handlers for MyEvent will be kept alive (until those handlers are removed). I believe this includes anonymous delegates. - Anonymous
March 17, 2010
@Jeffrey,I can clearly see the difference between anonymous delegates and otherwise, as the myClass finalizer is only called when the anonymous delegate is used.Any design keeping references to objects inside an event handler, IMHO, has to be questioned. - Anonymous
March 17, 2010
The comment has been removed - Anonymous
March 17, 2010
I am actually against weakly referenced events. I mean, if they were weakly referenced from the beginning, this might have been a good idea. But if you make the change now, it would break lots of my code. - Anonymous
March 18, 2010
Steve, anonymous delegates will never cause weak references to show up. If the class that defines them is collected, it's because there was no reference at all. This is the case if you never close over (that is, use) a local or the instance itself, such as in your example. In other words, anonymous delegates are emitted essentially as static methods whenever possible and the delegate created from the static method has no reference to the instance that created it.Ivan, good news, we're not changing this.Michael, I'm not sure what problem you're referring to. If you have a delegate, you can always store it in such a way to remove it from the event handler. Now, I am sure that some APIs make it difficult or impossible to know when to do this remove; is that your problem? If it is, you could insert your own weak reference wrapper on the delegate; maybe I'll blog about that some time.chris - Anonymous
March 18, 2010
The comment has been removed - Anonymous
March 18, 2010
Sorry, I'm assuming by 'local' you mean a locally defined variable as opposed to a member variable, which would cause a problem. - Anonymous
March 18, 2010
I may have got my wires crossed with the following article regarding events and weak references:http://msdn.microsoft.com/en-us/library/dd458915.aspxWe are using the Prism framework, so I may rework our project to use the EventAggregator, as all the heavy work has been done for us.This may be what other people are looking for here instead of having to roll their own weak reference events. - Anonymous
March 18, 2010
@ChrisI think it's good that you removed the lock(this) on event accessors. This bothered me ever since I read Jeffrey Richter's Applied .NET (now CLR via C#) but I get the impression that you're not taking the strong reference problem seriously. Yes we can insert some form of weak reference wrapper but then we could also write our own event accessors to work around lock(this). Although I was aware of the lock(this) problem with the default event accessors, in all my years I've never seen it actually cause a problem. On the other hand, I've seen the strong reference mechanism result in unexpected memory leaks all too often. Now if you can't change to a weak reference mechanism as the default mechanism due to breaking changes then fair enough but I think the language needs to offer much better support in this area, such as supporting an opt-in approach for a new model. - Anonymous
March 18, 2010
There's a good WeakReference pattern via System.Windows.WeakEventManager. Unfortunately, you need to inherit WeakEventManager for each event you need.Still; it is a pattern and not alot of code. I tried coming up with a generic Manager but couldn't get it to work.All my apps only have two events StateChangeCompleting and StateChangeCompleting where the object graph notifies all listeners.It would be grand if the compiler good implement the weak reference patten for us. - Anonymous
March 19, 2010
@Kevin,I completely agree, the standard should be weak references (as with the CompositeWPFEvent) and then opt in for strong references (which I understand are more performant). Can't see a reason why it should be more complicated than this. - Anonymous
March 19, 2010
@DuncanYou're correct that patterns exist but all of them I've seen are either non-genric or non-trivial. E.g. the WeakEventManager approach you mentioned is a WPF pattern. In addition to writing a manager for each event as you mentioned, it requires you to take dependencies on WPF assemblies and it requires clients to add handlers in a non-standard way (by implementing an interface). There will be cases where this is simply not an option. This comes back to my main reason for responding to the blog. Although the lock(this) on event accessors was a problem I've not seen any evidence to suggest it was anything other than rarely encountered and trivial workarounds have always existed. Memory leaks due to the strong reference mechanism for events are, in my experience, more often encountered and the workarounds are either non-generic or non-trivial. I've long wished the C# team would address this or at least offer an explanation about why it's not possible for them to do anything in this area. - Anonymous
March 19, 2010
A pattern that addresses management of attached events: attach events in property accessors; and remove them when null is assigned. To release all events attached to an object, assign null to it's associated property. It works well because it's unlikely that you're going to throw an exception in a set accessor; and assigning nulls to accessors when releasing events is robust in the face of objects that are in a peculiar state due to an error.e.g.:EventTargetClass eventTarget;EventTargetClass EventTarget { get { return eventTarget; } set { if (eventTarget != null) { ... detach all events ... } eventTarget = value; if (eventTarget != null) { .... attach all events ... } } - Anonymous
March 19, 2010
The disadvantage to relying on weak references, of course: failure to remove events on a no-longer-needed object is almost definitely a programming error; so you SHOULD have removed your events. - Anonymous
March 20, 2010
What about the if (Event != null) checks that we all have to worry about? Isn't a lack of manual locking going to cause problems if delegates are removed from the event between the check and the final event call? Can the null check just be folded into the event operator() call? This would certainly solve an ongoing concern of mine... - Anonymous
March 21, 2010
@edrowland: With your reasoning, failure to free a no-longer-needed object would also be a programming error, and we would not have garbage collection. - Anonymous
March 22, 2010
@Chris:There is a nice extension method workaround for dealing with EventHandler<T>public static class EventExtensions { public static void Raise<T>(this EventHandler<T> handler, object sender, T eventArgs) where T : EventArgs { if (handler != null) { handler(sender, eventArgs); } } } - Anonymous
March 23, 2010
I do it in the following manner for event handlers which are no longer needed after being fired. EventHandler saveResult = null; // saving saveResult = (s, ea) => { // unsubscribe immediately (s as EntityViewModelBase).SaveEvent -= saveResult; // some code to save... }; entityViewModel.SaveEvent += saveResult; - Anonymous
March 26, 2010
The comment has been removed - Anonymous
April 07, 2010
The comment has been removed - Anonymous
May 04, 2010
I also encountered problems with events holding references to objects that prevent both the referenced object (and the object of which the event is a member) from being GC'd. Here is what I have come up with.An "event" is, essentially, a delegate. The Delegate class has a "RemoveAll" method (http://msdn.microsoft.com/en-us/library/system.delegate.removeall.aspx).In a finalizer or in a dispose method, or in a "close" handler of a window, etc., I have used the RemoveAll method.It is a static method, so provide the event member (i.e., the "delegete" to which handlers subscribe) as both the source and destination and the method removes all contained references from the event. Here's a bit of pseudo code if this is not clear:class MyClass{event MyDelegate myEvent;// subscriptionmyEvent += MyHandler-Subscriber;finalizer/close/Dispose, etc...Delegate.RemoveAll(myEvent, myEvent);Volia! ;) All contained references gone!Granted, this is not automatic and I would much prefer that with a weak reference to this method which is, IMHO, a work-around at best. Until we get better code from the great Microsoft, this is the best method that I have so far seen or come up with.