CSync leaves stale change handlers after quitting the hosted game

[ratijas]

CSync leaves stale change handlers after quitting the hosted game

When the host quits the game back to the main menu, events handlers from ConfigSyncBehaviour.OnNetworkSpawn -> ConfigFile.SettingChanged somehow become stale such that its assignment _deltas[currentIndex] = syncedEntryBase.ToDelta(); causes NullReferenceException:

[Error  :   BepInEx] System.NullReferenceException: Object reference not set to an instance of an object
  at Unity.Collections.LowLevel.Unsafe.UnsafeList`1[T].get_Item (System.Int32 index) [0x00000] in <d0c0081e1ce74649912b55aa702cd13d>:0
  at Unity.Collections.NativeList`1[T].get_Item (System.Int32 index) [0x00000] in <d0c0081e1ce74649912b55aa702cd13d>:0
  at Unity.Netcode.NetworkList`1[T].set_Item (System.Int32 index, T value) [0x00023] in <895801699cfc4b4ab52267f31e2a4998>:0
  at CSync.Lib.ConfigSyncBehaviour+<>c__DisplayClass18_0.<OnNetworkSpawn>b__0 (System.Object _, BepInEx.Configuration.SettingChangedEventArgs args) [0x00014] in /home/runner/work/CSync/CSync/CSync/CSync/Lib/ConfigSyncBehaviour.cs:70
  at BepInEx.Configuration.ConfigFile.OnSettingChanged (System.Object sender, BepInEx.Configuration.ConfigEntryBase changedEntryBase) [0x00048] in <fe49c90fe8e24102b42489c11910c71c>:0

Every time the host quick the game, one more set of handlers become stale, so after 2nd quick there are two exceptions per value change, after 3rd quick there are three at once and so on.

It doesn't happen for clients unless they tried to host too. If a player previously hosted, quit, and joined as a client, then syncing a value from host would behave alright, but trying to change the value locally would result in the following InvalidOperationException:

[Error  :   BepInEx] System.InvalidOperationException: Client is not allowed to write to this NetworkList
  at Unity.Netcode.NetworkList`1[T].set_Item (System.Int32 index, T value) [0x00018] in <895801699cfc4b4ab52267f31e2a4998>:0
  at CSync.Lib.ConfigSyncBehaviour+<>c__DisplayClass18_1.<OnNetworkSpawn>b__0 (System.Object _, BepInEx.Configuration.SettingChangedEventArgs args) [0x00022] in D:\Code\CSync\CSync\CSync\Lib\ConfigSyncBehaviour.cs:70
  at BepInEx.Configuration.ConfigFile.OnSettingChanged (System.Object sender, BepInEx.Configuration.ConfigEntryBase changedEntryBase) [0x00048] in <fe49c90fe8e24102b42489c11910c71c>:0

AFAICT it does not affect the ability of CSync to synchronize the actual values. (Although yesterday I got a desync somehow, which prompted this investigation in the first place).

This bug happens even with the provided ExamplePlugin. The only way to fix is to close the game completely and launch again.

I use LethalConfig v1.4.6 by AinaVT to change values via in-game UI, and LCDirectLAN v1.1.3 by TIRTAGT to test between two devices on my local network.

[Lordfirespeed]

The first error sounds like a real issue, the second one is expected behaviour. Clients are not supposed to be able to assign to the Value of a synced config value

Seems like perhaps a gameobject isn't being destroyed when it should be? This shouldn't actually break anything, it just wastes resources and fills the log with error messages.

[ratijas]

On a second thought, OnNetworkSpawn is not idempotent, and doesn't have any corresponding OnNetworkDespawn to unsubscribe. What's more interesting is that the bug was able to hide because of this Count instead of a conventional 0..n loop!

            foreach (var syncedEntryBase in EntryContainer.Values)
            {
                var currentIndex = _deltas.Count;

The first error sounds like a real issue, the second one is expected behaviour. Clients are not supposed to be able to assign to the Value of a synced config value

I... don't think it should result in an exception though? Not sure if this is a problem of a lack of integration between LethalConfig and CSync, but possible

[ratijas]

Through a quick goggling I found this article about weak event handlers in C#, but seems like it only applies to desktop Windows and proprietary MS stuff? https://learn.microsoft.com/en-us/dotnet/desktop/wpf/events/weak-event-patterns?redirectedfrom=MSDN

[Lordfirespeed]

On a second thought, OnNetworkSpawn is not idempotent, and doesn't have any corresponding OnNetworkDespawn to unsubscribe.

That ought to fix it - feel free to PR

I... don't think it should result in an exception though?

LethalConfig should catch the exception and show its message to yh user

[ratijas]

LethalConfig should catch the exception and show its message to the user

LethalConfig has nothing to do with this. Both exceptions are caused by CSync trying to modify a dead list, and BepInEx catching and printing them to the console.