fix delegates lifetime: unsubscribe stale handlers on network despawn

[ratijas]

CSync leaves stale change handlers after quitting the hosted game.

When the host quits the game back to the main menu, ConfigSyncBehaviour despawns and gets destroyed by Unity (not by .NET GC), but any delegates connected to events in OnNetworkSpawn method remain and go stale.

So next time a player hosts a lobby without shutting down and re-launching the game completely, when any of subscribed config files or config entries change, stale delegates execute and try to assign to a dead NetworkList _deltas[index]. Of course it results in an exception being logged to the console by BepInEx (which wraps event handler invocation in a try-catch).

To fix this, store subscribed delegates, and properly unsubscribe them during despawn.

Note that NetworkList and _syncEnabled variable can not be cleared/reset due to Unity issue which only got resolved in the most recent version.

References:

• https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/delegates/using-delegates
• fix: Avoid throwing exception when using NetworkList without a NetworkManager Unity-Technologies/com.unity.netcode.gameobjects#3502

Fixes #13

[ratijas]

I could combine two lists of delegates into one with a huge signature or an internal struct, but it doesn't matter too much.

[ratijas]

Polite ping?

[Lordfirespeed]

No, do not do this. Revert.

[ratijas]

_deltas is already initialized in the field declaration, so its reassignment shouldn't be needed (moreover, it is marked as readonly now).

[Lordfirespeed]

At the time I wrote the code, I knew field declaration initialisers existed and I intentionally didn't use one (hence null!)
I had a reason for doing that, I don't want to go digging for it again. This change introduces a bug risk which wasn't there before, accepting it would be a liability.

[Lordfirespeed]

I intentionally did not do this. Revert or use an index-based for-loop

[ratijas]

Mind if I ask, for what reason? Is it not performant enough for a once-in-a-lifetime execution? Or not readable? Or something else?

[ratijas]

Cannot apply indexing with [] to an expression of type 'ICollection<SyncedEntryBase>'[CS0021](https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CS0021))

Dictionary's Values is not an array, so straightforward index-based for loop is not possible. No matter how you write it, it would suck either way. For example,

            var index = 0;
            foreach (var syncedEntryBase in EntryContainer.Values)
            {
                // do stuff
                // 20 lines of code later (possibly with break and continue), don't forget to increment
                index++;
            }

[Lordfirespeed]

Option 1: Array.from (less preferable)
Option 2: use IDictionary.Count to determine bounds of index iteration
Option 3: leave it as it is

My reasonining wasn't really about performance, it's more about readability. The way I originally wrote it was more readable. .Select() to get an index is magic BS that you have to be familiar with C# to know.

[ratijas]

Yeah, Select() isn't the sweetest part of C#, but python-style enumerate() will only come in the next C#/.NET release :(

anyways, as you might have seen, I already changed it to index++ at the end of the loop. It gets the job done.

[Lordfirespeed]

I think I prefer using currentIndex = blah.size at the top of the loop, since that will keep all the loop logic together 😅

[Lordfirespeed]

This has been copied from OnDestroy, I don't like the duplication. Please can you explain why this is necessary?
If it's necessary, please can you test that this definitely gets run by all clients - then we can remove the OnDestroy override to remove the duplication

[ratijas]

I don't even remember for sure. But it perfectly mirrors whatever setup is being performed in OnNetworkSpawn(), so there's that. Probably gonna re-test it later.

[Lordfirespeed]

I mainly don't like the possibility that the clean-up is run twice.