fix: shutdown and shutdown notification issues [MTT-7791] [MTT-7540]

[NoelStephensUnity]

This PR includes several fixes to the NetworkManager shutdown sequence where:

• A client disconnected by a server-host would not receive a notification
• A server-host could shutdown during a relay connection but periodically the transport disconnect message to any connected clients could be dropped.
• A host could disconnect its local client but remain running as a server
• OnClientDisconnectedCallback was not being invoked under certain conditions when disconnected.
• OnClientDisconnectedCallback was not returning the assigned client identifier on the client-side when invoked.
• If a host or server shutdown while a client owned NetworkObjects (other than the player) it would throw an exception.
• If you modify a NetworkVariable or NetworkList during a NetworkManager shutdown an exception was thrown.

MTT-7791
MTT-7540

fix: #2759
fix: #2705
fix: #2699
fix: #2389
fix: #2064
fix: #1880

Changelog

• Fixed: Issue where a client disconnected by a server-host would not receive a local notification.
• Fixed: Issue where a server-host could shutdown during a relay connection but periodically the transport disconnect message sent to any connected clients could be dropped.
• Fixed: Issue where a host could disconnect its local client but remain running as a server.
• Fixed: Issue where OnClientDisconnectedCallback was not being invoked under certain conditions.
• Fixed: Issue where OnClientDisconnectedCallback was always returning 0 as the client identifier.
• Fixed: Issue where if a host or server shutdown while a client owned NetworkObjects (other than the player) it would throw an exception.
• Fixed: Issue where setting values on a NetworkVariable or NetworkList within OnNetworkDespawn during a shutdown sequence would throw an exception.
• Changed: The server or host shutdown so it will now perform a "soft shutdown" when NetworkManager.Shutdown is invoked. This will send a disconnect notification to all connected clients and the server-host will wait for all connected clients to disconnect or timeout after a 5 second period before completing the shutdown process.
• Changed: OnClientDisconnectedCallback will now return the assigned client identifier on the local client side if the client was approved and assigned one prior to being disconnected.

Testing and Documentation

• Includes integration test updates.
• Updates to public documentation: PR-1149

[ShadauxCat]

Any particular reason you're putting all these ifs on their own line instead of using the standard else if?

[NoelStephensUnity]

It is the standard way to do that, but can make it all one line.

[NoelStephensUnity]

I am mistaken... I have Rust in my brain.
"else if" is the correct way (my brain went on vacation Wednesday night)

[ShadauxCat]

Not a critique, just curious why these (and ProcessSendQueues) needed to be moved, just so I can understand the changes you made better.

[NoelStephensUnity]

Ahh... yeah this is because the DisconnectRemoteClient(clientId) invocation below on line 1132 can invoke OnClientDisconnectFromServer which invokes DisconnectRemoteClient:

        internal void DisconnectRemoteClient(ulong clientId)
        {
            MessageManager.ProcessSendQueues();
            OnClientDisconnectFromServer(clientId);
        }

It processes the send queue prior to invoking OnClientDisconnectFromServer(clientId);. However, on line 995 (within OnClientDisconnectFromServer) the ClientDisconnectedMessage is being sent to any remaining connected clients. This means the last client to be disconnected would not get the ClientDisconnectedMessage if a host or server was shutting down.

I didn't want to change the order of operations in DisconnectRemoteClient but wanted to make sure the last ClientDisconnectedMessage was sent out.
(it is sort of a mute point...but I guess I was just in the "make sure any last messages are sent" mode)

Thinking about it... the last client would get the last message... so I probably don't need to do that... and since I am sending disconnect messages prior to even shutting down... that probably doesn't even need to be invoked any longer.

[ShadauxCat]

The wording of this message feels a little passive aggressive to me. "Are you doing X? If so, try not to do X." I feel like the second sentence is implied by the question and can be left off.