Warning for removing subscription from events

[pnarimani]

Imagine that you have an event in a class and another class is subscribing to it:

    public class ClassA : MonoBehaviour
    {
        public event Action MyEvent;
    }

    public class ClassB : MonoBehaviour
    {
        public ClassA ReferenceToA;

        private void Awake()
        {
            ReferenceToA.MyEvent += AOnMyEvent;
        }

        private void AOnMyEvent()
        {
            gameObject.transform.position = new Vector3(100,1, 100);
        }
    }

When class B gets instantiated, a subscription will be added to MyEvent. But when it gets destroyed, it doesn't remove the subscription. On the next MyEvent invocation a MissingReferenceException will occur.
Resharper/Rider should warn the programmer about this and provide a quick fix to unsubscribe from every subscribed event in OnDestroy.

[rob8861]

this is not how you do that in Unity. .NET events should be subscribed to in OnEnable and unsubscribed from in OnDisable.

your code should look like this

public class ClassA : MonoBehaviour
{
    public event Action MyEvent;
}

public class ClassB : MonoBehaviour
{
    public ClassA ReferenceToA;

    private void OnEnable()
    {
        ReferenceToA.MyEvent += AOnMyEvent;
    }

   private void OnDisable()
    {
        ReferenceToA.MyEvent -= AOnMyEvent;
    }

    private void AOnMyEvent()
    {
        gameObject.transform.position = new Vector3(100,1, 100);
    }
}

[citizenmatt]

Do you have any references for this as a best practice?

[bddckr]

I never found any, but with VRTK we came to the conclusion that most users of our toolkit for Unity expect a component to not do anything if it's disabled. Therefore we settled on the same OnEnable + OnDisable implementation, but it kinda depends. There will always be that one need that needs a hacky solution that ultimately wants event listeners to run, even if they are part of a disabled component.

[rob8861]

the official unity training videos and manuals recommend sub'ing and unsub'ing in OnEnable and OnDisable. https://unity3d.com/learn/tutorials/topics/scripting/events

However, I agree with bddckr, this is just a "best practice" and in most cases works best to avoid unexpected behaviors or memory leaks.

the reason the OP kept seeing calls to the method is because the script kept holding a reference to the event and therefore it never got destroyed. In fact, if I remember correctly, Destroy doesn't actually nullefy an object but just marks it for GC.

[pnarimani]

this is not how you do that in Unity. .NET events should be subscribed to in OnEnable and unsubscribed from in OnDisable.

I agree with you. But I think resharper should leave that choice to the user.
I'm requesting a warning for "Missing event unsubscriptions".

Given the code below:

    public class ClassA : MonoBehaviour
    {
        public event Action MyEvent;
    }

    public class ClassB : MonoBehaviour
    {
        public ClassA ReferenceToA;

        private void Awake()
        {
            ReferenceToA.MyEvent += Always;
        }

        private void OnEnable()
        {
            ReferenceToA.MyEvent += OnlyWhenEnabled;
        }

        private void OnlyWhenEnabled()
        {
        }

        private void Always()
        {

        }
    }

Resharper should produce 2 warnings: 1- Missing event unsubscription for ReferenceToA.MyEvent += Always;. Quick Fix: Add ReferenceToA.MyEvent -= Always; in OnDestroy method.

2- Missing event unsubscription for ReferenceToA.MyEvent += OnlyWhenEnabled;. Quick Fix: Add ReferenceToA.MyEvent -= OnlyWhenEnabled; in OnDisable method.

[pnarimani]

Something that I discovered today was that If you build the game with IL2CPP backend on Android platform, it doesn't throw MissingReferenceException. It prints this error in logcat: an event listener is still subscribed to an event with the method MethodNameHere even though it is null. Be sure to balance your event subscriptions.