NetworkVariable and NetworkList throw NullReferenceException if modified during application quit

[A1win]

Description

When Shutdown() is called on the server during Application Quit and a NetworkObject's OnNetworkDespawn causes a different NetworkObject to modify a NetworkList or NetworkVariable that's attached to it, a NullReferenceException is thrown.

For NetworkList.cs, the exception gets thrown in the MarkNetworkObjectDirty function on line 78.

For NetworkVariableBase.cs, it gets thrown on SetDirty function on line 98.

This is an edge case of #2606 that was fixed in Netcode 1.5.2. Previously, this error always occurred during Shutdown() even if the shutdown was not caused by application quit.

As a less than ideal workaround, it seems to be possible to check for NetworkManager.ShutdownInProgress before making modifications to NetworkVariables or NetworkLists during application quit.

Reproduce Steps

1. Spawn NetworkObject A with a NetworkVariable attached to it.
2. Spawn NetworkObject B with a NetworkList attached to it.
3. Spawn NetworkObject C with an OnNetworkDespawn function that modifies the NetworkVariable of NetworkObject A.
4. Spawn NetworkObject D with an OnNetworkDespawn function that modifies the NetworkList of NetworkObject B.
5. Quit the application as the server (or exit play mode in the Editor), causing a shutdown. Notice errors thrown by NetworkVariableBase.cs and NetworkList.cs.

I'm not sure if this is a reliable way to reproduce the issue and I don't currently have a small sample project to reproduce it in.

Actual Outcome

NetworkVariableBase and NetworkList throw a NullReferenceException when modified during application quit.

Expected Outcome

No exceptions are thrown when modifying a NetworkVariable or a NetworkList during application quit.

Environment

• OS: Windows 10
• Unity Version: 2022.3.7f1
• Netcode Version: 1.5.2
• Netcode Commit: 3636884

[cbwan]

I have the same issue in 1.6.0.
OnApplicationQuit calls NetworkVariableBase::SetDirty which then causes an exception because m_NetworkBehaviour.NetworkManager.BehaviourUpdater is null.

[NoelStephensUnity]

@A1win @cbwan
In order to better understand the issue, could you describe why you would want to modify a NetworkVariableBase derived class when a NetworkObject is despawning since the modifications will not be relayed to any client? I am just trying to better understand the use-case in order to determine if there is an alternate recommended way to handle what your code is doing during OnNetworkDespawn.

Modifying a NetworkVariableBase derived class during OnNetworkSpawn (as long as the instance has write permissions) is typically the better path to approach and if you have some values you wish to preserve (i.e. just despawning but not destroying) then you might be better setting those values in a normal property that is applied during the re-spawning of the instance.

Otherwise, I would highly recommend using NetworkManager.ShutdownInProgress prior modifying any NetworkVariableBase derived class (or invoking a method on a component of a different NetworkObject) from within OnNetworkDespawn.

[cbwan]

@NoelStephensUnity Thank you for your answer. In my case I am not modifying the network variable during OnNetworkSpawn.
I have a simple private NetworkVariable _usersNb that I increase OnClientConnectedCallback and decrease on OnClientDisconnectCallback and that's all.

[cbwan]

Although it is possible that in my particular case the application quit and the disconnection of all the clients happen at the exact same frame. When I get the clients to quit before the host quits I don't have the error.

[A1win]

@NoelStephensUnity In my case, I don't intentionally modify the NetworkVariable or NetworkList during shutdown, but there are some pretty common use cases where it makes sense to modify them when a different NetworkObject despawns. For example:

• A unit dies in an RTS game, which causes the supply resource that unit is reserving to release, and causes a NetworkVariable (in a different NetworkObject) to update so that the player knows how much of that resource they now have remaining.
• A character dies, which causes a NetworkList of characters to update, to inform a player that the character is no longer on that list.

These kind of use cases happen commonly throughout the game, but it just happens that they also modify a NetworkVariable or a NetworkList when the application is shutting down, because everything is despawning at that point. Adding a check for ShutdownInProgress is a workaround, but it would require us to add the check pretty much everywhere where we modify a NetworkList or a NetworkVariable, just so that it wouldn't cause errors during application quit.

This only happens during application quit and doesn't seem to cause any other problems than a logged error, so it's not necessarily critical, but probably worth fixing eventually anyway. 🙂

[NoelStephensUnity]

@A1win

Adding a check for ShutdownInProgress is a workaround, but it would require us to add the check pretty much everywhere where we modify a NetworkList or a NetworkVariable, just so that it wouldn't cause errors during application quit.

I wouldn't think that you should have to check all places, just when despawning?
Example pseudo code:

    public class UnitCounterExample : NetworkBehaviour
    {
        private NetworkVariable<int> m_UnitCount = new NetworkVariable<int>();
        public void UnitCountChanged(bool increment = true)
        {
            // Assuming server write privs
            if (!IsServer)
            {
                return;
            }
            if (increment)
            {
                m_UnitCount.Value++;
            }
            else
            {
                m_UnitCount.Value--;
            }
        }
    }

    public class Unit : NetworkBehaviour
    {
        public UnitCounterExample UnitCounterExample;
        public override void OnNetworkSpawn()
        {
            if (IsServer)
            {
                UnitCounterExample.UnitCountChanged();
            }
            base.OnNetworkSpawn();
        }

        public override void OnNetworkDespawn()
        {
            if (!NetworkManager.ShutdownInProgress && IsServer)
            {
                UnitCounterExample.UnitCountChanged(false);
            }
            base.OnNetworkDespawn();
        }
    }

I guess it would depend on how things are put together.

Soo... what I think the issue is at hand is you are getting an exception for a situation that I don't think you should be getting an exception for...
What NetworkVariable.Value is currently:

        public virtual T Value
        {
            get => m_InternalValue;
            set
            {
                // Compare bitwise
                if (NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
                {
                    return;
                }

                if (m_NetworkBehaviour && !CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
                {
                    throw new InvalidOperationException("Client is not allowed to write to this NetworkVariable");
                }

                Set(value);
                m_IsDisposed = false;
            }
        }

What I think we could change to avoid this exception:

        public virtual T Value
        {
            get => m_InternalValue;
            set
            {
                // Compare bitwise
                if (NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))
                {
                    return;
                }

                // If no networkbehaviour, we can't determine permissions so just return
                // If no NetworkManager, we return
                // If shutting down, we return
                if (m_NetworkBehaviour == null || NetworkBehaviour.NetworkManager == null || NetworkBehaviour.NetworkManager.ShutdownInProgress )
                {
                    return;
                }

                if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
                {
                    throw new InvalidOperationException("Client is not allowed to write to this NetworkVariable");
                }

                Set(value);
                m_IsDisposed = false;
            }
        }

It shouldn't throw an exception when:

• There is no NetworkBehaviour
• There is no NetworkManager
• A shutdown is in progress

Under these conditions, silently exiting without setting anything would be the best option (TBD).
Regarding NetworkList, something similar could be added to prevent this from happening.

Let me mark this for import as it is a regression oriented bug where we were allowing it to proceed under conditions that would lead to an exception in other areas of the code.