Skip to content

feat: disconnect reason #2280

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.

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

Merged
merged 37 commits into from
Nov 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
f0790d2
disconnect reason, wip
jeffreyrainy Oct 26, 2022
8ef9969
adding disconnect reason as a property to NetworkManager
jeffreyrainy Oct 27, 2022
7b7b5cf
feat: providing a disconnect reason
jeffreyrainy Oct 27, 2022
3c67ce9
removing unwanted changes in PR
jeffreyrainy Oct 27, 2022
4c4335f
Merge branch 'develop' into feat/disconnect-reason
jeffreyrainy Oct 27, 2022
37055ec
style: standards
jeffreyrainy Oct 27, 2022
25b812f
Adding reason in DisconnectClient, checking for null reason
jeffreyrainy Oct 28, 2022
55f0ec5
add summary xmldoc for ConnectionApprovalResponse.Reason
jeffreyrainy Oct 28, 2022
98eaeb0
Keeping API compatibility
jeffreyrainy Oct 28, 2022
99358bf
fix: default parameter hiding
jeffreyrainy Oct 28, 2022
0168a14
Merge branch 'develop' into feat/disconnect-reason
jeffreyrainy Oct 29, 2022
1bea4ae
WIP, adding test for disconnection reason
jeffreyrainy Oct 31, 2022
8dc1c2b
Merge branch 'feat/disconnect-reason' of github.com:Unity-Technologie…
jeffreyrainy Oct 31, 2022
d683e98
fixed typo
jeffreyrainy Oct 31, 2022
80b278d
Refactor, moving the disconnect reason SendMessage higher in the call…
jeffreyrainy Oct 31, 2022
a9767dd
test improvements
jeffreyrainy Oct 31, 2022
35f9279
final adjustments
jeffreyrainy Oct 31, 2022
7ea9d6b
final adjustments
jeffreyrainy Oct 31, 2022
890c8f6
Merge branch 'feat/disconnect-reason' of github.com:Unity-Technologie…
jeffreyrainy Oct 31, 2022
3471527
adding line removed mistakenly
jeffreyrainy Oct 31, 2022
3ea4a4f
style: coding standards
jeffreyrainy Oct 31, 2022
9efaee1
some PR review comments
jeffreyrainy Nov 1, 2022
a6d8796
better handling of oversized reasons
jeffreyrainy Nov 1, 2022
6047dda
Addressing some more PR review comments
jeffreyrainy Nov 1, 2022
1203c8a
Merge branch 'develop' into feat/disconnect-reason
jeffreyrainy Nov 1, 2022
496dfce
Read/WriteValueSafe instead of duplicating the size
jeffreyrainy Nov 2, 2022
e4494a8
Merge branch 'develop' into feat/disconnect-reason
jeffreyrainy Nov 2, 2022
f113e06
more PR review comments
jeffreyrainy Nov 2, 2022
6739f30
Merge branch 'develop' into feat/disconnect-reason
jeffreyrainy Nov 2, 2022
8127eeb
Changelog
jeffreyrainy Nov 2, 2022
7b06f84
moving test into /messaging
jeffreyrainy Nov 2, 2022
35d9315
style: netcode.standards
jeffreyrainy Nov 2, 2022
f622388
Merge develop into feat/disconnect-reason
netcode-ci-service Nov 3, 2022
3030a05
Testing approval disconnection reason
jeffreyrainy Nov 3, 2022
dc10881
Merge branch 'feat/disconnect-reason' of github.com:Unity-Technologie…
jeffreyrainy Nov 3, 2022
b14cc83
Merge develop into feat/disconnect-reason
netcode-ci-service Nov 3, 2022
0a52c7b
Merge develop into feat/disconnect-reason
netcode-ci-service Nov 4, 2022
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
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Added
- Added `NetworkObject` auto-add helper and Multiplayer Tools install reminder settings to Project Settings. (#2285)
- Added `public string DisconnectReason` getter to `NetworkManager` and `string Reason` to `ConnectionApprovalResponse`. Allows connection approval to communicate back a reason. Also added `public void DisconnectClient(ulong clientId, string reason)` allowing setting a disconnection reason, when explicitly disconnecting a client.

### Fixed

Expand Down
40 changes: 40 additions & 0 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ public NetworkPrefabHandler PrefabHandler
private bool m_ShuttingDown;
private bool m_StopProcessingMessages;

// <summary>
// When disconnected from the server, the server may send a reason. If a reason was sent, this property will
// tell client code what the reason was. It should be queried after the OnClientDisconnectCallback is called
// </summary>
public string DisconnectReason { get; internal set; }

private class NetworkManagerHooks : INetworkHooks
{
private NetworkManager m_NetworkManager;
Expand Down Expand Up @@ -443,6 +449,11 @@ public class ConnectionApprovalResponse
/// If the Approval decision cannot be made immediately, the client code can set Pending to true, keep a reference to the ConnectionApprovalResponse object and write to it later. Client code must exercise care to setting all the members to the value it wants before marking Pending to false, to indicate completion. If the field is set as Pending = true, we'll monitor the object until it gets set to not pending anymore and use the parameters then.
/// </summary>
public bool Pending;

// <summary>
// Optional reason. If Approved is false, this reason will be sent to the client so they know why they
// were not approved.
public string Reason;
}

/// <summary>
Expand Down Expand Up @@ -889,6 +900,7 @@ private void Initialize(bool server)
return;
}

