Skip to content

fix: Disconnect event notifications #2390 #3551

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

Draft
wants to merge 13 commits into
base: develop-2.0.0
Choose a base branch
from

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Jul 17, 2025

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.

Documentation

  • No new documentation required.
    • When we create documentation on creating your own custom NetworkTransport, this should be included.

Testing & QA

Functional Testing

Manual testing :

  • Manual testing done
    • Asteroids manual test (PR pending)

Automated tests:

  • Covered by existing automated tests
  • Covered by new automated tests

Does the change require QA team to:

  • Review automated tests?
  • Execute manual tests?

If any boxes above are checked, please add QA as a PR reviewer.

Backport

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

This update is a work in progress resolution to the issue where disconnect event notifications were too vague and did not provide any way to know what the cause of the disconnect was.
Adding some final adjustments and (hopefully) improving the disconnect event messaging upon a local client or server shutting down.
When using NotMeRpc, make sure we only send to connected clients and not the service to avoid infinite spamming of the RPC.
Adding additional comments and disconnect event specific messages for UnityTransport.
Missed a parameter in the XML-API
@simon-lemay-unity
Copy link
Contributor

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.

Fixing some minor test issues after the disconnect notification updates.
@NoelStephensUnity
Copy link
Collaborator Author

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?

Updating the multi-client test.
@simon-lemay-unity
Copy link
Contributor

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
Copy link
Collaborator Author

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.

Making adjustments based on peer review of this branch's PR.
Adding changelog entries.
Trying a possible new "tag" unreleased format.
Trying out just marking it with a temporary release date.
@simon-lemay-unity
Copy link
Contributor

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)?

@@ -520,6 +524,27 @@ internal void DataEventHandler(ulong transportClientId, ref ArraySegment<byte> p
#endif
}

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

Choose a reason for hiding this comment

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

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

// We only add when there is a "DAHost" by
// - excluding the server id when using client-server (i.e. !m_NetworkManager.DistributedAuthorityMode )
// - excluding if connected to the CMB backend service (i.e. we don't want to send to service as it will broadcast it back)
if (clientId == NetworkManager.ServerClientId && (!m_NetworkManager.DistributedAuthorityMode || m_NetworkManager.CMBServiceConnection))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

/// The Netcode for GameObjects standardized disconnection event types.
/// </summary>
/// <remarks>
/// <see cref="AddDisconnectEventMap"/> provides you with the ability to register the transport's disconnect event types with the local equivalent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

NoelStephensUnity added a commit that referenced this pull request Aug 14, 2025
## Depends on
This PR depends upon the fix for the `SendTo.NotMe` fix in #3551.

## AttachableBehaviour and Support Components

The purpose of this PR (feat) is to address the complexity of "picking
up" or "dropping" an item in the world which can become complex when
using the traditional `NetworkObject` parenting pattern. In this PR
there are three primary components added to help reduce this complexity:

- `AttachableBehaviour`: Provides "out of the box" support for attaching
(i.e. parenting) a nested child `GameObject` that includes an
`AttachableBehaviour` component to another nested child `GameObject`
with an `AttachableNode` component that is associated with a different
`NetworkObject`.
- `AttachableNode`: This component is required by the
`AttachableBehaviour` component in order to be able to attach (i.e.
parent) to another GameObject without having to parent the entire
`NetworkObject` component the `AttachableBehaviour` component is
associated with.
- `ComponentController`: This component provides users the ability to
synchronize the enabling or disabling of any `Object` derived component
that has an `enabled` property.

_This PR also incorporates a new "Helpers" subfolder under the NGO
components folder where additional helper components will live._

## Documentation Update

## New Documentation

[AttachableBehaviour](https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/blob/feat/attachable-networkbehaviour-and-object-controller/com.unity.netcode.gameobjects/Documentation~/components/Helpers/attachablebehaviour.md)

### Network Components Section Update
<img width="957" height="589" alt="image"
src="https://github.com/user-attachments/assets/48e9edaf-cf8d-4a61-b0b9-c0369bb8f26f"
/>

### New Foundational Components Section
<img width="1145" height="831" alt="image"
src="https://github.com/user-attachments/assets/f3dc71b4-cd07-4884-a5a6-fc7c78e9e24c"
/>

### New Helper Components Section
<img width="1015" height="537" alt="image"
src="https://github.com/user-attachments/assets/407636b8-9085-4b80-8bb4-d55b18512ce1"
/>

## NetworkBehaviour.OnNetworkPreDespawn

Added another virtual method to `NetworkBehaviour`,
`OnNetworkPreDespawn`, that is invoked before running through the
despawn sequence for the `NetworkObject` and all `NetworkBehaviour`
children of the `NetworkObject` being despawned. This provides an
opportunity to do any kind of cleanup up or last micro-second state
updates prior to despawning.

