Unsubscribing from C# events when GameObject is destroyed

I can use the += operator to subscribe a C# Action (listener) to an event (source).
Later, I can use the -= operator to unsubscribe the listener from the source.

So here's a scenario:
1. I attach a listener A to a source B using the += operator.
2. Listener A is destroyed
3. Source B broadcasts an event again.

In this scenario, A still receives the event even though it doesn't exist anymore, which can cause problems. If I manually unsubscribe (-=) before A is destroyed, it fixes the error, but I'd like an automagic way of doing this every time a GameObject is destroyed. Then it would be one less thing I have to worry about in my code.

Anyone know of a way of doing this?

 

Use method OnDestroy

 

Yeah, OnDestroy will probably need to be used at some point, but I don't think you're seeing what I'm asking.

I can manually detach all my listeners in OnDestroy, but I'd like to avoid this step and have it work automatically. Moreover, I like to use anonymous functions a lot, and those are hard to manually detach.

 

You're going to have to do it manually...

 

Not true. I already have a solution to this problem, but its implementation is quite dirty and spans multiple files. Moreover, it requires that actions only have one parameter, but that's probably fixable with an even dirtier solution.

I'm posting here to see if there is a more concise, common solution that I'm just missing.

 

I have a solution as well that uses WeakReferences to handle it, but it's not super clean... there isn't a good way to do it cleanly that's why you need to do it manually.

 

Hmmm, Weak Refs. It seems like you could just remove the reference when the GameObject is destroyed?

My solution does work reliably, and I'm using it to register and auto-detach over 100 events without problems. But it's dirty (to implement, not to use), and wouldn't work with another's codebase easily without some modifications. Also, as I said, it only allows one parameter. And it requires OnDestroy to be defined in a base class, which means subclasses have to override a separate destroy method.

Perhaps there just is no clean solution, and that's the end of it. I think I heard Unity is reworking their event system in an upcoming release, to replace their current SendMessage system. Maybe they'll have to address the same problem too.

 

So there are really a couple of things you could look at. The best solution is to manually remove in OnDestroy... or for non-behaviors, implement IDisposable and do event removal when your object is disposed / finalized.

With that not being an option, you could look at creating your own event system where you really don't use events at all. Instead, you would just use multicast delegates. When subscribing to the events, you would need to store your event function in a WeakReference and keep track of it somehow. You could use a Dictionary<int, WeakReference> where the int would be the result of GetHashCode so you could easily do a lookup.

Now you'd obviously need to inherit from WeakReference and create your own method call to use as a proxy and that will be what is actually added to the multicast delegate. When the proxy method is executed, it extracts the object back from the WeakReference and invokes the matching method on that object.

Now, you also need to check to see if the object has been destroyed but that's just a property of the WeakReference class. Additionally, you'd need a way to manage and check those references. I have a similar system that is used for messaging / notification between game objects. There is a class that throws a couple empty objects onto the heap (depending on GC generation) and then waits for the GC to run. When the GC runs, it fires an event, at which point I iterate through my WeakReferences and remove any whose underlying object have been destroyed. In your case, you would just unhook the proxy method of the weakreference from the multicast delegate and then remove the weakreference from the collection pool.

That is probably the easiest way I could think to automate it... and it's still not easy or clean.

 

Here's the GCNotification class I modified from a Wintellect article. You just hook into the GCDone event to receive notifications when a collection occurs:

Code (csharp):

