Memory leak unsubscribing using anonymous delegate

Unity Courses Talk
,
#1

If you subscribe like this player.Health.OnDie += health => HandlePlayerDie(player); then you cannot unsubscribe using player.Health.OnDie -= health => HandlePlayerDie(player);, because it is a reference to a new anonymous delegate.

It can be validated with the following experiment:

       public event Action Test;

        private void Start()
        {
            Test += () => OnTest();
            Test -= () => OnTest();
            
            Test?.Invoke();
        }
      
        private void OnTest()
        {
            Debug.LogWarning("DID NOT UNSUBSCRIBE");
        }

Better approach would be resolving tank player from Health object inside a method.

2 Likes
Unsubscribing using anonymous delegate does not work
#2

Is that the case?

I think in this particular example (that one in course) it really doesnt matter that much, becouse health component will be destroyed along with the player game object instance.

#3

It is indeed the case. If the object lives for the duration of the game it doesn’t matter because it will never need to be unsubscribed but if you do need to unsubscribe, you cannot do it if you subscribed to an anonymous function. I personally try to completely avoid subscribing events to anonymous functions, even if they are going to live forever

1 Like
#4

While it does not matter in this particular case, the unsubscribe simply does not work and will create unnecessary confusion when a developer will try to apply the pattern for a different context.

1 Like
#5

We are subscribing to event Action allocation, that is in the class instance. I don’t get why this function is anonymous?

#6 
() => OnTest();

This bit creates an anonymous function. It is anonymous because it has no name. There is no way to reference it again.

Test += () => OnTest();

This subscribes to Test using a delegate it creates

Test -= () => OnTest();

This unsubscribes from Test using a delegate it creates. It is not the delegate that was created when we subscribed, it is a new one. There is no reference.

In @drake’s example, the correct way to subscribe and unsubscribe would be

public event Action Test;

private void Start()
{
	Test += OnTest;
	Test -= OnTest;
		
	Test?.Invoke();
}
  
private void OnTest()
{
	Debug.LogWarning("DID NOT UNSUBSCRIBE");
}
1 Like
#7

I found a solution to this issue

    private void HandlePlayerSpawned(TankPlayer player)
    {
        player.Health.OnDie += HandlePlayerDie;
    }

    private void HandlePlayerDespawned(TankPlayer player)
    {
        player.Health.OnDie -= HandlePlayerDie;
    }

    private void HandlePlayerDie(Health health)
    {
        TankPlayer player = health.GetComponentInParent<TankPlayer>();

        if (player != null)
        {
            Destroy(player.gameObject);
            StartCoroutine(RespawnPlayer(player.OwnerClientId));
        }
    }

Though, I believe TankPlayer shouldn’t have static events

1 Like
#8

Was just about to post about this as soon as I saw the instructor write that code. Glad to see that @drake had already pointed the problem along with a nice little example.

Along with the memory leak, this also shows a gap in instructor’s knowledge (or at least an oversight, cause I’ve seen even senior programmers new to C# make this mistake). This, I think, is a more serious problem because viewers who are not familiar with intermediate C# will end up following the same code. So it might not be harmful for this specifc game, but it for sure is spreading a wrong information to the viewers.

@DrDisappointment’s solution is good, as it avoid the anonymous function. A less elegant alternative to this would be to store a mapping between the TankPlayer and its anonymous function in the RespawnHandler. That way the correct function can be unregistered while despawning.

Adding my code for reference:

private readonly Dictionary<TankPlayer, Action<Health>> _playerDeathHandlers = new();

private void HandlePlayerSpawn(TankPlayer player)
{
    if (_playerDeathHandlers.TryAdd(player, (_) => HandlePlayerDied(player)))
    {
        player.Health.OnDie += _playerDeathHandlers[player];
    }
}

private void HandlePlayerDespawn(TankPlayer player)
{
    if (_playerDeathHandlers.TryGetValue(player, out var deathHandler))
    {
        player.Health.OnDie -= deathHandler;
    }
}
2 Likes

Privacy & Terms