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 all commits
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
73 changes: 61 additions & 12 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>)> _settingChangedListeners = new();
private readonly List<(SyncedEntryBase, EventHandler)> _entrySyncEnabledChangedListeners = new();

[MemberNotNull(nameof(EntryContainer))]
private void EnsureEntryContainer()
Expand All @@ -48,7 +52,6 @@ private void EnsureEntryContainer()
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()
Expand All @@ -59,28 +62,32 @@ public override void OnNetworkSpawn()
{
_syncEnabled.Value = true;

var index = 0;
foreach (var syncedEntryBase in EntryContainer.Values)
{
var currentIndex = _deltas.Count;
_deltas.Add(syncedEntryBase.ToDelta());

syncedEntryBase.BoxedEntry.ConfigFile.SettingChanged += (_, args) =>
void onSettingChanged(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 += onSettingChanged;
_settingChangedListeners.Add((configFile, onSettingChanged));

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

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

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

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

base.OnNetworkSpawn();
}

public override void OnNetworkDespawn()
{
EnsureEntryContainer();

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

foreach (var (syncedEntryBase, onEntrySyncEnabledChanged) in _entrySyncEnabledChangedListeners)
{
syncedEntryBase.SyncEnabledChanged -= onEntrySyncEnabledChanged;
}

_settingChangedListeners.Clear();
_entrySyncEnabledChangedListeners.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