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

[ShadauxCat]

MTT-1378

Changelog

com.unity.netcode.gameobjects

• Fixed: Fixed throwing an exception in OnNetworkUpdate causing other OnNetworkUpdate calls to not be executed.

Testing and Documentation

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

[mattwalsh-unity]

Want to understand more about the absorbing of exceptions

[0xFA11]

+1 for Debug.LogException() instead of NetworkLog.LogError() both in spawn and despawn flows.
also LogAssert.Expect exists, that'd potentially make tests simpler too.

[TwoTenPvP]

+1 for Debug.LogException() instead of NetworkLog.LogError() both in spawn and despawn flows.

also LogAssert.Expect exists, that'd potentially make tests simpler too.

Surley we want to use NetworkLog though? Perhaps adding NetworkLog.LogException

Otherwise NetworkLog is incomplete if only half the logs are networked.

[0xFA11]

+1 for Debug.LogException() instead of NetworkLog.LogError() both in spawn and despawn flows.
also LogAssert.Expect exists, that'd potentially make tests simpler too.

Surley we want to use NetworkLog though? Perhaps adding NetworkLog.LogException

Otherwise NetworkLog is incomplete if only half the logs are networked.

I see your point @TwoTenPvP but I think NetworkLog & NetworkLog.LogException needs a separate conversation. We might come back to this to update once we have that other discussion.

[TwoTenPvP]

+1 for Debug.LogException() instead of NetworkLog.LogError() both in spawn and despawn flows.

also LogAssert.Expect exists, that'd potentially make tests simpler too.

Surley we want to use NetworkLog though? Perhaps adding NetworkLog.LogException

Otherwise NetworkLog is incomplete if only half the logs are networked.

I see your point @TwoTenPvP but I think NetworkLog & NetworkLog.LogException needs a separate conversation. We might come back to this to update once we have that other discussion.

Personally I prefer a bit worse log messages than an incomplete feature. NetworkLog.LogError / Debug.LogException.

I think using NetworkLog.LogError is the best choice if NetworkLog.LogException is out of the question

[NoelStephensUnity]

Looks perfectly fine to me!
(except the integration test)

🙈

[JesseOlmer]

Closed and re-opened to fix the JIRA association. Please make sure to add the JIRA ticket to the title upon PR creation in the future.

[JesseOlmer]

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.

[NoelStephensUnity]

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.

[ShadauxCat]

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.

[NoelStephensUnity]

The included integration test only verifies that it throws an exception but does not verify that other components on the same GameObject still have their OnNetworkSpawn and OnNetworkDespawn method invoked when there are one or more NetworkBehaviour components on the GameObject and one of them throws an exception during the spawn/despawn periods.

Maybe adding an additional component after the one that throws an exception could be the verification that it was invoked after the NetworkBehaviour before it throws an exception?

[NoelStephensUnity]

Looks good to me kitty!