Skip to content
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

fix: host not generating its own local client disconnect notification #2822

Merged
merged 11 commits into from Mar 12, 2024

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Feb 4, 2024

This fix assures a host will invoke OnClientDisconnectCallback for its local client when it is internally shutting down.

fix: #2789

Changelog

  • Fixed: Issue where the host was not invoking OnClientDisconnectCallback for its own local client when internally shutting down.

Testing and Documentation

  • Includes integration test update.
  • No documentation changes or additions were necessary.

Assure the host generates a notification for the local host client no longer being connected when it shuts itself down.
Include modification to the DisconnectTest to validate this fix.
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner February 4, 2024 23:54
updating the change log
fixing improper name for callback
Rewrote the SenderIdTest to be a bit more flexible with regard to waiting for the client disconnect event and not failing if it received anything afterwards.
whitespace removal
}
m_ServerNetworkManager.OnClientDisconnectCallback += firstCallback;
m_ClientsDisconnected.Clear();
m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnected;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a check for this behavior in OnConnectionEvent? InvokeOnClientDisconnectCallback should invoke both of them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also discovered an issue with StopOneClient while adding the test... so it was a 2 4 1.
Validated the ConnectionEvent.ClientDisconnected triggers under both scenarios and fixed the integration test method.
👍

Adding check for ConnectionEvent.ClientDisconnected for both test types.
Fixing issue with StopOneClient destroying the NetworkManager when destroy is set to false.
@NoelStephensUnity NoelStephensUnity merged commit ed867b5 into develop Mar 12, 2024
24 checks passed
@NoelStephensUnity NoelStephensUnity deleted the fix/host-on-client-disconnect-local-client branch March 12, 2024 21:12
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.

None yet

3 participants