OnClientDisconnectCallback not triggered if server disconnects client

[daalen]

Description

When the server disconnects a client through NetworkManager.Singleton.DisconnectClient([clientId]), OnClientDisconnectCallback is not triggered. It is triggered when the client disconnects itself.

Reproduce Steps

1. Subscribe to NetworkManager.Singleton.OnClientDisconnectCallback
2. On the server, call NetworkManager.Singleton.DisconnectClient([clientId]) with [clientId] any client that is not the server

Actual Outcome

The function subscribed to OnClientDisconnectCallback is not called.

Expected Outcome

The function subscribed to OnClientDisconnectCallback is always called, regardless of how the client got disconnected.

Environment

• OS: macOS Big Sur
• Unity Version: 2022.1.8f1
• Netcode Version: 1.0.0

Additional Context

Possibly related to #795

[NoelStephensUnity]

We possibly need to improve the documentation on this method. The intended usage of OnClientDisconnectedCallback is:

• server-side: to notify the server that a client disconnected itself (i.e. a client/player exits a network session)
• client-side: to be notified that the server disconnected it (i.e. server sends a disconnect or terminates the transport's connection)

The original logic behind this was:

• If a server is disconnecting a client from user-side script/code, then the server already knows the client is disconnected.
• If a client disconnects itself then it knows that it is "disconnected".

However, there are discussions about further improving the connect disconnect process and we are definitely looking at several possibilities that would provide some form of event even if it is the local system disconnecting itself and/or shutting itself down.

Until we determine the better approach, if you could further describe your scenario where on the server-side you have script that disconnects the client but then in some other server-side script it requires the OnClientDisconnectedCallback to be triggered in order to (insert what that code does here). I might be able to provide a (hopefully) simple work around.

[daalen]

OK, if that's the intended use that's fine, though indeed the documentation could be clearer. I was using this callback for cleanup and to have the client return itself to the login scene, but it was easy enough to work around.

[TheCaveOfWonders]

Just ran through the same issue.

My 2 cents, if the current behavior stays, the name of this event needs to change, because it is simply not accurate, changing the documentation alone will still lead to confusion.

When we encounter a clear and simple name, we often don't bother digging deeper into the documentation, this is a perfect example of that, the name "OnClientDisconnectedCallback" is very simple, clear and makes the intention obvious, yet the actual behavior is not reflective of the name; this event is not always raised as the name describes,

[NoelStephensUnity]

@TheCaveOfWonders
I completely understand.
We are continuing to review over areas like this. There are two ways to think about this:

1. The OnClientDisconnectedCallback is a notification of a network disconnect event that was initiated remotely.
2. The OnClientDisconnectedCallback is a notification that a network session of a client, both local and remote, has ended.

Under the first line of thinking, this would always be "remote relative" and not specific to the instance running:

• A host/server would be notified of any client that initiated the disconnect event.
• A client would be notified if it was disconnected by the server/host.
  This line of thinking keeps the event tied to things the local player did not initiate themselves.

Under the second line of thinking, this would be relative to the local and remote instances:

• A host/server would be notified of any client that initiated the disconnect event (local/remote)
  • As a host it would be notified when the local "host client" was disconnected, but in reality the only time that would happen is if the host is shutting down the NetworkManager.
• A client would be notified if the local player exits the session or if the server disconnected the client.
  • Distinguishing between being disconnected from the server or the local player exiting the session I believe was the original reason behind the way things are currently.

I believe the correct path (at least my personal opinion) is to improve how NGO delivers notifications. Notifications for a client being disconnected should include additional information to distinguish between a local disconnection and a remote disconnection. Currently, it only delivers a client identifier that is no longer valid when a local player disconnects itself.

The tough part with SDK development is determining when to make a change to an API that could require users to make changes on their end (i.e. a "breaking change") even if it will improve a weak spot/flaw in the original implementation.

The more feedback we get for issues of this nature the more we can make clear justifications for temporary adjustments that might deviate from the original intention but will reduce the user pain associated with it.

So, with that all said... 😸

Let me review over that area of code and see if I can whip up a quick PR that would lean towards the second line of thinking.

[Mercury-Leo]

I was about to open a bug report on this but found it here.

I encountered the same problem. I saw the OnClientDisconnectCallback and was sure this would be called when the client is disconnected. When NetworkManager.Singleton.DisconnectClient is called, I expected this to also invoke the callback.
This led for me bug hunting for quite awhile until I found that DisconnectClient doesn't invoke the callback.

I can see the reason behind, if I called Disconnect I already know that the client is disconnecting so I won't have to use the callback, but in general it makes sense that when a client disconnects it will also invoke the ClientDisconnect callback.

[TheCaveOfWonders]

@NoelStephensUnity
I'm on version 1.5.2
Was this recently changed? OnClientDisconnectCallback is now triggered on the server even if it's the server that kicked the client. Whereas before if the server kicks a client, OnClientDisconnectCallback would not trigger on the server.

Basically this used to not get called on the side that initiated the disconnect.

[LordDarkenon]

I'm getting some behaviour that isn't as described in the NGO 1.6 documentation regarding OnClientDisconnect callback events. The documentation states:-

"When disconnect notifications are triggered:

• Clients are notified when they're disconnected by the server.
• The server is notified if the client side disconnects (that is, a player intentionally exits a game session)
• Both the server and clients are notified when their network connection is unexpectedly disconnected (network interruption)"

I'm experiencing something different. So for example if I have a Host (clientId==0) and 2 clients connected (clientId==1 & clientId==2), then if the Host shuts itself down (using NetworkManager.Singleton.Shutdown()) then I'm seeing the following:-

1. The OnClientDisconnect event is triggered on the Host for the 2 clients (1&2),
2. I'm also seeing the OnClientDisconnect event being triggered on Client 1 for Client 0 disconnecting, and on Client 2 for Client 0 disconnecting.

This isn't the behaviour in the documentation.

According to the documentation, if I'm reading it correctly, the OnClientDisconnect event should be called on Client 1 for Client 1 disconnecting, on Client 2 for Client 2 disconnecting (Bullet 1 - since these disconnections were triggered by the host shutting itself down). The server/host shouldn't be notified (Bullet 2 - since the server/host initiated the disconnect). And Bullet 3 shouldn't apply since it's not a network interruption.

So either the documentation is incorrect, or there is something else going on ..

[NoelStephensUnity]

@LordDarkenon
There is indeed a bug here and I think we can make this less confusing.
What should be happening is:

• Host Shutdown:
  • Clients should get an OnClientDisconnectCallback event with the NetworkManager.ServerClientId signaling the disconnect came from the server (i.e. it is relative). This will also happen if there is a transport failure.
  • Host should get the OnClientDisconnectCallback event with the NetworkManager.ServerClientId signaling the server (itself) disconnected.
• Client Shutdown:
  • Local client should get the OnClientDisconnectCallback event with the NetworkManager.LocalClientId signaling the local client terminated the connection.
  • Host should get the OnClientDisconnectCallback event with the client id that disconnected.

However, it looks like the NetworkTransport.OnTransportEvent is being removed prior to the NetworkConnectionManager shutting down...which prevents the local message from getting triggered by the transport which is shutdown within the NetworkConnectionManager when it is shutdown.

            if (NetworkConfig != null && NetworkConfig.NetworkTransport != null)
            {
                NetworkConfig.NetworkTransport.OnTransportEvent -= ConnectionManager.HandleNetworkEvent;
            }

I have upgraded this to a bug and we will address the issue.
👍

[st-matskevich]

Unity 2022.3.17f1
NGO 1.8.0
Windows 10

If a client in 'client' mode disconnects - OnClientDisconnected is triggered with clientNetworkID equal to LocalClientId
If a client in 'host' mode disconnects - OnClientDisconnected is not triggered
Same behavior with listening to OnConnectionEvent

In both cases, disconnect is triggered by calling NetworkManager.Singleton.Shutdown()

Should I provide a sample scene? Could this issue be reopened?
@NoelStephensUnity

[NoelStephensUnity]

Well,
Technically if you shutdown the host-server it is understood that the host-client will no longer be connected... but I do see that it does not invoke for the host when the host shuts itself down.

I will add this into the next update, but for the time being you can always subscribe to the NetworkManager.OnClientStopped event to be notified when any client is completely disconnected and stopped. This does tigger on the host side when you shut it down.

Good catch and low hanging fruit to fix.

[NoelStephensUnity]

#2822 will be in the next update.
👍

[LordDarkenon]

So for fix #2822 how will that work? If you shut down the Host-Server will OnClientDisconnected just be triggered on the Host-Client? Will OnClientDisconnected be triggered or not on other clients informing them that client0 disconnected? Just trying to anticipate the impact of this on my code, since by changing this you will impact existing code that since NGO 1.8.0 now doesn't expect OnClientDisconnected to be triggered on the Host-Client or other connected clients when the Host-Server (client 0) disconnects.

In fact NGO 1.8.0 implemented a breaking change from NGO 1.7.1. In 1.7.1 if the host/server disconnected, then the clients got a disconnect message that client 0 disconnected. With NGO 1.8.0 the clients no longer get a message that client 0 disconnected if the host/server disconnects. This threw me when I updated to NGO 1.8.0, and after some debugging I realised what had changed, and had to go through my code and refactor it to take this into account. So I'd rather that the clients don't get a disconnect message if the host/server disconnects for #2822!

[LordDarkenon]

Also the documentation for NGO 1.8.0 appears to be incorrect regarding Client DisconnectionNotifications (https://docs-multiplayer.unity3d.com/netcode/current/components/networkmanager/#client-disconnection-notifications).

It states that :-
"Scenarios where the disconnect notification won't be triggered:

When a server "logically" disconnects a client.
Reason: The server already knows the client is disconnected.

When a client "logically" disconnects itself.
Reason: The client already knows that it's disconnected."

In fact disconnection notifications are generated for both those situations - which I think is correct and logical.

For example, if the server/host shuts down client 1 using NetworkManager.Singleton.DisconnectClient(1), both the server/host and client 1 get a disconnect message that clientId = 1 has disconnected.

Also, for example, if client 1 shuts down (using NetworkManager.Singleton.ShutDown()), both the server/host and client 1 get a disconnect message that clientId = 1 has disconnected.

As I said, I think this is logical and correct and it's the documentation that is wrong (and needs to be corrected because it's just confusing).