1.  
2. using System;
3. using System.Threading;
4.  
5.  
6. namespace parentElement.Messaging
7. {
8.     //The following class is from:
9.     //Jeff Richter - http://www.wintellect.com/CS/blogs/jeffreyr/archive/2009/12/22/receiving-notifications-garbage-collections-occur.aspx
10.     internal static class GcNotification
11.     {
12.         private static Action<Int32> _gcDone; // The event’s field
13.         internal static event Action<Int32> GcDone
14.         {
15.             add
16.             {
17.                 // If there were no registered delegates before, start reporting notifications now
18.                 if (_gcDone == null) { new GenObject(0); new GenObject(2); }
19.                 _gcDone += value;
20.             }
21.             remove
22.             {
23.                 if (_gcDone != null) _gcDone -= value;
24.             }
25.         }
26.         private sealed class GenObject
27.         {
28.             private readonly Int32 _generation;
29.             internal GenObject(Int32 generation) { _generation = generation; }
30.             ~GenObject()
31.             { // This is the Finalize method
32.                 // If this object is in the generation we want (or higher),
33.                 // notify the delegates that a GC just completed
34.                 if (GC.GetGeneration(this) >= _generation)
35.                 {
36.                     //Thread safe get of the _gcDone delegate - will not be interrupted
37.                     var temp = Interlocked.CompareExchange(ref _gcDone, null, null);
38.                     //Fire the event
39.                     if (temp != null) temp(_generation);
40.                 }
41.                 // Keep reporting notifications if there is at least one delegate
42.                 // registered, the AppDomain isn't unloading, and the process
43.                 // isn’t shutting down
44.                 if ((_gcDone != null)
45.                     !AppDomain.CurrentDomain.IsFinalizingForUnload()
46.                     !Environment.HasShutdownStarted)
47.                 {
48.                     // For Gen 0, create a new object; for Gen 2, resurrect the
49.                     // object  let the GC call Finalize again the next time Gen 2 is GC'd
50.                     if (_generation == 0) new GenObject(0);
51.                     else GC.ReRegisterForFinalize(this);
52.                 }
53.                 else { /* Let the objects go away */ }
54.             }
55.         }
56.     }
57. }
58.  

 

Hmmm. I still don't understand why you'd use weak refs. If you could remove the listener from the OnDestroy method, wouldn't it just automatically be reclaimed by the GC? My solution uses OnDestroy btw, but it requires all my monobehaviors to inherit from a common base class, which defines that OnDestroy method.

 

The WeakRef + delegate solution was to solve your problem about parameters and the requirement to inherit anything.

 

But I want the detachment to occur when the GameObject is destroyed, not when the object is reclaimed by the garbage collector. The latter can occur at any arbitrary time, and a callback from the source could occur before collection occurs.

 

