CHANGE: Add event processing limits (#1325)

Author: andrew-oc

Assets/TestScript.cs.meta

@@ -20,6 +20,7 @@
 using UnityEngine.TestTools.Constraints;
 using UnityEngine.TestTools.Utils;
 using Is = UnityEngine.TestTools.Constraints.Is;
+using Random = UnityEngine.Random;
 using TouchPhase = UnityEngine.InputSystem.TouchPhase;
 
 #pragma warning disable CS0649
@@ -1991,7 +1992,6 @@ public IEnumerator Events_CanTestInputDistributedOverFrames()
 
     [Test]
     [Category("Events")]
-    [Ignore("Not implemented yet. Fix as part of https://jira.unity3d.com/browse/ISX-557.")]
     public void Events_MaximumEventLoadPerUpdateIsLimited()
     {
         // Default setting is 5MB.
@@ -2009,7 +2009,7 @@ public void Events_MaximumEventLoadPerUpdateIsLimited()
         InputSystem.onEvent += (eventPtr, device) => ++ eventCount;
 
         LogAssert.Expect(LogType.Error, "Exceeded budget for maximum input event throughput per InputSystem.Update(). Discarding remaining events. "
-            + "Increase InputSystem.settings.maxEventBytesPerUpdate or set it to 0 to raise the limit.");
+            + "Increase InputSystem.settings.maxEventBytesPerUpdate or set it to 0 to remove the limit.");
 
         InputSystem.Update();
 
@@ -2041,6 +2041,10 @@ public void Events_CanQueueEventsFromWithinEventProcessing()
         var keyboard = InputSystem.AddDevice<Keyboard>();
 
         var numMouseEventsQueued = InputTestRuntime.kDefaultEventBufferSize / StateEvent.GetEventSizeWithPayload<MouseState>() + 1;
+
+        // allow all these events
+        InputSystem.settings.maxQueuedEventsPerUpdate = numMouseEventsQueued;
+
         var numMouseEventsReceived = 0;
         InputSystem.onEvent +=
             (eventPtr, device) =>
@@ -2068,5 +2072,36 @@ public void Events_CanQueueEventsFromWithinEventProcessing()
         Assert.That(numMouseEventsReceived, Is.EqualTo(numMouseEventsQueued));
     }
 
+    [Test]
+    [Category("Events")]
+    public void Events_MaximumQueuedEventsDuringEventProcessingIsLimited()
+    {
+        // Default setting is 1000.
+        Assert.That(InputSystem.settings.maxQueuedEventsPerUpdate, Is.EqualTo(1000));
+
+        InputSystem.settings.maxQueuedEventsPerUpdate = 20;
+
+        var mouse = InputSystem.AddDevice<Mouse>();
+
+        var callbackCount = 0;
+        var action = new InputAction(type: InputActionType.Value, binding: "<mouse>/position");
+        action.performed +=
+            _ =>
+        {
+            if (callbackCount > InputSystem.settings.maxQueuedEventsPerUpdate)
+                Assert.Fail("Maximum queued event count exceeded");
+
+            callbackCount++;
+            Set(mouse.position, Random.insideUnitCircle * 100, queueEventOnly: true);
+        };
+        action.Enable();
+
+        Set(mouse.position, Random.insideUnitCircle * 100);
+
+        LogAssert.Expect(LogType.Error, $"Maximum number of queued events exceeded. Set the '{nameof(InputSettings.maxQueuedEventsPerUpdate)}' setting to a higher value if you " +
+            $"need to queue more events than this. Current limit is '{InputSystem.settings.maxQueuedEventsPerUpdate}'.");
+        Assert.That(callbackCount - 1, Is.EqualTo(InputSystem.settings.maxQueuedEventsPerUpdate));
+    }
+
     ////TODO: test thread-safe QueueEvent
 }

Assets/Tests/InputSystem/CoreTests_Events.cs

@@ -108,6 +108,8 @@ however, it has to be formatted properly to pass verification tests.
   * Puts an upper limit on the number of event bytes processed in a single update.
   * If exceeded, any additional event data will get thrown away and an error will be issued.
   * Set to 5MB by default.
+- Added a new API-only setting called `InputSystem.settings.maxQueuedEventsPerUpdate`.
+  * This limits the number of events that can be queued during event processing using the `InputSystem.QueueEvent` method. This guards against infinite loops in the case where an action callback queues an event that causes the same action callback to be called again.
 - Added `InputSystemUIInputModule.AssignDefaultActions` to assign default actions when creating ui module in runtime.
 - Added `UNITY_INCLUDE_TESTS` define constraints to our test assemblies, which is 2019.2+ equivalent to `"optionalUnityReferences": ["TestAssemblies"]`.
 

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -30,7 +30,7 @@ internal unsafe struct InputEventStream
                 (byte*)NativeArrayUnsafeUtility
                     .GetUnsafeBufferPointerWithoutChecks(m_NativeBuffer.data));
 
-        public InputEventStream(ref InputEventBuffer eventBuffer)
+        public InputEventStream(ref InputEventBuffer eventBuffer, int maxAppendedEvents)
         {
             m_CurrentNativeEventWritePtr = m_CurrentNativeEventReadPtr =
                 (InputEvent*)NativeArrayUnsafeUtility.GetUnsafeBufferPointerWithoutChecks(eventBuffer.data);
@@ -42,6 +42,7 @@ public InputEventStream(ref InputEventBuffer eventBuffer)
             m_CurrentAppendEventReadPtr = m_CurrentAppendEventWritePtr = default;
             m_AppendBuffer = default;
             m_RemainingAppendEventCount = 0;
+            m_MaxAppendedEvents = maxAppendedEvents;
 
             m_IsOpen = true;
         }
@@ -71,6 +72,14 @@ public void Close(ref InputEventBuffer eventBuffer)
 
         public void Write(InputEvent* eventPtr)
         {
+            if (m_AppendBuffer.eventCount >= m_MaxAppendedEvents)
+            {
+                Debug.LogError($"Maximum number of queued events exceeded. Set the '{nameof(InputSettings.maxQueuedEventsPerUpdate)}' " +
+                    $"setting to a higher value if you need to queue more events than this. " +
+                    $"Current limit is '{m_MaxAppendedEvents}'.");
+                return;
+            }
+
             var wasAlreadyCreated = m_AppendBuffer.data.IsCreated;
             var oldBufferPtr = (byte*)m_AppendBuffer.bufferPtr.data;
 
@@ -117,6 +126,7 @@ public void Write(InputEvent* eventPtr)
         private InputEvent* m_CurrentNativeEventReadPtr;
         private InputEvent* m_CurrentNativeEventWritePtr;
         private int m_RemainingNativeEventCount;
+        private readonly int m_MaxAppendedEvents;
 
         // During Update, new events that are queued will be added to the append buffer
         private InputEventBuffer m_AppendBuffer;

Packages/com.unity.inputsystem/InputSystem/Events/InputEventStream.cs

@@ -2631,11 +2631,20 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             var processingStartTime = Stopwatch.GetTimestamp();
             var totalEventLag = 0.0;
 
-            m_InputEventStream = new InputEventStream(ref eventBuffer);
+            m_InputEventStream = new InputEventStream(ref eventBuffer, m_Settings.maxQueuedEventsPerUpdate);
+            var totalEventBytesProcessed = 0U;
 
             // Handle events.
             while (m_InputEventStream.remainingEventCount > 0)
             {
+                if (m_Settings.maxEventBytesPerUpdate > 0 &&
+                    totalEventBytesProcessed >= m_Settings.maxEventBytesPerUpdate)
+                {
+                    Debug.LogError("Exceeded budget for maximum input event throughput per InputSystem.Update(). Discarding remaining events. "
+                        + "Increase InputSystem.settings.maxEventBytesPerUpdate or set it to 0 to remove the limit.");
+                    break;
+                }
+
                 InputDevice device = null;
                 var currentEventReadPtr = m_InputEventStream.currentEventPtr;
 
@@ -2786,6 +2795,8 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
                             haveChangedStateOtherThanNoise = UpdateState(device, eventPtr, updateType);
                         }
 
+                        totalEventBytesProcessed += eventPtr.sizeInBytes;
+
                         // Update timestamp on device.
                         // NOTE: We do this here and not in UpdateState() so that InputState.Change() will *NOT* change timestamps.
                         //       Only events should.

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

@@ -417,9 +417,8 @@ public float multiTapDelayTime
         /// </summary>
         /// <remarks>
         /// This setting establishes a bound on the amount of input event data processed in a single
-        /// update and thus limits throughput allowed for input. This both prevents long stalls from
-        /// leading to long delays in input processing and prevents deadlocks when events themselves
-        /// lead to additional events being queued.
+        /// update and thus limits throughput allowed for input. This prevents long stalls from
+        /// leading to long delays in input processing.
         ///
         /// When the limit is exceeded, all events remaining in the buffer are thrown away (the
         /// <see cref="InputEventBuffer"/> is reset) and an error is logged. After that, the current
@@ -443,6 +442,35 @@ public int maxEventBytesPerUpdate
             }
         }
 
+        /// <summary>
+        /// Upper limit on the number of <see cref="InputEvent"/>s that can be queued within one
+        /// <see cref="InputSystem.Update"/>.
+        /// <remarks>
+        /// This settings establishes an upper limit on the number of events that can be queued
+        /// using <see cref="InputSystem.QueueEvent"/> during a single update. This prevents infinite
+        /// loops where an action callback queues an event that causes the action callback to
+        /// be called again which queues an event...
+        ///
+        /// Note that this limit only applies while the input system is updating. There is no limit
+        /// on the number of events that can be queued outside of this time, but those will be queued
+        /// into the next frame where the <see cref="maxEventBytesPerUpdate"/> setting will apply.
+        ///
+        /// The default value is 1000.
+        /// </remarks>
+        /// </summary>
+        public int maxQueuedEventsPerUpdate
+        {
+            get => m_MaxQueuedEventsPerUpdate;
+            set
+            {
+                if (m_MaxQueuedEventsPerUpdate == value)
+                    return;
+
+                m_MaxQueuedEventsPerUpdate = value;
+                OnChange();
+            }
+        }
+
         /// <summary>
         /// List of device layouts used by the project.
         /// </summary>
@@ -496,7 +524,8 @@ public ReadOnlyArray<string> supportedDevices
         [Tooltip("Determine when Unity processes events. By default, accumulated input events are flushed out before each fixed update and "
             + "before each dynamic update. This setting can be used to restrict event processing to only where the application needs it.")]
         [SerializeField] private UpdateMode m_UpdateMode = UpdateMode.ProcessEventsInDynamicUpdate;
-        [SerializeField] private int m_MaxEventBytesPerUpdate = 5 * 1014 * 1024;
+        [SerializeField] private int m_MaxEventBytesPerUpdate = 5 * 1024 * 1024;
+        [SerializeField] private int m_MaxQueuedEventsPerUpdate = 1000;
 
         [SerializeField] private bool m_CompensateForScreenOrientation = true;
         [SerializeField] private bool m_FilterNoiseOnCurrent = false;