RemoveAt should *Always* remove items from a list

[FREEZX]

Just got kicked in the nuts by a race condition error where sometimes, after calling Despawn on an object, in the same frame other scripts would not be updated on the IsServer and InDeinitializing fields, and I was calling [SyncList].RemoveAt(i), followed by i-- in a for loop.
I never thought RemoveAt could possibly fail to remove an item from a list in any case, so my game was freezing in an endless loop.
All that was wrapped in an if (IsServer && !IsDeinitializing)
Solved it by comparing the old count of items with the new count of items to make sure the item got removed, but spent plenty of time debugging 🤣

Based on that experience, I believe that all list manipulation functions should be reliable and always work as expected. Silently ignoring is the worst option possible IMO. At least throw an exception, so any erroneous code won't allow the programmer to get the game/editor stuck in an endless loop.

[FirstGearGames]

So this hapepned as clientHost? On which version? Because something similar was already fixed.

[FREEZX]

ClientHost, version 3.10.7R

[FirstGearGames]

Can you show the code? Is it happening on the server side I assume?
This sounds like a bug report if you can post it there. But also, it would be way more efficient to iterate everything in the loop, then use RemoveRange. Not sure if there's a remove range option...might need to add that in if not.

[FREEZX]

It's a little complex as there are a few systems at work with this bug, but here's the gist of it:
I have an Exploder enemy that explodes, deals damage to everything around, including itself, and despawns right after. Here is how that happens:

    [Server]
    public void Explode()
    {
        List<AttackStrategy> exploderStrategies = EnemyControl.AttackManager.AttackStrategies;
        for (int i = 0; i < exploderStrategies.Count; i++)
        {
            if (exploderStrategies[i] is ExplodeAttackStrategy)
            {
                ExplodeAttackStrategy eas = exploderStrategies[i] as ExplodeAttackStrategy;
                ExplodeAttackStrategyState easState = EnemyControl.AttackManager.GetStratState(eas) as ExplodeAttackStrategyState;
                ExplosionHelper.ProcessExplosion(
                        easState.OriginTrans.position,
                        eas.ExplosionRadius,
                        eas.AffectedLayers,
                        eas.BlockingLayers,
                        EnemyControl.PawnStatus.Damage * eas.BaseDamage,
                        eas.DamageFalloff);
                VfxManager.PlayVfxPooled(eas.ExplosionParticle, EnemyControl.AttackManager.transform.position, Vector3.zero);
                break;
            }
        }

        Despawn();
    }

the ExplosionHelper.ProcessExplosion is a fairly complex function that does a spherecast and deals damage to anything that is implements IDamageable. That includes the same enemy's damageable body parts. Damage taken on a body part propagates to the owner's PawnStatus. PawnStatus simply keeps a player or enemy's stats - health, defense, hunger etc...

Here's how the explosion damage is being applied:

    [Server]
    public void AddEffect(StatusEffect effect, bool stacking)
    {
        effect.StartTime = SkopjeTimeManager.CurrentTime;
        if (!stacking)
        {
            int idx = StatusEffects.FindIndex(x => x.Tag != null && x.Tag.Equals(effect.Tag));
            if (idx >= 0)
            {
                StatusEffects.RemoveAt(idx);
            }
        }

        EffectAdded?.Invoke(effect); // Andrej : Any processing to be done before entering the list(sync)
        StatusEffects.Add(effect);
    }

StatusEffects is a SyncList of StatusEffect, which is a class with information on how this pawnStatus is effected.

PawnStatus then processes all pending status effects in it's Update function:

    public void Update()
    {
        if (IsServer && !IsDeinitializing)
        {
            ApplyStatEffects();
            UpdateStatState(SkopjeTimeManager.CurrentTime);
        }
    }

Here's where the issue occured, including my dumb workaround:

    public void UpdateStatState(double gameTime)
    {
        for (int i = 0; i < StatusEffects.Count; ++i)
        {
            ApplyInstantStatEffect(StatusEffects[i]);

            if (StatusEffects[i].ShouldEffectBeRemoved(gameTime))
            {
                Debug.LogWarningFormat(
                    "DEBUG : Removing [{0}] [{1}]", StatusEffects[i].BuffStat.ToString(), StatusEffects[i].Value.ToString("F2"));
                // Workaround for despawn race condition
                int oldCount = StatusEffects.Count;
                StatusEffects.RemoveAt(i);
                // Remove success
                if(StatusEffects.Count < oldCount) {
                    --i;
                }
                continue;
            }
        }
        if (Defense <= 0)
            RemoveAllEffectsForBuff(BuffStat.Defense);
    }

Without the oldCount / newCount, this code hanged.

[FirstGearGames]

So you are ultimately just doing a for loop and using RemoveAt on the server? Is this server only issue, or also as clientHost?

[FREEZX]

Yes. I only had it happen as clientHost, as I don't ever use a standalone server

[FirstGearGames]

Unable to reproduce. Possibly solved by #505

There was another bug reported recently and solved in 3.11.8 where the client was getting duplicate entries under the following scenario:

This scenario occurs in the same tick.

• Server add entry in OnStartServer/Network.
• Server sends full SyncList to clientA for spawn.
• Server has dirty of newly added entry and sends that to all observers. As a result clientA gets extra entries.

As a resolution I made it so clients ignore potential duplicate entries such as this. I am wondering if this also resolved your bug because I cannot now replicate it.

Hopefully these changes did fix your problem. I'm going to close this out as unable to replicate and link the resolved issue. If you are still having the problem in 3.11.8 (releasing soon) please let me know/re-open this. I'll need you to also include a test project please.

Here's my test code...

        [Range(2, 10)]
        public int RemoveCount = 2;
        public bool RemoveTest;
        [SyncObject]
        private readonly SyncList<int> _removeAtBug = new SyncList<int>();

        private void Awake()
        {
            AddEntries(10);
        }


        private void Update()
        {
            TryRemoveTest();
         }

        private void AddEntries(int value)
        {
            for (int i = 0; i < value; i++)
                _removeAtBug.Add(i);
        }

        private void TryRemoveTest()
        {
            if (!RemoveTest)
                return;

            RemoveTest = false;
            if (RemoveCount < _removeAtBug.Count)
                AddEntries((_removeAtBug.Count - RemoveCount) * 2);

            int removed = 0;
            //Remove from the start and to try and force error.
            for (int i = 0; i < _removeAtBug.Count; i++)
            {
                if (i == 0 || i == (_removeAtBug.Count - 1))
                    _removeAtBug.RemoveAt(i);

                removed++;
                if (removed == RemoveCount)
                    break;
            }
            Debug.Log($"Removed {removed} entries.");
        }