<!-- Add short version of the JIRA ticket to the PR title (e.g. "feat:
new shiny feature [MTT-123]") -->

## Changelog

- Added: `AttachableBehaviour` helper component to provide an alternate
approach to parenting items without using the `NetworkObject` parenting.
- Added : `AttachableNode` helper component that is used by
`AttachableBehaviour` as the target node for parenting.
- Added: `ComponentController` helper component that can be used to
synchronize the enabling and disabling of components and can be used in
conjunction with `AttachableBehaviour`.
- Added: `NetworkBehaviour.OnNetworkPreDespawn` that is invoked before
running through the despawn sequence for the `NetworkObject` and all
`NetworkBehaviour` children of the `NetworkObject` being despawned.


<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

## Documentation
[//]: # (
This section is REQUIRED and should mention what documentation changes
were following the changes in this PR.
We should always evaluate if the changes in this PR require any
documentation changes.
)

- Includes documentation updates:
  - Added new documentation section.
  - Refactored network components section.
  - Fixed some image links.


## Testing & QA
[//]: #  (
This section is REQUIRED and should describe how the changes were tested
and how should they be tested when Playtesting for the release.
It can range from "edge case covered by unit tests" to "manual testing
required and new sample was added".
Expectation is that PR creator does some manual testing and provides a
summary of it here.)

### Functional Testing
[//]: # (If checked, List manual tests that have been performed.)
_Manual testing :_
- [X] `Manual testing done`
- Asteroids `AttachableBehaviour` manual test
[PR-42](https://github.cds.internal.unity3d.com/unity/Asteroids-CMB-NGO-Sample/pull/42)

_Automated tests:_
- [ ] `Covered by existing automated tests`
- [X] `Covered by new automated tests`

_Does the change require QA team to:_

- [ ] `Review automated tests`?
- [ ] `Execute manual tests`?

If any boxes above are checked, please add QA as a PR reviewer.


## Backport

This is a v2.x only feature.

<!-- If this is a backport:
 - Add the following to the PR title: "\[Backport\] ..." .
 - Link to the original PR.
If this needs a backport - state this here
If a backport is not needed please provide the reason why.
If the "Backports" section is not present it will lead to a CI test
failure.
-->

---------

Co-authored-by: Emma <[email protected]>
Co-authored-by: Amy Reeve <[email protected]>
Co-authored-by: Unity Netcode CI <[email protected]>
NoelStephensUnity added a commit that referenced this pull request Aug 15, 2025
## Purpose of this PR

The regex we used to validate hostnames did not accept single words as
valid hostnames. But single words _can_ be valid hostnames. The most
common is of course "localhost" but one can edit one's hosts file to
define any word as something the local resolver will resolve to an IP
address.

This PR addresses this by removing any validation of the provided
hostname. We could modify the validity check to accept single words too,
but the regex is pretty indigestible already and it's simpler to just
let the resolver fail if the user provided garbage. UTP will signal this
through a disconnection event with an appropriate reason (which we'll be
able to map to a nice error message once [this
PR](#3551)
lands).

While I was at it, I also made a few cleanups and improvements:

- Removed the `UTP_TRANSPORT_2_4_ABOVE` define. NGO depends on UTP 2.4.0
so it can be safely assumed that users will have it installed. No need
to conditionally compile the code that depends on it.
- Added a test that establishes a connection using hostname resolution.
- Simplified the logic around connections and avoid bypassing the
`Connect` method when connecting to a hostname.
- Deprecated `ConnectionAddressData.ServerEndPoint`. We don't use it
anymore, it doesn't work with hostnames, and it's not providing any
value over just calling `NetworkEndpoint.Parse`. And worst of all: its
capitalization of "endpoint" doesn't match what we use elsewhere.
- Modified the listen address logic so that if a domain name is used, by
default if remote connections are allowed we will listen on :: (the IPv6
"any" address) instead of 0.0.0.0. This can still be overridden using
`SetConnectionData`. The reason for this change is that the resolver in
UTP prioritizes IPv6 addresses over IPv4. So if we listen on IPv4 by
default, we're likely to get issues if the resolver then ends up with an
IPv6 address. In current versions of UTP for instance, this causes
errors on Windows (a fix is on the way). I'm looking into changing the
behavior of the resolver to prefer IPv4, but that's an engine change so
might take a while to land. In the meantime defaulting to IPv6 seems
like the best approach.

### Changelog

- Fixed: Fixed an issue where `UnityTransport` would not accept single
words as valid hostnames (notably "localhost").
- Changed: Marked `UnityTransport.ConnectionAddressData.ServerEndPoint`
as obsolete. It can't work when using hostnames as the server address,
and its functionality can easily be replicated using
`NetworkEndpoint.Parse`.

## Documentation

No documentation changes or additions were necessary.

## Testing & QA

Tested with manual and automated tests.

## Backport

Hostname resolution is only supported in UTP 2.4+ and Unity 6.1+, so no
backport necessary.

---------

Co-authored-by: Michał Chrobot <[email protected]>
Co-authored-by: Noel Stephens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants