Merge pull request #734 from hadashiA/ku/fix-start-multiple-calls

Fix an issue with Start being called multiple times with exception
hadashiA · Dec 20, 2024 · e74d882 · e74d882
2 parents 7d839ca + a93b958
commit e74d882
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 76 deletions.
diff --git a/VContainer/Assets/Tests/Unity/PlayerLoopItemTest.cs b/VContainer/Assets/Tests/Unity/PlayerLoopItemTest.cs
@@ -1,4 +1,5 @@
-using System.Collections.Generic;
+using System;
+using System.Collections.Generic;
 using NUnit.Framework;
 using UnityEngine.TestTools.Constraints;
 using VContainer.Unity;
@@ -8,7 +9,7 @@ namespace VContainer.Tests.Unity
 {
     public class PlayerLoopItemTest
     {
-        private class Ticker : ITickable, IPostTickable, IPostLateTickable, IFixedTickable, IPostFixedTickable
+        class Ticker : ITickable, IPostTickable, IPostLateTickable, IFixedTickable, IPostFixedTickable
         {
             public void Tick() { }
             public void FixedTick() { }
@@ -17,84 +18,101 @@ public void PostLateTick() { }
             public void PostFixedTick() { }
         }
 
-        private class TickableLoopItemTest
+        class TestStartable : IStartable
         {
-            [Test]
-            public void MoveNextWithoutAllocation()
+            public int Executed { get; private set; }
+
+            public void Start()
             {
-                var list = new List<ITickable> { new Ticker(), new Ticker() };
-                var exceptionHandler = new EntryPointExceptionHandler(exception => { });
-                var tickableLoopItem = new TickableLoopItem(list, exceptionHandler);
-
-                Assert.That(() =>
-                {
-                    tickableLoopItem.MoveNext();
-                }, Is.Not.AllocatingGCMemory());
+                Executed++;
             }
         }
-        
-        private class PostTickableLoopItemTest
+
+        class ThrowStartable : IStartable
         {
-            [Test]
-            public void MoveNextWithoutAllocation()
+            public int Executed { get; private set; }
+
+            public void Start()
             {
-                var list = new List<IPostTickable> { new Ticker(), new Ticker() };
-                var exceptionHandler = new EntryPointExceptionHandler(exception => { });
-                var tickableLoopItem = new PostTickableLoopItem(list, exceptionHandler);
-
-                Assert.That(() =>
-                {
-                    tickableLoopItem.MoveNext();
-                }, Is.Not.AllocatingGCMemory());
+                Executed++;
+                throw new InvalidOperationException("OOPS");
             }
         }
-
-        private class PostLateTickableLoopItemTest
+
+        [Test]
+        public void Start_Throw()
         {
-            [Test]
-            public void MoveNextWithoutAllocation()
+            var item = new TestStartable();
+            var thrownItem = new ThrowStartable();
+            var exceptionHandler = new EntryPointExceptionHandler(ex => { });
+            var loopItem = new StartableLoopItem(new IStartable[] { thrownItem, item }, exceptionHandler);
+
+            Assert.That(loopItem.MoveNext(), Is.False);
+        }
+
+        [Test]
+        public void Tick_MoveNextWithoutAllocation()
+        {
+            var list = new List<ITickable> { new Ticker(), new Ticker() };
+            var exceptionHandler = new EntryPointExceptionHandler(exception => { });
+            var tickableLoopItem = new TickableLoopItem(list, exceptionHandler);
+
+            Assert.That(() =>
             {
-                var list = new List<IPostLateTickable> { new Ticker(), new Ticker() };
-                var exceptionHandler = new EntryPointExceptionHandler(exception => { });
-                var tickableLoopItem = new PostLateTickableLoopItem(list, exceptionHandler);
-
-                Assert.That(() =>
-                {
-                    tickableLoopItem.MoveNext();
-                }, Is.Not.AllocatingGCMemory());
-            }
+                tickableLoopItem.MoveNext();
+            }, Is.Not.AllocatingGCMemory());
         }
-
-        private class FixedTickableLoopItemTest
+
+        [Test]
+        public void PostTick_MoveNextWithoutAllocation()
         {
-            [Test]
-            public void MoveNextWithoutAllocation()
+            var list = new List<IPostTickable> { new Ticker(), new Ticker() };
+            var exceptionHandler = new EntryPointExceptionHandler(exception => { });
+            var tickableLoopItem = new PostTickableLoopItem(list, exceptionHandler);
+
+            Assert.That(() =>
             {
-                var list = new List<IFixedTickable> { new Ticker(), new Ticker() };
-                var exceptionHandler = new EntryPointExceptionHandler(exception => { });
-                var tickableLoopItem = new FixedTickableLoopItem(list, exceptionHandler);
-
-                Assert.That(() =>
-                {
-                    tickableLoopItem.MoveNext();
-                }, Is.Not.AllocatingGCMemory());
-            }
+                tickableLoopItem.MoveNext();
+            }, Is.Not.AllocatingGCMemory());
         }
-
-        private class PostFixedTickableLoopItemTest
+
+        [Test]
+        public void LateTick_MoveNextWithoutAllocation()
         {
-            [Test]
-            public void MoveNextWithoutAllocation()
+            var list = new List<IPostLateTickable> { new Ticker(), new Ticker() };
+            var exceptionHandler = new EntryPointExceptionHandler(exception => { });
+            var tickableLoopItem = new PostLateTickableLoopItem(list, exceptionHandler);
+
+            Assert.That(() =>
             {
-                var list = new List<IPostFixedTickable> { new Ticker(), new Ticker() };
-                var exceptionHandler = new EntryPointExceptionHandler(exception => { });
-                var tickableLoopItem = new PostFixedTickableLoopItem(list, exceptionHandler);
-
-                Assert.That(() =>
-                {
-                    tickableLoopItem.MoveNext();
-                }, Is.Not.AllocatingGCMemory());
-            }
+                tickableLoopItem.MoveNext();
+            }, Is.Not.AllocatingGCMemory());
+        }
+
+        [Test]
+        public void FixedTick_MoveNextWithoutAllocation()
+        {
+            var list = new List<IFixedTickable> { new Ticker(), new Ticker() };
+            var exceptionHandler = new EntryPointExceptionHandler(exception => { });
+            var tickableLoopItem = new FixedTickableLoopItem(list, exceptionHandler);
+
+            Assert.That(() =>
+            {
+                tickableLoopItem.MoveNext();
+            }, Is.Not.AllocatingGCMemory());
+        }
+
+        [Test]
+        public void PostFixedTick_MoveNextWithoutAllocation()
+        {
+            var list = new List<IPostFixedTickable> { new Ticker(), new Ticker() };
+            var exceptionHandler = new EntryPointExceptionHandler(exception => { });
+            var tickableLoopItem = new PostFixedTickableLoopItem(list, exceptionHandler);
+
+            Assert.That(() =>
+            {
+                tickableLoopItem.MoveNext();
+            }, Is.Not.AllocatingGCMemory());
         }
     }
