feat: disconnect reason

[jeffreyrainy]

Tentative API addition for disconnection reason

[NoelStephensUnity]

Looks good!
Really like your solution to notify the user that they exceeded the maximum allowed message size. Much better than my suggested exception. 💯

[JesseOlmer]

@SamuelBellomo fyi

[SamuelBellomo]

We have a flow in boss room where when the host shuts down gracefully, we'll send a message to all clients telling them "hey! this is a nice shutdown, please give a popup saying the host just left". We need this flow, since else clients will think there was some wifi issues and will do their reconnect coroutine.
I wonder how we'd do this now that we have this new flow. Since shutdown also disconnects clients, could it be possible to set a reason there as well?
Or would users be expected to disconnect all clients with reason before shutting down?
Cause without that message, users would have no way of knowing if this is an elegant shutdown (and that clients just go back to main menu) vs a weird disconnect where the client would make some effort to try to reconnect, not knowing what happened to the host.

[SamuelBellomo]

I guess the solution could still be a custom message (or RPC thinking of it) and still have the "delay one frame to send the RPC so we flush before closing the connection). We would still have this weird "wait one frame" though :(

[jeffreyrainy]

We could add an API to shutdown with a disconnect reason. Internally, it would disconnect all clients with this reason, and then shutdown. This would be exactly equivalent to you disconnecting all clients before shutting down. Please let me know if you are requesting this API.

That said, from there, two things can happen. Either the UTP flushing of its queue survives the shutdown. Or maybe it doesn't. Maybe shutdown overrides the queuing and you lose recent sends. Maybe it guarantees flushing before shutting down. Someone would need to check. In any case, if it doesn't do what is needed you'd have to file a bug with Transport.

[SamuelBellomo]

Pretty sure I don't have the power to request anything of you :P
Since we are adding this new way of doing things, with disconnect reason, my thinking was it'd be great if this was used in most places a disconnect could happen.
We couldn't do it for ALL cases for sure, timeouts and the such wouldn't get the reason message anyway. But it'd make sure disconnect reason is a first class citizen instead of just a patch/something added later?
Besides shutdown, are there other places users can be disconnected?

[SamuelBellomo]

So just tested this real quick in boss room when pointing to your branch and it seems to work (and yep, UTP already allowed to flush, that was fixed a few months ago):

        if (m_DisconnectReason != ConnectStatus.Undefined)
        {
            string reasonString = JsonUtility.ToJson(m_DisconnectReason);
            var connectedClients = NetworkManager.Singleton.ConnectedClients.ToList();
            for (int i = connectedClients.Count - 1; i >= 0; i--)
            {
                var connectedClient = connectedClients[i];
                if (connectedClient.Key != NetworkManager.Singleton.LocalClientId)
                {
                    NetworkManager.Singleton.DisconnectClient(connectedClient.Key, reason: reasonString);
                }
            }
        }

        m_ConnectionManager.NetworkManager.Shutdown()

And then reading that value in OnClientDisconnectCallback. Testing it in game, when I shutdown the host I'll get the usual "host has left" popup on clients. It's a bit cumbersome to write that for loop but not the end of the world. Not the mountain I'll die on :P

[jeffreyrainy]

ok, if UTP flushes even on shutdown, you may convince me to add a Shutdown API that also takes a reason. Let me think about it

[SamuelBellomo]

I think it's particular to your DisconnectClient which does its own sort of flushing. I'd need to check, but if I were to send an RPC right before shutdown, I think it wouldn't work and I'd need to wait a frame before doing the Shutdown. If I remember the conversation from a few months ago correctly, UTP did its flush, but NGO was clearing things before the UTP flush. Your disconnect flushes things before the shutdown clearing (I think, to confirm)