Skip to content

fix: Corrected wrong clientId on receive by keeping SDK internal mapping of client ID to transport ID so we can ensure no overlap #1368

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 6 commits into from
Oct 28, 2021

Conversation

ShadauxCat
Copy link
Collaborator

With both UNet and UTP, the following flow happens:

  • Client initiates a connection to the server
    • Transport opens a socket. Socket gets ID A
  • Server accepts connection from client
    • Transport opens a socket. Socket gets the same ID as the socket opened on the client, because like client, this is the first socket that server has opened and it just happens that way
  • Server tells client "your ID is A" because that's the server's view of the client ID
  • On the client, the server ID is also A.

So because LocalClientId and ServerClientId happen to have the same value because the server is giving the client an ID from a separate process's ID space, any checks on senderId == LocalClientId will return true... but senderId == ServerClientId will also return true...

...except in the case of UNet, where it uses a sentinel value of 0 to indicate the server client ID... but when receiving a message, the ID it passes is the actual ID instead of the sentinel value, which makes the problem worse for UNet because senderId == ServerClientId will return false... even though it actually is the server's client ID and if the client were to send a message back to that ID, it would, in fact, go to the server.

This addresses that issue by separating the concepts of "client ID" and "transport ID". The SDK uses client IDs that are mapped to transport IDs when interacting with the transport. This way we can ensure that "ServerClientId" (which is a references a socket on the local process) and "LocalClientId" (which actually references a socket on the remote server process) will always have unique values - ServerClientId is always 0 on the clients, and server's clientId values start at 1 and count up, so they can't overlap.

MTT-1534

Changelog

com.unity.netcode.gameobjects

  • Fixed: Fixed an issue where ServerClientId and LocalClientId could have the same value, causing potential confusion, and also fixed an issue with the UNet where the server could be identified with two different values, one of which might be the same as LocalClientId, and the other of which would not.

Testing and Documentation

  • Includes integration tests.
  • No documentation changes or additions were necessary.

@@ -243,6 +245,10 @@ public ulong LocalClientId

private Dictionary<ulong, NetworkClient> m_ConnectedClients = new Dictionary<ulong, NetworkClient>();

private ulong m_NextClientId = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bendoyon do you know if this breaks any of our assumptions about client ids in the profiler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope not since UNet and UTP client IDs aren't remotely alike... it's not really safe to have any assumptions about client ids, I don't think...

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 the only assumption we make is that 0 is the server, so that we can write "Server" instead of "Client 0", which should work in this case, but wanted a sanity check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even that's not a safe assumption! With UTP the server is not 0. It's quite a large number, actually! But now it will be safe after this. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I don't remember seeing that issue when testing with UTP. Glad that our incorrect assumption becomes correct with this change though

m_TransportIdToClientIdMap.Remove(transportId);
m_ClientIdToTransportIdMap.Remove(clientId);

//if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, no, not at all, thank you.

@andrews-unity andrews-unity merged commit 5bc76ea into develop Oct 28, 2021
@andrews-unity andrews-unity deleted the fix/wrong_clientid_on_receive branch October 28, 2021 17:10
andrews-unity added a commit that referenced this pull request Oct 28, 2021
…ing of client ID to transport ID so we can ensure no overlap (#1368)
mattwalsh-unity pushed a commit that referenced this pull request Oct 28, 2021
…ing of client ID to transport ID so we can ensure no overlap (#1368) (#1374)
mollstam pushed a commit to Keepsake-Games/com.unity.netcode.gameobjects that referenced this pull request Feb 13, 2023
…ing of client ID to transport ID so we can ensure no overlap (Unity-Technologies#1368)

* fix: Corrected wrong clientId on receive by keeping SDK internal mapping of client ID to transport ID so we can ensure no overlap
mollstam pushed a commit to Keepsake-Games/com.unity.netcode.gameobjects that referenced this pull request Feb 13, 2023
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