Skip to content

fix delegates lifetime: unsubscribe stale handlers on network despawn #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix delegates lifetime: unsubscribe stale handlers on network despawn #14

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9898ee8
fix delegates lifetime: unsubscribe stale handlers on network despawn
ratijas Aug 8, 2025
1c7cb9b
rename and partially revert Awake
ratijas Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 60 additions & 18 deletions CSync/CSync/Lib/ConfigSyncBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using BepInEx.Configuration;
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Unity.Netcode;
using UnityEngine;
using LogLevel = BepInEx.Logging.LogLevel;
Expand Down Expand Up @@ -36,7 +38,9 @@ public bool SyncEnabled
}

private readonly NetworkVariable<bool> _syncEnabled = new();
private NetworkList<SyncedEntryDelta> _deltas = null!;
private readonly NetworkList<SyncedEntryDelta> _deltas = new();
private readonly List<(ConfigFile, EventHandler<SettingChangedEventArgs>)> _configFileDelegates = new();
private readonly List<(SyncedEntryBase, EventHandler)> _entrySyncEnabledDelegates = new();

[MemberNotNull(nameof(EntryContainer))]
private void EnsureEntryContainer()
Expand All @@ -45,12 +49,6 @@ private void EnsureEntryContainer()
throw new InvalidOperationException("Entry container has not been assigned.");
}

private void Awake()
Copy link

@Lordfirespeed Lordfirespeed Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, do not do this. Revert.

Copy link
Author

@ratijas ratijas Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@Lordfirespeed Lordfirespeed Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
EnsureEntryContainer();
_deltas = new NetworkList<SyncedEntryDelta>();
}

public override void OnNetworkSpawn()
{
EnsureEntryContainer();
Expand All @@ -59,28 +57,30 @@ public override void OnNetworkSpawn()
{
_syncEnabled.Value = true;

foreach (var syncedEntryBase in EntryContainer.Values)
foreach (var (syncedEntryBase, index) in EntryContainer.Values.Select((e, i) => (e, i)))
Copy link

@Lordfirespeed Lordfirespeed Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ratijas ratijas Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@ratijas ratijas Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++;
            }

Copy link

@Lordfirespeed Lordfirespeed Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ratijas ratijas Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@Lordfirespeed Lordfirespeed Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var currentIndex = _deltas.Count;
_deltas.Add(syncedEntryBase.ToDelta());

syncedEntryBase.BoxedEntry.ConfigFile.SettingChanged += (_, args) =>
void configFileDelegate(object sender, SettingChangedEventArgs args)
{
if (!ReferenceEquals(syncedEntryBase.BoxedEntry, args.ChangedSetting)) return;
_deltas[currentIndex] = syncedEntryBase.ToDelta();
};
_deltas[index] = syncedEntryBase.ToDelta();
}
var configFile = syncedEntryBase.BoxedEntry.ConfigFile;
configFile.SettingChanged += configFileDelegate;
_configFileDelegates.Add((configFile, configFileDelegate));

syncedEntryBase.SyncEnabledChanged += (_, args) =>
void entrySyncEnabledDelegate(object sender, EventArgs args)
{
_deltas[currentIndex] = syncedEntryBase.ToDelta();
};
_deltas[index] = syncedEntryBase.ToDelta();
}
syncedEntryBase.SyncEnabledChanged += entrySyncEnabledDelegate;
_entrySyncEnabledDelegates.Add((syncedEntryBase, entrySyncEnabledDelegate));
}

InitialSyncCompletedHandler?.Invoke(this, EventArgs.Empty);
return;
}

if (IsClient)
else if (IsClient)
{
_syncEnabled.OnValueChanged += OnSyncEnabledChanged;
_deltas.OnListChanged += OnClientDeltaListChanged;
Expand All @@ -94,6 +94,48 @@ public override void OnNetworkSpawn()

InitialSyncCompletedHandler?.Invoke(this, EventArgs.Empty);
}

base.OnNetworkSpawn();
}

public override void OnNetworkDespawn()
{
EnsureEntryContainer();

if (IsServer)
{
foreach (var (configFile, configFileDelegate) in _configFileDelegates)
{
configFile.SettingChanged -= configFileDelegate;
}

foreach (var (syncedEntryBase, entrySyncEnabledDelegate) in _entrySyncEnabledDelegates)
{
syncedEntryBase.SyncEnabledChanged -= entrySyncEnabledDelegate;
}

_configFileDelegates.Clear();
_entrySyncEnabledDelegates.Clear();
// NetworkList and NetworkVariable can not be modified now because the network manager has already shut down.
// See https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/pull/3502
// Should not be needed anyway, since this component isn't being reused between network sessions.
// _deltas.Clear();
// _syncEnabled.Value = false;
}
else if (IsClient)
{
DisableOverrides();
Copy link

@Lordfirespeed Lordfirespeed Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ratijas ratijas Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@Lordfirespeed Lordfirespeed Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


foreach (var delta in _deltas)
{
ResetOverrideValue(delta);
}

_deltas.OnListChanged -= OnClientDeltaListChanged;
_syncEnabled.OnValueChanged -= OnSyncEnabledChanged;
}

base.OnNetworkDespawn();
}

public override void OnDestroy()
Expand Down