fix: changing SceneEventData allocator from TmpJob to Persistent (#1237)

* fix

this removes the memory leak warning where the InternalBuffer reader was being created with Allocator.TmpJob (if around for more than 4 frames it generates a warning).  Switch this to be Allocator.Persistent since loading and scene synchronization will most likely happen over more than a 4 frame period of time.

* test

first half.

* test

Backing out scenes loaded for MultiSceneSynchronizationTest

* fix and test

While writing the test for this PR I found an issue where it could try to dispose SceneEventData instance more than once during a MultiInstance unit test.  Removing the EndSceneEvent(sceneEventId); from line 998 fixed this issue.

The rest of this commit finalizes the test for this PR.

* style

replaced UNITY_EDITOR || DEVELOPMENT_BUILD with UNITY_INCLUDE_TESTS

* refactor

Created a better test to verify that changing the FastBufferReader's memory allocation type from Allocator.TmpJob to Allocator.Persistent fixes the issue where a scene event could take longer than 4 frames and cause an error and log that a potential memory leak could be happening.

Removed previous test and associated changes to NetworkSceneManager.

* refactor

missed one #if UNITY_INCLUDE_TESTS

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -770,7 +770,7 @@ private void OnClientUnloadScene(uint sceneEventId)
             }
             s_IsSceneEventActive = true;
             var sceneUnload = (AsyncOperation)null;
-#if UNITY_EDITOR || DEVELOPMENT_BUILD
+#if UNITY_INCLUDE_TESTS
             if (m_IsRunningUnitTest)
             {
                 sceneUnload = new AsyncOperation();
@@ -800,7 +800,7 @@ private void OnClientUnloadScene(uint sceneEventId)
             });
 
 
-#if UNITY_EDITOR || DEVELOPMENT_BUILD
+#if UNITY_INCLUDE_TESTS
             if (m_IsRunningUnitTest)
             {
                 OnSceneUnloaded(sceneEventId);
@@ -993,10 +993,10 @@ private void OnClientSceneLoadingEvent(uint sceneEventId)
                 }
                 else
                 {
+                    EndSceneEvent(sceneEventId);
                     throw new Exception($"Could not find the scene handle {sceneEventData.SceneHandle} for scene {sceneName} " +
                         $"during unit test.  Did you forget to register this in the unit test?");
                 }
-                EndSceneEvent(sceneEventId);
                 return;
             }
 #endif
@@ -1273,7 +1273,7 @@ private void OnClientBeginSync(uint sceneEventId)
             var loadSceneMode = sceneIndex == sceneEventData.SceneIndex ? sceneEventData.LoadSceneMode : LoadSceneMode.Additive;
 
             // Always check to see if the scene needs to be validated
-            if (!ValidateSceneBeforeLoading(sceneEventData.SceneIndex, loadSceneMode))
+            if (!ValidateSceneBeforeLoading(sceneIndex, loadSceneMode))
             {
                 EndSceneEvent(sceneEventId);
                 return;
@@ -1303,7 +1303,7 @@ private void OnClientBeginSync(uint sceneEventId)
                 shouldPassThrough = true;
             }
 
-#if UNITY_EDITOR || DEVELOPMENT_BUILD
+#if UNITY_INCLUDE_TESTS
             if (m_IsRunningUnitTest)
             {
                 // In unit tests, we don't allow clients to load additional scenes since

com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs

@@ -475,7 +475,8 @@ internal void Deserialize(FastBufferReader reader)
                             // We store off the trailing in-scene placed serialized NetworkObject data to
                             // be processed once we are done loading.
                             m_HasInternalBuffer = true;
-                            InternalBuffer = new FastBufferReader(reader.GetUnsafePtrAtCurrentPosition(), Allocator.TempJob, reader.Length - reader.Position);
+                            // We use Allocator.Persistent since scene loading could take longer than 4 frames
+                            InternalBuffer = new FastBufferReader(reader.GetUnsafePtrAtCurrentPosition(), Allocator.Persistent, reader.Length - reader.Position);
                         }
                         break;
                     }
@@ -517,7 +518,8 @@ internal void CopySceneSyncrhonizationData(FastBufferReader reader)
                 }
 
                 m_HasInternalBuffer = true;
-                InternalBuffer = new FastBufferReader(reader.GetUnsafePtrAtCurrentPosition(), Allocator.TempJob, sizeToCopy);
+                // We use Allocator.Persistent since scene synchronization will most likely take longer than 4 frames
+                InternalBuffer = new FastBufferReader(reader.GetUnsafePtrAtCurrentPosition(), Allocator.Persistent, sizeToCopy);
             }
         }
 

testproject/Assets/Tests/Runtime/NetworkSceneManagerTests.cs

@@ -546,4 +546,59 @@ public IEnumerator SceneEventDataPoolSceneLoadingTest([Values(LoadSceneMode.Sing
             yield break;
         }
     }
+
+    /// <summary>
+    /// This is where all of the SceneEventData specific tests should reside.
+    /// </summary>
+    public class SceneEventDataTests
+    {
+        /// <summary>
+        /// This verifies that change from Allocator.TmpJob to Allocator.Persistent
+        /// will not cause memory leak warning notifications if the scene event takes
+        /// longer than 4 frames to complete.
+        /// </summary>
+        /// <returns></returns>
+        [UnityTest]
+        public IEnumerator FastReaderAllocationTest()
+        {
+            var fastBufferWriter = new FastBufferWriter(1024, Unity.Collections.Allocator.Persistent);
+            var networkManagerGameObject = new GameObject("NetworkManager - Host");
+
+            var networkManager = networkManagerGameObject.AddComponent<NetworkManager>();
+            networkManager.NetworkConfig = new NetworkConfig()
+            {
+                ConnectionApproval = false,
+                NetworkPrefabs = new List<NetworkPrefab>(),
+                NetworkTransport = networkManagerGameObject.AddComponent<SIPTransport>(),
+            };
+
+            networkManager.StartHost();
+
+            var sceneEventData = new SceneEventData(networkManager);
+            sceneEventData.SceneEventType = SceneEventData.SceneEventTypes.S2C_Load;
+            sceneEventData.SceneIndex = 0;
+            sceneEventData.SceneEventProgressId = Guid.NewGuid();
+            sceneEventData.LoadSceneMode = LoadSceneMode.Single;
+            sceneEventData.SceneHandle = 32768;
+
+            sceneEventData.Serialize(fastBufferWriter);
+            var nativeArray = new Unity.Collections.NativeArray<byte>(fastBufferWriter.ToArray(), Unity.Collections.Allocator.Persistent);
+            var fastBufferReader = new FastBufferReader(nativeArray, Unity.Collections.Allocator.Persistent, fastBufferWriter.ToArray().Length);
+
+            var incomingSceneEventData = new SceneEventData(networkManager);
+            incomingSceneEventData.Deserialize(fastBufferReader);
+
+            // Wait for 30 frames
+            var framesToWait = Time.frameCount + 30;
+            yield return new WaitUntil(() => Time.frameCount > framesToWait);
+
+            // As long as no errors occurred, the test verifies that
+            incomingSceneEventData.Dispose();
+            fastBufferReader.Dispose();
+            nativeArray.Dispose();
+            fastBufferWriter.Dispose();
+            networkManager.Shutdown();
+            UnityEngine.Object.Destroy(networkManagerGameObject);
+        }
+    }
 }