Skip to content

fix: Don't let exceptions in OnNetworkSpawn/OnNetworkDespawn block processing of the next callback. [MTT-1378] #1739

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.

Sign up for GitHub

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
NoelStephensUnity merged 17 commits into from
Aug 25, 2022
Merged

fix: Don't let exceptions in OnNetworkSpawn/OnNetworkDespawn block processing of the next callback. [MTT-1378] #1739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6b3e1ab
fix: Don't let exceptions in OnNetworkSpawn/OnNetworkDespawn block pr…
ShadauxCat Feb 22, 2022
d502aa1
Standards and changelog.
ShadauxCat Feb 22, 2022
ef344d2
Standards fixes
ShadauxCat Feb 22, 2022
4749f46
It would be good to check in the right file.
ShadauxCat Feb 22, 2022
23d4b15
I swear these tests passed before but I have no idea how.
ShadauxCat Mar 1, 2022
012aa98
Address review feedback.
ShadauxCat Mar 29, 2022
530e728
Switched to LogAssert.Expect
ShadauxCat Mar 29, 2022
84b195a
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat May 9, 2022
032202b
Updated tests to NetcodeIntegrationTest style
ShadauxCat May 9, 2022
cede7c3
Standards fix
ShadauxCat May 20, 2022
2ece99f
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat Aug 24, 2022
8a7c8b8
Rewrote tests according to feedback.
ShadauxCat Aug 24, 2022
a70c778
Test the behavior on both server and client
ShadauxCat Aug 24, 2022
3753803
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat Aug 24, 2022
5017321
style
NoelStephensUnity Aug 24, 2022
0fe6471
Update com.unity.netcode.gameobjects/CHANGELOG.md
ShadauxCat Aug 24, 2022
2ba24c6
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
NoelStephensUnity Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
- Fixed NetworkList to properly call INetworkSerializable's NetworkSerialize() method (#1682)
- Fixed The NetworkConfig's checksum hash includes the NetworkTick so that clients with a different tickrate than the server are identified and not allowed to connect (#1728)
- Fixed OwnedObjects not being properly modified when using ChangeOwnership (#1731)
- Fixed throwing an exception in OnNetworkUpdate causing other OnNetworkUpdate calls to not be executed. (#1739)
- Fixed throwing an exception in OnNetworkSpawn/OnNetworkDespawn causing other OnNetworkSpawn/OnNetworkDespawn calls to not be executed. (#1739)

## [1.0.0-pre.5] - 2022-01-26

Expand Down
4 changes: 2 additions & 2 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ internal void InvokeBehaviourNetworkSpawn()
}
catch (Exception e)
{
Console.WriteLine(e);
NetworkLog.LogError(e.ToString());
}
}
}
Expand All @@ -818,7 +818,7 @@ internal void InvokeBehaviourNetworkDespawn()
}
catch (Exception e)
{
Console.WriteLine(e);
NetworkLog.LogError(e.ToString());
Comment thread
ShadauxCat marked this conversation as resolved.
Outdated
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions testproject/Assets/Tests/Runtime/OnNetworkSpawnExceptionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class OnNetworkSpawnExceptionTests : BaseMultiInstanceTest
[UnityTest]
public IEnumerator WhenOnNetworkSpawnThrowsException_FutureOnNetworkSpawnsAreNotPrevented()
Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer May 9, 2022

Choose a reason for hiding this comment

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

These tests feel pretty rough in their current state.

  • There are lots of magic numbers. There are even magic numbers that depend on other magic numbers (2, 3, and 5)
  • You have 1 setup for 2 different tests. Is all the code in the setup necessary for both?
  • You have log asserts in your test setup. Are these asserts actual tests, or are they an attempt to verify that setup has done what it's expected to?
  • You are using the multi-instance tests and have a client but it's unclear if this is actually necessary for this test (don't these virtual methods get invoked on the server too? Can't you write a much simpler/more contained UnityTest that doesn't involve a client or waiting for frames with timeout asserts and if (IsClient)?)
  • It's unclear (to me) why you have if (NumClientSpawns > 2) guard in the test components.

While it's a very contentious topic, generally speaking, the more self-contained a test is, the better for understanding and lower the risk for future false positives. I'm not personally a fan of having to read 3 classes and 6 methods to understand what's actually happening in this 1 line test case.

Copy link
Copy Markdown
Member

@NoelStephensUnity NoelStephensUnity May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

While throwing an exception in a component added to the player prefab is valid, I think this test should add more NetworkBehaviour derived components because the real test is to validate that all NetworkBehaviour components on a GameObject get OnNetworkSpawn and OnNetworkDespawn invoked even if one of the components throws and exception.

Copy link
Copy Markdown
Collaborator Author

@ShadauxCat ShadauxCat Aug 24, 2022

Choose a reason for hiding this comment

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

I think I've addressed everything you mentioned here, except for the use of multi-instance tests. I think it's valuable to test that the behavior is the same on both server and client. I did, however, update the test to make sure it isn't exclusively a client-side test, since OnNetworkSpawn and OnNetworkDespawn are called on the server too.

{
LogAssert.ignoreFailingMessages = true;
Comment thread
ShadauxCat marked this conversation as resolved.
Outdated
//Spawning was done during setup
Assert.AreEqual(5, OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns);
yield return null;
Expand All @@ -56,6 +57,7 @@ public IEnumerator WhenOnNetworkSpawnThrowsException_FutureOnNetworkSpawnsAreNot
[UnityTest]
public IEnumerator WhenOnNetworkDespawnThrowsException_FutureOnNetworkDespawnsAreNotPrevented()
{
LogAssert.ignoreFailingMessages = true;
//Spawning was done during setup. Now we despawn.
for (var i = 0; i < 5; ++i)
{
Expand All @@ -71,11 +73,15 @@ public IEnumerator WhenOnNetworkDespawnThrowsException_FutureOnNetworkDespawnsAr

public override IEnumerator Setup()
{
yield return StartSomeClientsAndServerWithPlayers(true, NbClients, _ =>
OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns = 0;
OnNetworkDespawnThrowsExceptionComponent.NumClientDespawns = 0;
LogAssert.ignoreFailingMessages = true;
yield return StartSomeClientsAndServerWithPlayers(false, NbClients, _ =>
{
m_Prefab = new GameObject();
var networkObject = m_Prefab.AddComponent<NetworkObject>();
m_Prefab.AddComponent<OnNetworkSpawnThrowsExceptionComponent>();
m_Prefab.AddComponent<OnNetworkDespawnThrowsExceptionComponent>();
MultiInstanceHelpers.MakeNetworkObjectTestPrefab(networkObject);

var validNetworkPrefab = new NetworkPrefab();
Expand All @@ -96,7 +102,7 @@ public override IEnumerator Setup()
}

var result = new MultiInstanceHelpers.CoroutineResultWrapper<bool>();
MultiInstanceHelpers.Run(
yield return MultiInstanceHelpers.Run(
MultiInstanceHelpers.WaitForCondition(
() => OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns == 5, result));
Assert.IsTrue(result.Result);
Expand Down