A Native Collection has not been disposed, resulting in a memory leak

shrike86 commented 2 months ago

Description

I am using NetworkList's but I keep getting exceptions in the editor related to a memory leaks. I don't really have reproduction steps, but it seems to happen frequently. Often, I will make code changes during run time and then stop and restart my server and client to test and on startup the exceptions will occur.

I am initialising the NetworkList in awake and I always call base.OnDestroy in my overriden OnDestroy methods.

Environment

OS: [Windows 11]
Unity Version: [2023.2.13f1]
Netcode Version: [1.8.1]

Additional Context

Exception Message:

A Native Collection has not been disposed, resulting in a memory leak. Allocated from: Unity.Collections.NativeList1:.ctor(Int32, AllocatorHandle) (at .\Library\PackageCache\com.unity.collections@1.4.0\Unity.Collections\NativeList.cs:116) Unity.Netcode.NetworkList1:.ctor() (at .\Library\PackageCache\com.unity.netcode.gameobjects@1.8.1\Runtime\NetworkVariable\Collections\NetworkList.cs:15) System.Reflection.RuntimeConstructorInfo:InternalInvoke(RuntimeConstructorInfo, Object, Object[], Exception&) System.Reflection.RuntimeConstructorInfo:InternalInvoke(Object, Object[], Boolean) System.RuntimeType:CreateInstanceMono(Boolean, Boolean) System.RuntimeType:CreateInstanceSlow(Boolean, Boolean, Boolean, Boolean) System.RuntimeType:CreateInstanceDefaultCtor(Boolean, Boolean, Boolean, Boolean, StackCrawlMark&) System.Activator:CreateInstance(Type, Boolean, Boolean) System.Activator:CreateInstance(Type, Boolean) Unity.Netcode.Editor.NetworkBehaviourEditor:RenderNetworkVariable(Int32) (at .\Library\PackageCache\com.unity.netcode.gameobjects@1.8.1\Editor\NetworkBehaviourEditor.cs:72) Unity.Netcode.Editor.NetworkBehaviourEditor:OnInspectorGUI() (at .\Library\PackageCache\com.unity.netcode.gameobjects@1.8.1\Editor\NetworkBehaviourEditor.cs:268) UnityEditor.UIElements.<>c__DisplayClass74_0:b__0() UnityEngine.UIElements.IMGUIContainer:DoOnGUI(Event, Matrix4x4, Rect, Boolean, Rect, Action, Boolean) UnityEngine.UIElements.IMGUIContainer:DoMeasure(Single, MeasureMode, Single, MeasureMode) UnityEngine.UIElements.VisualElement:Measure(VisualElement, LayoutNode&, Single, LayoutMeasureMode, Single, LayoutMeasureMode, LayoutSize&) UnityEngine.UIElements.Layout.LayoutDelegates:InvokeMeasureFunction(LayoutNode&, Single, LayoutMeasureMode, Single, LayoutMeasureMode, LayoutSize&) UnityEngine.UIElements.Layout.LayoutNative:CalculateLayout(IntPtr, Single, Single, Int32, IntPtr) UnityEngine.UIElements.Layout.LayoutProcessorNative:UnityEngine.UIElements.Layout.ILayoutProcessor.CalculateLayout(LayoutNode, Single, Single, LayoutDirection) UnityEngine.UIElements.Layout.LayoutProcessor:CalculateLayout(LayoutNode, Single, Single, LayoutDirection) UnityEngine.UIElements.Layout.LayoutNode:CalculateLayout(Single, Single) UnityEngine.UIElements.UIRLayoutUpdater:Update() UnityEngine.UIElements.VisualTreeUpdater:UpdateVisualTreePhase(VisualTreeUpdatePhase) UnityEngine.UIElements.Panel:UpdateWithoutRepaint() UnityEditor.EditorElementUpdater:CreateInspectorElementsForMilliseconds(Int64) UnityEditor.PropertyEditor:Update() UnityEditor.HostView:SendUpdate() UnityEditor.EditorApplication:Internal_CallUpdateFunctions()

Example usage of NetworkList:

public class NetworkActionState : NetworkBehaviour
    {
        public NetworkList<CharacterAction> CharacterActions { get => _characterActions; }

        [Inject] IPublisher<CharacterActionsInitialisedMessage> _charActionsInitPub;

        IServerCharacter _serverCharacter;
        NetworkList<CharacterAction> _characterActions;

        private void Awake()
        {
            _characterActions = new();
            _serverCharacter = GetComponent<IServerCharacter>();
        }

        public override void OnDestroy()
        {
            base.OnDestroy();
        }

        public override void OnNetworkSpawn()
        {
            if (!IsServer) { enabled = false; }
            else
            {
                if (_serverCharacter is ServerEnemy serverEnemy)
                {

                }
                else if (_serverCharacter is ServerPlayer serverPlayer)
                {
                    StartCoroutine(WaitToInit(serverPlayer));
                }
            }
        }

        IEnumerator WaitToInit(ServerPlayer serverPlayer)
        {
            PersistentPlayer persistentPlayer = null;
            yield return new WaitUntil(() => GameDataSource.Instance.PersistentPlayerRuntimeCollection.TryGetPlayer(OwnerClientId, out persistentPlayer));
            Initialise(persistentPlayer);
        }

        void Initialise(PersistentPlayer persistentPlayer)
        {
            var networkCharActions = persistentPlayer.NetworkCharacterState.InGameChar.Value.Actions;
            var actionData = JsonConvert.DeserializeObject<CharacterActions>(networkCharActions);

            foreach (var action in actionData.Actions)
            {
                var actionId = new ActionID { ID = action.ActionID };
                CharacterAction charAction = new()
                {
                    ActionID = actionId,
                    Learnt = action.Learnt,
                };

                var serializedAction = JsonConvert.SerializeObject(charAction);
                Utils.Logger.LogInformation($"Adding new character action: {serializedAction}", LoggingCategory.Actions);

                _characterActions.Add(charAction);
            }

            StartCoroutine(WaitToPublishCharacterActionsInitMessage());
        }

        IEnumerator WaitToPublishCharacterActionsInitMessage()
        {
            yield return new WaitForEndOfFrame();
            _charActionsInitPub.Publish(new CharacterActionsInitialisedMessage());
        }

        public bool TryFindAction(ActionID actionId, out CharacterAction characterAction)
        {
            characterAction = new CharacterAction();

            for (int i = 0; i < _characterActions.Count; i++)
            {
                if (_characterActions[i].ActionID == actionId)
                {
                    characterAction = _characterActions[i];
                    break;
                }
            }
            return characterAction.ActionID.ID != 0;
        }
    }

Risord commented 2 months ago

It's not a bug. NetworkList implements IDisposable interface which tells that it must be free when no longer used. Since in this case you are the one who created NetworkList it's also your responsibility to call Dispose method. Same would apply to .NET native MemoryStream for example.

shrike86 commented 2 months ago

It's not a bug. NetworkList implements IDisposable interface which tells that it must be free when no longer used. Since in this case you are the one who created NetworkList it's also your responsibility to call Dispose method. Same would apply to .NET native MemoryStream for example.

Thank you for the explanation regarding the disposal of NetworkList. I understand the analogy to other IDisposable implementations in .NET. However, I have a couple of concerns:

Firstly, there seems to be a lack of documentation on NetworkList needing explicit disposal. If this is an essential requirement, could you point me to where this is documented? Clear documentation would greatly aid in avoiding such issues.

Secondly, the example provided in the NGO documentation does not explicitly call the Dispose method on NetworkList. This example (see below) led me to believe that disposal might be handled internally, especially since NetworkList cannot be initialised at declaration like NetworkVariable and must be initialized in Awake to avoid memory leaks. Here’s the snippet from the documentation:

public class PlayerState : NetworkBehaviour
{
    // ^^^^^^^ including all code from previous example ^^^^^^^

    // The weapon booster currently applied to a player
    private NetworkVariable<WeaponBooster> PlayerWeaponBooster = new NetworkVariable<WeaponBooster>();

    /// <summary>
    /// A list of team members active "area weapon boosters" that can be applied if the local player
    /// is within their range.
    /// </summary>
    private NetworkList<AreaWeaponBooster> TeamAreaWeaponBoosters;

    void Awake()
    {
        //NetworkList can't be initialized at declaration time like NetworkVariable. It must be initialized in Awake instead.
        //If you do initialize at declaration, you will run into Memmory leak errors.
        TeamAreaWeaponBoosters = new NetworkList<AreaWeaponBooster>();
    }

    void Start()
    {
        /*At this point, the object hasn't been network spawned yet, so you're not allowed to edit network variables! */
        //list.Add(new AreaWeaponBooster());
    }

    void Update()
    {
        //This is just an example that shows how to add an element to the list after its initialization:
        if (!IsServer) { return; } //remember: only the server can edit the list
        if (Input.GetKeyUp(KeyCode.UpArrow))
        {
            TeamAreaWeaponBoosters.Add(new AreaWeaponBooster()));
        }
    }

    public override void OnNetworkSpawn()
    {
        base.OnNetworkSpawn();
        if (IsClient)
        {
            TeamAreaWeaponBoosters.OnListChanged += OnClientListChanged;
        }
        if (IsServer)
        {
            TeamAreaWeaponBoosters.OnListChanged += OnServerListChanged;
            TeamAreaWeaponBoosters.Add(new AreaWeaponBooster()); //if you want to initialize the list with some default values, this is a good time to do so.
        }
    }

    void OnServerListChanged(NetworkListEvent<AreaWeaponBooster> changeEvent)
    {
        Debug.Log($"[S] The list changed and now has {TeamAreaWeaponBoosters.Count} elements");
    }

    void OnClientListChanged(NetworkListEvent<AreaWeaponBooster> changeEvent)
    {
        Debug.Log($"[C] The list changed and now has {TeamAreaWeaponBoosters.Count} elements");
    }
}

Lastly, the stack trace from my issue originates from Unity.Netcode.Editor.NetworkBehaviourEditor:RenderNetworkVariable, suggesting the problem might be linked to Unity's handling of NetworkList in the editor rather than misuse of the class.

Given these points, I would appreciate further clarification on the proper usage and disposal of NetworkList, as well as any adjustments that might be needed in the documentation or editor tooling to prevent such issues.

Risord commented 2 months ago

Sorry for sloppy response. Indeed it seems that NetworkList is intended to be disposed automatically by NetworkBehavior.OnDestroy (just like NetworkVariable which seems to implements IDisposable too).

Stacktrace is indeed suspicious.

Unity-Technologies / com.unity.netcode.gameobjects