-}
+}
diff --git a/VContainer/Assets/VContainer/Runtime/Unity/ContainerBuilderUnityExtensions.cs b/VContainer/Assets/VContainer/Runtime/Unity/ContainerBuilderUnityExtensions.cs
@@ -14,6 +14,9 @@ public static void EnsureDispatcherRegistered(IContainerBuilder containerBuilder
         {
             if (containerBuilder.Exists(typeof(EntryPointDispatcher), false)) return;
             containerBuilder.Register<EntryPointDispatcher>(Lifetime.Scoped);
+
+            containerBuilder.RegisterEntryPointExceptionHandler(UnityEngine.Debug.LogException);
+
             containerBuilder.RegisterBuildCallback(container =>
             {
                 container.Resolve<EntryPointDispatcher>().Dispatch();
@@ -120,7 +123,7 @@ public static void RegisterEntryPointExceptionHandler(
             this IContainerBuilder builder,
             Action<Exception> exceptionHandler)
         {
-            builder.RegisterInstance(new EntryPointExceptionHandler(exceptionHandler));
+            builder.Register(c => new EntryPointExceptionHandler(exceptionHandler), Lifetime.Scoped);
         }
 
         public static RegistrationBuilder RegisterComponent<TInterface>(
@@ -197,7 +200,7 @@ public static ComponentRegistrationBuilder RegisterComponentInNewPrefab<T>(
         {
             return builder.RegisterComponentInNewPrefab(typeof(T), prefab, lifetime);
         }
-        
+
         public static ComponentRegistrationBuilder RegisterComponentInNewPrefab<T>(
             this IContainerBuilder builder,
             Func<IObjectResolver, T> prefab,
@@ -206,7 +209,7 @@ public static ComponentRegistrationBuilder RegisterComponentInNewPrefab<T>(
         {
             return builder.Register(new ComponentRegistrationBuilder(prefab, typeof(T), lifetime));
         }
-        
+
         public static ComponentRegistrationBuilder RegisterComponentInNewPrefab<TInterface, TImplement>(
             this IContainerBuilder builder,
             Func<IObjectResolver, TImplement> prefab,
@@ -300,7 +303,7 @@ public static RegistrationBuilder RegisterUnmanagedSystemFromWorld<T>(this ICont
             Type refType = typeof(UnmanagedSystemReference<>);
             Type target = refType.MakeGenericType(typeof(T));
             var reference = (UnmanagedSystemReference)Activator.CreateInstance(target, system, world);
-            
+
             return builder.RegisterComponent(reference)
                 .As(target);
         }

diff --git a/VContainer/Assets/VContainer/Runtime/Unity/EntryPointDispatcher.cs b/VContainer/Assets/VContainer/Runtime/Unity/EntryPointDispatcher.cs
@@ -22,7 +22,7 @@ public void Dispatch()
         {
             PlayerLoopHelper.EnsureInitialized();
 
-            EntryPointExceptionHandler exceptionHandler = container.ResolveOrDefault<EntryPointExceptionHandler>();
+            var exceptionHandler = container.ResolveOrDefault<EntryPointExceptionHandler>();
 
             var initializables = container.Resolve<ContainerLocal<IReadOnlyList<IInitializable>>>().Value;
             for (var i = 0; i < initializables.Count; i++)

diff --git a/VContainer/UserSettings/Layouts/default-2021.dwlt b/VContainer/UserSettings/Layouts/default-2021.dwlt
@@ -48,7 +48,7 @@ MonoBehaviour:
   m_MinSize: {x: 300, y: 200}
   m_MaxSize: {x: 24288, y: 16192}
   vertical: 0
-  controlID: 83
+  controlID: 84
 --- !u!114 &3
 MonoBehaviour:
   m_ObjectHideFlags: 52
@@ -223,7 +223,7 @@ MonoBehaviour:
   m_MinSize: {x: 200, y: 200}
   m_MaxSize: {x: 16192, y: 16192}
   vertical: 1
-  controlID: 84
+  controlID: 63
 --- !u!114 &10
 MonoBehaviour:
   m_ObjectHideFlags: 52
@@ -248,7 +248,7 @@ MonoBehaviour:
   m_MinSize: {x: 200, y: 100}
   m_MaxSize: {x: 16192, y: 8096}
   vertical: 0
-  controlID: 85
+  controlID: 64
 --- !u!114 &11
 MonoBehaviour:
   m_ObjectHideFlags: 52
@@ -333,7 +333,7 @@ MonoBehaviour:
     scrollPos: {x: 0, y: 0}
     m_SelectedIDs: 84440000
     m_LastClickedID: 17540
-    m_ExpandedIDs: 0000000086440000
+    m_ExpandedIDs: 000000006e440000
     m_RenameOverlay:
       m_UserAcceptedRename: 0
       m_Name: 
@@ -361,7 +361,7 @@ MonoBehaviour:
     scrollPos: {x: 0, y: 0}
     m_SelectedIDs: 
     m_LastClickedID: 0
-    m_ExpandedIDs: 0000000086440000
+    m_ExpandedIDs: 000000006e440000
     m_RenameOverlay:
       m_UserAcceptedRename: 0
       m_Name: 
@@ -495,7 +495,7 @@ MonoBehaviour:
       scrollPos: {x: 0, y: 0}
       m_SelectedIDs: 
       m_LastClickedID: 0
-      m_ExpandedIDs: 1efbffff
+      m_ExpandedIDs: 30fbffff
       m_RenameOverlay:
         m_UserAcceptedRename: 0
         m_Name: