Make sure we call settimerevent when adding and removing timers... duh...

Author: HaroldCindy

VM/src/llltimers.cpp

@@ -43,6 +43,7 @@ int timer_event_wrapper_cont(lua_State *L, [[maybe_unused]] int status)
 // Forward declarations for timer wrapper management
 static void register_timer_wrapper(lua_State *L, lua_LLTimers *lltimers);
 static void unregister_timer_wrapper(lua_State *L, lua_LLTimers *lltimers);
+static void schedule_next_tick(lua_State *L, lua_LLTimers *lltimers);
 
 static void lltimers_dtor(lua_State *L, void *data)
 {
@@ -144,12 +145,15 @@ static int lltimers_on(lua_State *L)
     // Add to the end of the timers array
     lua_rawseti(L, -2, old_len + 1);
 
-    lua_pop(L, 1); // Pop timers array
-
     // Register timer wrapper with LLEvents when adding first timer
     if (old_len == 0)
         register_timer_wrapper(L, lltimers);
 
+    // Reschedule timer tick since we added a new timer
+    schedule_next_tick(L, lltimers);
+
+    lua_pop(L, 1); // Pop timers array
+
     // Return the passed-in handler so it can be unregistered later
     lua_pushvalue(L, 3);
     return 1;
@@ -190,6 +194,10 @@ static int lltimers_once(lua_State *L)
 
     // Add to the timers array
     lua_rawseti(L, -2, old_len + 1);
+
+    // Reschedule timer tick since we added a new timer
+    schedule_next_tick(L, lltimers);
+
     lua_pop(L, 1); // Pop timers array
 
     // Register timer wrapper with LLEvents when adding first timer
@@ -247,10 +255,11 @@ static int lltimers_off(lua_State *L)
 
     lua_pop(L, 1); // Pop timers array
 
-    // Check if we just removed the last timer
-    if (found && len == 1)
+    // Reschedule timer tick since we may have removed a timer
+    // (this will also unregister the wrapper if we removed the last timer)
+    if (found)
     {
-        unregister_timer_wrapper(L, lltimers);
+        schedule_next_tick(L, lltimers);
     }
 
     lua_pushboolean(L, found);
@@ -313,23 +322,26 @@ static void unregister_timer_wrapper(lua_State *L, lua_LLTimers *lltimers)
 }
 
 // Helper function to schedule the next timer tick
-// Expects timers_tab to be on the stack at the given index
-static void schedule_next_tick(lua_State *L)
+// Accepts LLTimers userdata as parameter and manages its own stack
+static void schedule_next_tick(lua_State *L, lua_LLTimers *lltimers)
 {
     auto *sl_state = LUAU_GET_SL_VM_STATE(lua_mainthread(L));
 
     if (!sl_state->setTimerEventCb)
         return; // No callback set, can't schedule
 
-    int len = lua_objlen(L, LIVE_TIMERS_ARRAY);
+    // Get timers array on the stack
+    lua_getref(L, lltimers->timers_tab_ref);
+    int len = lua_objlen(L, -1);
 
     if (len == 0)
     {
+        lua_pop(L, 1); // Pop timers array
+
         // No timers pending, unsubscribe from the parent timer event
         sl_state->setTimerEventCb(L, 0.0);
 
         // Unregister timer wrapper from LLEvents
-        auto *lltimers = (lua_LLTimers *)lua_touserdata(L, LLTIMERS_USERDATA);
         unregister_timer_wrapper(L, lltimers);
 
         return;
@@ -339,13 +351,15 @@ static void schedule_next_tick(lua_State *L)
     double min_time = HUGE_VAL;
     for (int i = 1; i <= len; i++)
     {
-        lua_rawgeti(L, LIVE_TIMERS_ARRAY, i);
+        lua_rawgeti(L, -1, i);  // Get timer from timers array
         lua_rawgeti(L, -1, TIMER_NEXT_RUN);
         double next_run = lua_tonumber(L, -1);
         lua_pop(L, 2); // Pop nextRun and timer data
         min_time = fmin(min_time, next_run);
     }
 
+    lua_pop(L, 1); // Pop timers array
+
     // Get current time
     double current_time = sl_state->clockProvider ? sl_state->clockProvider(L) : 0.0;
 
@@ -589,7 +603,8 @@ static int lltimers_tick_cont(lua_State *L, [[maybe_unused]]int status)
     }
 
     // Done processing all timers, schedule the next tick
-    schedule_next_tick(L);
+    auto *lltimers = (lua_LLTimers *)lua_touserdata(L, LLTIMERS_USERDATA);
+    schedule_next_tick(L, lltimers);
 
     return 0;
 }

tests/SLConformance.test.cpp

@@ -603,9 +603,11 @@ TEST_CASE("LLEvents")
 }
 
 static double test_clock_time = 0.0;
+static double last_timer_interval = -1.0;
 TEST_CASE("LLTimers")
 {
     test_clock_time = 0.0;
+    last_timer_interval = -1.0;
     runConformance("lltimers.lua", nullptr, [](lua_State *L) {
         lua_pushcfunction(L, lua_break, "breaker");
         lua_setglobal(L, "breaker");
@@ -624,15 +626,23 @@ TEST_CASE("LLTimers")
         }, "getclock");
         lua_setglobal(L, "getclock");
 
+        // Provide a function to read the last timer interval
+        lua_pushcfunction(L, [](lua_State *L) {
+            lua_pushnumber(L, last_timer_interval);
+            return 1;
+        }, "get_last_interval");
+        lua_setglobal(L, "get_last_interval");
+
         auto sl_state = LUAU_GET_SL_VM_STATE(L);
         // Set up clock callback to return our test time
         sl_state->performanceClockProvider = sl_state->clockProvider = [](lua_State *L) {
             return test_clock_time;
         };
-        // Set up timer event callback (no-op for tests)
+        // Set up timer event callback to capture the last interval
         sl_state->setTimerEventCb = [](lua_State *L, double interval) {
             // In real usage, this would schedule a timer event
-            // For tests, we manually call _tick()
+            // For tests, we manually call _tick() and capture the interval
+            last_timer_interval = interval;
         };
     });
 }

tests/conformance/lltimers.lua

@@ -28,12 +28,12 @@ assert(typeof(on_handler) == "function")
 -- Simulate timer tick
 setclock(0.05) -- Not time yet
 -- `ll.GetTime()` should be using the clock provider.
-assert((ll.GetTime() - 0.05) < 0.01)
+assert(math.abs(ll.GetTime() - 0.05) < 0.01)
 LLTimers:_tick()
 assert(on_count == 0)
 
 -- This should be using our fake clock
-assert(os.clock() - 0.05 < 0.000001)
+assert(math.abs(os.clock() - 0.05) < 0.000001)
 
 incrementclock(0.05) -- Advance to past 0.1, should fire now
 LLTimers:_tick()
@@ -367,4 +367,33 @@ LLTimers:off(throw_error)
 LLTimers:_tick()
 assert_errors(function() timers_clone:_tick() end, "called!")
 
+-- Test that setTimerEventCb is called with correct intervals
+-- Single timer should schedule with correct interval
+setclock(10.0)
+local interval_timer1 = LLTimers:every(0.5, function() end)
+assert(math.abs(get_last_interval() - 0.5) < 0.001)
+
+-- Adding an earlier timer should reschedule to shorter interval
+local interval_timer2 = LLTimers:on(0.3, function() end)
+assert(math.abs(get_last_interval() - 0.3) < 0.001)
+
+-- Adding a later timer should not change the interval
+local interval_timer3 = LLTimers:on(1.0, function() end)
+-- Should still be 0.3 (the earliest timer)
+assert(math.abs(get_last_interval() - 0.3) < 0.001)
+
+-- Scenario 4: Removing the earliest timer should reschedule to next timer
+LLTimers:off(interval_timer2)
+-- Now earliest should be 0.5
+assert(math.abs(get_last_interval() - 0.5) < 0.001)
+
+-- Scenario 5: Removing all timers should call with 0.0
+LLTimers:off(interval_timer1)
+LLTimers:off(interval_timer3)
+assert(math.abs(get_last_interval() - 0.0) < 0.001)
+
+-- Oh also we should make sure that `:once()` behaves correctly.
+LLTimers:once(0.5, interval_timer1)
+assert(math.abs(get_last_interval() - 0.5) < 0.001)
+
 return "OK"