DisconnectReason = string.Empty;
IsApproved = false;

ComponentFactory.SetDefaults();
Expand Down Expand Up @@ -2004,12 +2016,31 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> payload, flo
/// </summary>
/// <param name="clientId">The ClientId to disconnect</param>
public void DisconnectClient(ulong clientId)
{
DisconnectClient(clientId, null);
}

/// <summary>
/// Disconnects the remote client.
/// </summary>
/// <param name="clientId">The ClientId to disconnect</param>
/// <param name="reason">Disconnection reason. If set, client will receive a DisconnectReasonMessage and have the
/// reason available in the NetworkManager.DisconnectReason property</param>
public void DisconnectClient(ulong clientId, string reason)
Copy link
Contributor

@SamuelBellomo SamuelBellomo Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
if (!IsServer)
{
throw new NotServerException($"Only server can disconnect remote clients. Please use `{nameof(Shutdown)}()` instead.");
}

if (!string.IsNullOrEmpty(reason))
{
var disconnectReason = new DisconnectReasonMessage();
disconnectReason.Reason = reason;
SendMessage(ref disconnectReason, NetworkDelivery.Reliable, clientId);
}
MessagingSystem.ProcessSendQueues();

OnClientDisconnectFromServer(clientId);
DisconnectRemoteClient(clientId);
}
Expand Down Expand Up @@ -2243,6 +2274,15 @@ internal void HandleConnectionApproval(ulong ownerClientId, ConnectionApprovalRe
}
else
{
if (!string.IsNullOrEmpty(response.Reason))
{
var disconnectReason = new DisconnectReasonMessage();
disconnectReason.Reason = response.Reason;
SendMessage(ref disconnectReason, NetworkDelivery.Reliable, ownerClientId);

MessagingSystem.ProcessSendQueues();
}

PendingClients.Remove(ownerClientId);
DisconnectRemoteClient(ownerClientId);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
namespace Unity.Netcode
{
internal struct DisconnectReasonMessage : INetworkMessage
{
public string Reason;

public void Serialize(FastBufferWriter writer)
{
string reasonSent = Reason;
if (reasonSent == null)
{
reasonSent = string.Empty;
}

if (writer.TryBeginWrite(FastBufferWriter.GetWriteSize(reasonSent)))
{
writer.WriteValueSafe(reasonSent);
}
else
{
writer.WriteValueSafe(string.Empty);
NetworkLog.LogWarning(
"Disconnect reason didn't fit. Disconnected without sending a reason. Consider shortening the reason string.");
}
}

public bool Deserialize(FastBufferReader reader, ref NetworkContext context)
{
reader.ReadValueSafe(out Reason);
return true;
}

public void Handle(ref NetworkContext context)
{
((NetworkManager)context.SystemOwner).DisconnectReason = Reason;
}
};
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using NUnit.Framework;
using Unity.Collections;

namespace Unity.Netcode.EditorTests
{
public class DisconnectMessageTests
{
[Test]
public void EmptyDisconnectReason()
{
var networkContext = new NetworkContext();
var writer = new FastBufferWriter(20, Allocator.Temp, 20);
var msg = new DisconnectReasonMessage();
msg.Reason = string.Empty;
msg.Serialize(writer);

var fbr = new FastBufferReader(writer, Allocator.Temp);
var recvMsg = new DisconnectReasonMessage();
recvMsg.Deserialize(fbr, ref networkContext);

Assert.IsEmpty(recvMsg.Reason);
}

[Test]
public void DisconnectReason()
{
var networkContext = new NetworkContext();
var writer = new FastBufferWriter(20, Allocator.Temp, 20);
var msg = new DisconnectReasonMessage();
msg.Reason = "Foo";
msg.Serialize(writer);

var fbr = new FastBufferReader(writer, Allocator.Temp);
var recvMsg = new DisconnectReasonMessage();
recvMsg.Deserialize(fbr, ref networkContext);

Assert.AreEqual("Foo", recvMsg.Reason);
}

[Test]
public void DisconnectReasonTooLong()
{
var networkContext = new NetworkContext();
var writer = new FastBufferWriter(20, Allocator.Temp, 20);
var msg = new DisconnectReasonMessage();
msg.Reason = "ThisStringIsWayLongerThanTwentyBytes";
msg.Serialize(writer);

var fbr = new FastBufferReader(writer, Allocator.Temp);
var recvMsg = new DisconnectReasonMessage();
recvMsg.Deserialize(fbr, ref networkContext);

Assert.IsEmpty(recvMsg.Reason);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections;
using System.Text.RegularExpressions;
using NUnit.Framework;
using UnityEngine;
using UnityEngine.TestTools;
using Unity.Netcode.TestHelpers.Runtime;
Expand All @@ -25,16 +26,48 @@ protected override void OnServerAndClientsCreated()
}

private int m_DisconnectCount;
private bool m_ThrowOnDisconnect = false;

public void OnClientDisconnectCallback(ulong clientId)
{
m_DisconnectCount++;
throw new SystemException("whatever");
if (m_ThrowOnDisconnect)
{
throw new SystemException("whatever");
}
}

[UnityTest]
public IEnumerator DisconnectReasonTest()
{
float startTime = Time.realtimeSinceStartup;
m_ThrowOnDisconnect = false;
m_DisconnectCount = 0;

// Add a callback for both clients, when they get disconnected
m_ClientNetworkManagers[0].OnClientDisconnectCallback += OnClientDisconnectCallback;
m_ClientNetworkManagers[1].OnClientDisconnectCallback += OnClientDisconnectCallback;

// Disconnect both clients, from the server
m_ServerNetworkManager.DisconnectClient(m_ClientNetworkManagers[0].LocalClientId, "Bogus reason 1");
m_ServerNetworkManager.DisconnectClient(m_ClientNetworkManagers[1].LocalClientId, "Bogus reason 2");

while (m_DisconnectCount < 2 && Time.realtimeSinceStartup < startTime + 10.0f)
{
yield return null;
}

Assert.AreEqual(m_ClientNetworkManagers[0].DisconnectReason, "Bogus reason 1");
Assert.AreEqual(m_ClientNetworkManagers[1].DisconnectReason, "Bogus reason 2");

Debug.Assert(m_DisconnectCount == 2);
}

[UnityTest]
public IEnumerator DisconnectExceptionTest()
{
m_ThrowOnDisconnect = true;
m_DisconnectCount = 0;
float startTime = Time.realtimeSinceStartup;

// Add a callback for first client, when they get disconnected
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,11 @@ private IEnumerator ConnectionApprovalHandler(int numClients, int failureTestCou
}
}

foreach (var c in clientsToClean)
{
Assert.AreEqual(c.DisconnectReason, "Some valid reason");
}

foreach (var client in clients)
{
// If a client failed, then it will already be shutdown
Expand Down Expand Up @@ -228,6 +233,14 @@ private void ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest
response.Rotation = null;
response.PlayerPrefabHash = m_PrefabOverrideGlobalObjectIdHash;
}
if (!response.Approved)
{
response.Reason = "Some valid reason";
}
else
{
response.Reason = string.Empty;
}
}


Expand Down