fix: Disconnect event notifications #2390

[NoelStephensUnity]

This is a work in progress fix for issue #2390.

The fix is to provide a way for a NetworkTransport derived class implementation can register disconnect event maps to a standardized set of disconnect event types. This PR includes modifications to UnityTransport so it registers its internal disconnect events with the generic ones.

MTTB-1345

Changelog

• Added: Disconnection event notification handling capabilities where NetworkTransport derived custom transports can set the current disconnect event type (NetworkTransport.DisconnectEvents) that, if implemented, will provide more details on why the transport disconnected.
• Added: Protected method NetworkTransport.SetDisconnectEvent that a NetworkTransport derived custom transport can use to provide the disconnect event type that occurred.
• Added: Protected virtual method NetworkTransport.OnGetDisconnectEventMessage that, when overridden, a NetworkTransport derived custom transport can use to provide a customized extended message for each NetworkTransport.DisconnectEvents value.
• Fixed: Issue where the disconnect event and provided message was too generic to know why the disconnect occurred.
• Fixed: Issue where SendTo.NotMe could cause an RPC to be delivered to the sender when connected to a live distributed authority session.
• Changed: UnityTransport now handles setting the current disconnect notification type, via internal UnityTransportNotificationHandler class, while also providing extended informational messages for each disconnect event type.

Testing and Documentation

• No tests have been added.
• Includes unit tests.
• Includes integration tests (wip)
• No new documentation required.
  • When we create documentation on creating your own custom NetworkTransport, this should be included.

Backport

TBD if we want to bring all of this into v1.x.

[simon-lemay-unity]

My initial reaction to this is that it seems complex for what it is.

Why couldn't we just impose on the NetworkTransport to use NGO's enum when setting a disconnect reason? That is, only add SetDisconnectEvent(DisconnectEvents disconnectEvent, string message = null) to the NetworkTransport API. Every transport can then do its own mapping internally in the way it makes the most sense for it (possibly in a more efficient way than by maintaining a dictionary on the side just for that).

I'm not sure I see the point of having all that mapping machinery in NetworkTransport. Plus, it kinda bakes the idea that a transport's disconnect reason will be a byte (or even just a numerical value) directly in the API. That's true for UnityTransport, but might not be the case for other transports. Say if a transport has their backend returning strings explaining the disconnect reason, then all of that mapping machinery is basically useless for them. They'll end up doing their own mapping of strings to disconnect events directly in their transport.

[NoelStephensUnity]

My initial reaction to this is that it seems complex for what it is.

Why couldn't we just impose on the NetworkTransport to use NGO's enum when setting a disconnect reason? That is, only add SetDisconnectEvent(DisconnectEvents disconnectEvent, string message = null) to the NetworkTransport API. Every transport can then do its own mapping internally in the way it makes the most sense for it (possibly in a more efficient way than by maintaining a dictionary on the side just for that).

I'm not sure I see the point of having all that mapping machinery in NetworkTransport. Plus, it kinda bakes the idea that a transport's disconnect reason will be a byte (or even just a numerical value) directly in the API. That's true for UnityTransport, but might not be the case for other transports. Say if a transport has their backend returning strings explaining the disconnect reason, then all of that mapping machinery is basically useless for them. They'll end up doing their own mapping of strings to disconnect events directly in their transport.

The approach you describe above I actually started with, but then I realized that I would have to decorate all places that implement the feature/update with UTP_TRANSPORT_2_4_ABOVE. Which would mean any custom NetworkTransport would have no way of having messages handled without including com.unity.transport. The higher abstraction ensures that any NetworkTransport derived class can provide a map to the "universal disconnect events" based on the values the transport in question provides.

Otherwise, users would always have to:

• Include com.unity.transport
• Replicate a chunk of the "mapping" code in this PR to map the transport they are adding to the UTP disconnect events.

This way, it keeps the notification "transport neutral" while providing some mechanism to register a transport's disconnect events to the NGO (standardized) disconnect events.

Does this make more sense?

[simon-lemay-unity]

Does this make more sense?

Not really. Maybe I'm missing something obvious, but I don't see how UTP would be required.

To be clear, my suggestion is not for the disconnection events in NetworkTransport to be UTP's. I'm proposing that NGO has its own enum for disconnection events (like it does in this PR), but that the only API in NetworkTransport be to signal those NGO disconnection events, and not provide any mapping machinery. If there's a need for a mapping between those NGO events and the transport's, then that mapping should be in the transport itself.

Basically my proposal would be:

abstract class NetworkTransport
{
    public enum DisconnectEvents
    {
        ...
    }

    public DisconnectEvents DisconnectEvent { get; protected set; }
    public string DisconnectEventMessage { get; protected set; }
}

public class UnityTransport
{
    ...
    case TransportEvent.Disconnect:
        ...
        switch (reader.ReadByte())
        {
            case (byte)Error.Timeout:
                DisconnectEvent = DisconnectEvents.ProtocolTimeout;
                DisconnectEventMessage = "Connection has timed out.";
            ...
        }
}

And while I get the argument that we don't want every transport to have to implement mapping logic, I don't agree with it. The mapping logic will normally be simple enough that I don't believe it's worth abstracting away in NetworkTransport. In the case of UnityTransport, it can be as simple as a switch statement. And it's not like the switch statement is more verbose or anything; the current proposal has basically the same amount of code required to set up the mappings through the generic mechanism.

[NoelStephensUnity]

the same amount of code required to set up the mappings through the generic mechanism.

Yeah...you have a good point about the amount of code and I wasn't 100% happy with my initial approach anyway.... I will tinker around with it and see if we can keep some of the non-event type related script while approaching it the way you described above.

[simon-lemay-unity]

Regarding the new design:

• I'm not sure I understand the point of OnGetDisconnectEventMessage. If a transport is interested in providing a custom message to go with their error, couldn't they just pass it to SetDisconnectEvent? Plus it's a bit weird to have a OnGetDisconnectEventMessage function when there is no GetDisconnectEventMessage.
• What is the purpose of UnityTransportNotificationHandler and why does it need to be a separate class that handles dynamic mappings through dictionaries? Maybe I'm missing something obvious, but couldn't it be just one big switch case at the point where we want to set the disconnection event (when we read the disconnection reason byte)?

[EmandM]

Suggested change

	private void GenerateDisconnectInformation(ulong clientId, ulong transportClientId, string reason = null)
	private void GenerateDisconnectInformation(ulong clientId, string reason = null)

It looks like transportClientId isn't being used here

[EmandM]

Seeing as we can't reproduce this error I think it's maybe best to remove the changes in this file 🙃

[EmandM]

The AddDisconnectEventMap function is defined in UnityTransport rather than here in NetworkTransport. From this code doc it looks like you've intended for it to live in NetworkTransport?