How about using the WeakEventManager (http://msdn.microsoft.com/en-us/library/system.windows.weakeventmanager(v=vs.110).aspx) or the WeakDelegate class (http://msdn.microsoft.com/en-us/library/cc533276.aspx) instead of just regular events? This will still break the connection from the source to the destroyed listener, but at least it will not hold a strong reference to the listener, so GC will at least happen sooner or later, and it will prevent memory leaks.

 

I've never understood the love some people seem to have for anonymous functions, because they cause issues exactly like this and make the code harder to read.

What's the benefit to them that outweighs this drawback? This isn't a rhetorical question. People seem to love the crap out of these, and I get the feeling that I'm missing something as to why.

Is there some higher level design you can apply across the board to get the same or a similar benefit without the drawback?

I personally just don't use anonymous methods. It's not that I consciously avoid them, but that I've never been in a design situation where they have been the optimum solution. I see plenty of downsides, but I've never been in a situation where there's an up side to them. Even the MSDN documentation seems to imply that the only benefit is the code being shorter, which I don't consider to be a benefit in and of itself since I prefer code that's a little more verbose and a lot easier to read.

 

It would probably still have the problem mentioned above. In most normal circumstances, a listener becomes invalid when it has been destroyed. This is true for most monobehaviors anyway. If you get a callback on a behavior after OnDestroy has been called, how do you know whether you should act upon it or not?

 

The code being shorter is the only benefit I know of. There isn't much of a difference in the underlying design. It's just mostly personal preference.

For instance, it's easier for me to read this:

Code (csharp):

1.  
2.     Timer(1, () => print("hello"));
3.  

Than it is this:

Code (csharp):

1.  
2.     private void MyMethod() {
3.         print("hello");
4.     }
5.  
6.     Timer(1, MyMethod);
7.  

... but if you like the second one better that's fine too.

 

So in your OnDestroy method you set a value that tells your script not to execute anything and if you get the callback, just check to see if it's been destroyed.

I'm with angrypenguin... anonymous functions are rarely that useful and it sounds like you're using them all over the place. This makes your code harder for someone else to understand, go through and debug. Your code is not self-documenting and becomes more obscure when you're using anonymous functions. The only time I've ever found them useful is in certain lambda situations (when needing to take a func as a parameter) or when using something like the task parallels library to do parallel loop iteration.

Just do it the best practices way instead of being lazy and unhook your events.

 

Best practices in who's book? In most cases, when you attach a method, it's named something like OnXXX. That name can be skipped. It's extraneous.

How about this, whenever you print a string, assign it to a variable first. Then it will be self documenting!

Code (csharp):

1.  
2.     var mySentence = "hello world";
3.     print(mySentence);
4.  

Since when is avoiding redundancy equivalent to being lazy?

 

Well, if that's really causing errors maybe you should redesign the logic? After calling Destroy the game object will still be alive until the end of the frame and the actual destruction will happen at the end of the frame. GC however may still happen even later, but if the object is still registered as an event listener that will never happen! The GC cannot release that memory if it's still referenced by a strong reference.

Anyway, here's an idea for a solution that may work in your case: Instead of destroying the objects using Destroy method make a DestroyAndDisconnect method that automatically goes through all the event sources and removes that object from the list. You'll have to also check the object type and if it is a GameObject do the same for all children and contained MonoBehaviors.

I'm not sure how to do that with events, but it should be easy with delegates: delegate fields have the GetInvocationList method that returns a Delegate[]. Then the Delegate class has a Target property which is a reference to the listener object... You get the idea?

 

+1

And maybe "lazy" isn't a word I should have used... but you've been given several possible solutions, all of which you discredit because they will still require some additional manual work. Nothing wrong with avoiding redundancy, but you're looking for a magic bullet that doesn't exist.

 

Yeah, this is more-or-less what my (rather dirty) solution does (read above), though it overrides OnDestroy, and not Destroy.

Dustin suggested using weak refs, so that's how we got on that tangent.

 

I rejected the weak ref solution because it simply doesn't work. Sorry, I don't want to be a downer or anything. I rejected the manual solution because I believe there is a better way of doing it (that's why I started this thread).

As for there being a simple way of doing this, maybe there is, maybe there isn't. I don't know ...

 

Lets ignore preferences.

1. Fact: If used properly, that name helps your code to be self-documenting. In your example you call it "MyMethod" which is patently useless. If it was called "PrintHello" it would make your code far clearer. Now imagine that it does something complex and think about whether it's easier to read a block of code to see what it does or see a name that tells you what the underlying code does.

2. Fact: The act of splitting the code out into its own method makes your software easier to maintain. Things which start out as eminently obvious one liners often do not remain that way!

3. Observation: The big one - you're thinking about what your'e doing purely from a "the code I am writing this second" perspective. You're not thinking about it from a higher level design perspective. From that higher level perspective, weigh up the benefits of "writing a bit less code" and (ignoring 1 and 2 above) consider whether that is worth the disadvantage you're giving yourself by not having explicit delegates to unhook.

Even if we accept that the name is indeed extraneous* that is no reason to discard the other advantages of named methods.

* Which says to me that you're not using the tools at your disposal to their best potential. If something is useless, is there a way you can change how you use it to make it useful?

 

"More code" is not the same as either "self documenting code" or "better code". You're introducing redundancy with your string example while neither adding useful information nor making already present information more easily digestible.

The important question here is "Will this increase or decrease the work I need to do to understand this code later?" Contrived examples of cases where specific things don't help won't aid you in making easier to read/write/understand/maintain/debug code.

 

Not always fact. For instance, let's pick a realistic case of binding to an event:

Code (csharp):

1. AttachEvent(window.CloseEvent, () => print("window is closing ..."));

I could factor out the print statement into an OnClose method, but what extra information would this give me, or someone else reading my code?

Not always fact. If the anonymous method goes over a certain limit, say, around five lines, I'll typically factor that out into a normal method. But for one or a few lines, I see no harm in them. They don't take up much space, and they don't seem hard for me to read or anything.

The anonymous function thing is largely just syntactic sugar. You like your way, I like mine. Whatever. The event detachment I see as a clear underlying redundancy. In 99+% of cases, I want auto-detachment to automatically happen. Why should I have to manually specify it every time like this:

Code (csharp):

1.  
2. private void OnDestroy() {
3.     window.CloseEvent -= OnClose;
4. }
5.  

 

Your examples are really contrived there, and ignore a lot of stuff I said (admittedly in an edit, if you didn't see it I strongly suggest giving it a read).

Also, your naming quite frankly sucks. You're naming things based on when they're called rather than what they do. I know that "OnEventName" is a common naming convention, but in this case it doesn't help, it hinders. See the "important question" I mentioned earlier.

Finally, call it syntactic sugar of you want, but keep in mind that you started this thread because you had a problem. There are plenty of solutions. None of them are magic bolt-ons. And that "syntactic sugar" that you refuse to change based on preference seems from the outside to be a big part of what's causing the problem.

 

Do you find the example with the close event contrived? If so why?

"OnXXX" is a sucky name for an event handler? Give me some of that crack you're smoking. It's a common naming convention for a reason.

Plenty? I've only seen two here:
1. Manual removal (already known)
2. Some sort of override of Destroy or OnDestroy, as I've already done and Flipbookee suggested. It works, but it's pretty dirty. That's why I created this thread to see if anyone had a simpler solution.

 

You don't seem to actually want to be helped. At least, not short of some magic extra code you can write.

Your example is contrived for starters because you call it "realistic" but then go on to do nothing but print a line. Then you say that it's more obfuscated to put the print in its own function which you give a name that doesn't tell you what it's doing. No S***! The thing is, if it were actually "realistic" then you'd be doing something in your event code.

Secondly, yes, OnXYZ is a common naming convention, and I use it a lot myself. The thing is, OnXYZ is the name of the event, not the thing(s) that happens when the event occurs. That distinction may be subtle, but it's very important.

If I have a function that, say, spawns a character, I will call it "SpawnCharacter" because that's what it does, I won't call it "OnLevelStart" just because that's when it happens to get called. Hoever, I may have an OnLevelStart event which calls SpawnCharacter. My "OnXYZ" event methods will typically just be a list of action methods that get called by that event.

 

I don't need to be helped, not in this thread anyway. Go back and read the thread again. I already have a solution, but it was kinda dirty to implement. But, seeing that many, many people also need to use events, there might be a more elegant way of handling it. That's why I created a thread to explore that.

I chose print because it's a built in function. Here are some pseudo functions, if it makes you happy:

Code (csharp):

1. AttachEvent(window.CloseEvent,
2.     () => {
3.         GamePreferences.UpdatePreferences(preferenceData));
4.         GameLogic.Unpause();
5.     });
6.  

This only works if your event handler does ONE THING. And even then, it might not be a good practice because it's not consistent with your other OnXXX methods. But that's debatable I guess.

 

I honestly don't understand what you're saying here. The pattern I described can clearly cover event handlers that need to have multiple things happen, since they're "a list of action methods". And I don't know how that could possibly imply a lack of consistency.

Or if you mean that a function "ScratchTheCat" should do just one thing, scratching the cat, then that's precisely correct. Functions should do what their name implies*. And, if I find myself writing functions/classes that can't be meaningfully names because they have multiple tasks then I see it as being time to refactor.

Anyway, my "elegant way of handling events" is what you'd call the "manual method". All of my events and their delegates are declared in one or more event classes - they just have lists of events**. Anything can subscribe to any event it is interested in, and is explicitly responsible for un-subscribing at the appropriate time. It is indeed "manual", but it's very clean and consistent. Setting up a new event is just declaring an appropriate delegate. Subscribing and un-subscribing is typically done in Awake() and OnDestroy() in the listener classes.

* This is partially context specific. Stuff like OnXYZ could have different expectations in different code bases. That's cool, just keep it consistent.

** That isn't to say it's the only places I use delegates etc., they're just the ones I consider to be "events" so far as the software's design is concerned.

 

Looking at my example:

Code (csharp):

1.  
2.         GamePreferences.UpdatePreferences(preferenceData));
3.         GameLogic.Unpause();
4.  

It does two things, namely updating preferences and unpausing. I guess I could call it DoClosingStuff, but I might as well call it OnClose at that point. But if my event handler did one thing, like update preferences, I could call it UpdatePreferences.

It's one of those things that can practically be worked around ok by just being methodical, but it is nevertheless redundant. I heard Unity is building a new event system to replace SendMessage, and hopefully they build in some sort of auto-unsubscription feature. It's sort of the Unity way, to just make things cleaner for the end user. You know, like C# where you can just allocate and forget ...

 

i haven't read the whole thread so maybe i repeat it. but you can iterate over the delegates of a multicast delegate and invoke each in a try block and when it throws an exception chances are the listener is destroyed so simply remove it.

Code (csharp):

1.  
2.             foreach(CallbackClass handler in eventTable[(int)eventmsg.MessageType].GetInvocationList())
3.             {
4.                 try
5.                 {
6.                     handler(eventmsg);
7.                 }
8.                 catch(Exception e)
9.                 {// something bad happened so output a message and remove the event as its probably not valid any more (fe listener object has been deleted)
10.                     Debug.Log("MessengerScript.InvokeDelegate: A Callback for " + eventmsg.MessageType + " is invalid and has been removed!");
11.                     RemoveListener(eventmsg.MessageType, callback);
12.                 }
13.             }
14.  

read more about this here.
this is a pretty automatic way and requires no manual removing of listeners. albeit i have no clue about runtime performance and other implications.

 

That won't work since delegates hold strong references to the listener instances, so such exception will never happen because the listener will never get destroyed.

 

Heh, interesting. Quite a hack. Though I think it would have the same problem that weak refs have (discussed earlier in this thread). When you destroy an object in Unity (with the Destroy method), it's not actually destroyed. Well, Unity thinks it's destroyed, as far as gameplay is concerned. But the object still exists in memory, until it can be reclaimed by the garbage collector.

 

I was wrong actually... Seems like this may work because anything derived from UnityEngine.Object has a non-managed part that resides internally in the engine. This part gets destroyed "for real" after calling Destroy or DestroyImmediate, either at the end of the frame or immediately. Any attempt to access the internal representation of the object will throw an exception even though the managed object may still be alive because of some still existing strong reference.

However, try-catch blocks and exceptions are expensive operations and should be avoided. There is an easy way to check if an object has been destroyed (that is Destroy has been called and the internal representation doesn't exist anymore, but the managed object still exists), see http://docs.unity3d.com/Documentation/ScriptReference/Object-operator_Object.html

Now with this in mind you can re-implement the invocation method to check for destroyed objects first and automatically remove them.

I wouldn't call this a hack but more a Unity friendly way of invoking event targets, or even "the only right way" to do that.

 

lol. That's very interesting. I'm surprised that works.

But I think I'd have to iterate over all my listeners, every frame, wouldn't I? Sounds pretty expensive, not to mention the try/catch blocks.

 

an exception is only thrown when an invalid delegate is executed, then its removed from the delegate list so for every destroyed object and every subscriber you get one exception. so when you have a couple of these each frame (probably less) this should not be an performance issue. its still an automatic way so everyone can judge if the convenience is worth the runtime overhead.

what do you think the runtime does with a multicast delegate? it surely also iterates over all elements in the list .

if its expensive only a measurement can tell. but its a "secure" way as subscribed objects may get destroyed and when you only forget to unsubscribe one your whole application crashes through an exception. so exceptions are thrown anyway if you want them or not. catching them let you react to their cause and try to recover the application from it.
however. if you go the manual way you can still use this method to find objects which do not properly unsubscribe (as printed by the debug log) and when you ship the game simply remove the test (or have an alternative code which just invokes for the build by defines).

 

Going back to the original question.

The cleanest way I can think to handle subscribing and unsubscribing events is to use some kind of manager to handle the lifetime of my objects. Eg: I have an EnemyManager class that has sole responsibility for creating, initializing, and destroying all Enemy objects in the game.

 

I was coming back to suggest something like this. I haven't done it for ages since C# provides delegates, but when coding in other languages I used to roll my own similar systems, and it'd be pretty simple to do so and include some kind of automatic unhook mechanism.

I also agree that the performance is unlikely to be an issue. For one of my projects I used a string based, brute force event system. It was naive and terrible (all events were passed to all active listeners, who were responsible for checking internally if it was relevant!) and the game was for low end mobile devices... but performance was a complete non-issue. The plan had been to replace it with something less stupid, but since it never even registered as a problem we never bothered.

So with that benchmark in mind, I don't think that using exceptions to unhook dead listeners is going to be an issue. Events probably don't take up much execution time as it is, and events that need to unhook something will be even rarer again. My only concern would be whether throwing the exception would cause allocation.

 

I've thought about using this approach (sounds a lot like the Factory pattern) myself, but in Unity it's still a bit dirty. The issue is that there's no way to stop 3rd party code from trashing the stuff the factory/manager class creates, since anything in the scene tree can be accessed and modified/destroyed globally. For instance, if something created by the factory gets re-parented, and then the parent gets destroyed, by default the factory created object will also get destroyed.

You can build in measures to work around this, but all of them add other forms of overhead.

 

In case anyone shall face the same problem: we now have UnityEvent thingy, which (among other nice features) will not invoke delegates to destroyed objects.

 

Yes, but the delegate will still hang around in there and will never get cleaned up. If using lamdas or delegates instead of named instance methods, they might still end up being called depending on what kind of method the compiler ends up generating from the lambda. For example if you do not reference any field or property of the MonoBehaviour in the lambda the compiler will probably generate a static method (until/if they switch to Roslyn compiler, https://stackoverflow.com/questions/30897647/delegate-caching-behavior-changes-in-roslyn).

I don't know if this behavior is consistent in Unity between platforms, but for platforms that use IL2CPP it should.

Interesting blogs:
https://blogs.unity3d.com/2015/06/03/il2cpp-internals-method-calls/
https://blogs.unity3d.com/2015/07/09/il2cpp-internals-garbage-collector-integration/

 

A usage of lambdas which I don't think I've seen mentioned is the fact that they allow you to hold additional info as part of the declaration itself. For example:

Code (csharp):

1. public void SaveData() {
2.    var data = new Data();
3.    ... // data gets filled in
4.    Dialog.Save(path => data.Save(path));
5. }

Now you could do it without using them:

Code (csharp):

1. private Data _dataInProgress;
2.  
3. public void SaveData () {
4.    var data = new Data();
5.    ... /// data gets filled in
6.    _dataInProgress = data;
7.    Dialog.Save(OnSaveConfirmed);
8. }
9.  
10. private void OnSaveConfirmed (string path) {
11.    _dataInProgress.Save(path);
12.    _dataInProgress = null;
13. }

As long as you're never saving multiple things at the same time anyway. And if using lambdas is causing problems, you should do it that way. But when it works, a lot of people find the former way cleaner.

Also, it's not that hard to unsubscribe lambdas:

Code (csharp):

1. private Action<string> _fnSpeech;
2.  
3. public void Start () {
4.    character.Talked += (_fnSpeech = message => { ... });
5. }
6.  
7. public void OnDestroy () {
8.    character.Talked -= _fnSpeech;
9. }

 

Here is my way to "automatically unsubscribe on destroy":

1. Make a "Subscriber" class that inherits MonoBehavior
2. Write the unsubscribe code in the OnDestroy of the "Subscriber"
3. When you want to subscribe, call gameObject.AddComponent<Subscriber>(), and then call subscriber.SetHanlder(xxx)

Then the unsubscribe code can be executed when "gameObject" is about to be destroyed because the "Subscriber" is one of its component. By this you don't need to add any code in the OnDestroy of the gameObject itself.

If you want to subscribe to various types of events, you can make the "SetHandler" generic. (as MonoBehavior itself cannot be generic in Unity)