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.