Fix setInterval starving the JS dispatch queue

Polyfills/Scheduling/Source/TimeoutDispatcher.cpp

@@ -19,15 +19,20 @@ namespace Babylon::Polyfills::Internal
     {
         TimeoutId id;
 
+        // Distinguishes this timeout from a later one that happens to reuse the
+        // same id, so an in-flight callback can never re-arm its replacement.
+        uint64_t sequence;
+
         // Make this non-shared when JsRuntime::Dispatch supports it.
         std::shared_ptr<Napi::FunctionReference> function;
 
         TimePoint time;
 
         std::optional<std::chrono::milliseconds> interval;
 
-        Timeout(TimeoutId id, std::shared_ptr<Napi::FunctionReference> function, TimePoint time, std::optional<std::chrono::milliseconds> interval)
+        Timeout(TimeoutId id, uint64_t sequence, std::shared_ptr<Napi::FunctionReference> function, TimePoint time, std::optional<std::chrono::milliseconds> interval)
             : id{id}
+            , sequence{sequence}
             , function{std::move(function)}
             , time{time}
             , interval{interval}
@@ -77,7 +82,7 @@ namespace Babylon::Polyfills::Internal
         }
         const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time;
         const auto time = Now() + delay;
-        const auto result = m_idMap.insert({id, std::make_unique<Timeout>(id, std::move(function), time, repeat ? std::make_optional<std::chrono::milliseconds>(delay) : std::nullopt)});
+        const auto result = m_idMap.insert({id, std::make_unique<Timeout>(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional<std::chrono::milliseconds>(delay) : std::nullopt)});
         m_timeMap.insert({time, result.first->second.get()});
 
         if (time <= earliestTime)
@@ -150,14 +155,18 @@ namespace Babylon::Polyfills::Internal
             while (!m_timeMap.empty() && m_timeMap.begin()->second->time == nextTimePoint)
             {
                 const auto id = m_timeMap.begin()->second->id;
+                const auto sequence = m_timeMap.begin()->second->sequence;
                 m_timeMap.erase(m_timeMap.begin());
-                const auto repeat = m_idMap[id]->interval.has_value();
-                if (repeat)
-                {
-                    const auto timeout = std::move(m_idMap.extract(id).mapped());
-                    DispatchImpl(std::move(timeout->function), *timeout->interval, true, timeout->id);
-                }
-                CallFunction(id);
+
+                // Repeating timeouts are deliberately NOT re-armed here. They are
+                // re-armed on the JS thread once the callback has actually run, so
+                // that at most one invocation of a given interval is ever queued.
+                // Re-arming here instead would let this thread -- which never waits
+                // while a due timeout exists -- spin and enqueue callbacks far
+                // faster than the JS thread can drain them. The resulting unbounded
+                // backlog starves every other item on the JS dispatch queue: other
+                // timers, and native async completions such as shader compilation.
+                CallFunction(id, sequence);
             }
 
             while (!m_shutdown && m_timeMap.empty())
@@ -167,32 +176,99 @@ namespace Babylon::Polyfills::Internal
         }
     }
 
-    void TimeoutDispatcher::CallFunction(TimeoutId id)
+    void TimeoutDispatcher::CallFunction(TimeoutId id, uint64_t sequence)
     {
-        m_runtime.Dispatch([id, this](Napi::Env) {
+        m_runtime.Dispatch([id, sequence, this](Napi::Env) {
             std::shared_ptr<Napi::FunctionReference> function{};
+            std::optional<std::chrono::milliseconds> interval{};
+            TimePoint scheduledTime{};
             {
                 std::unique_lock<std::recursive_mutex> lk{m_mutex};
                 const auto it = m_idMap.find(id);
-                if (it != m_idMap.end())
+                if (it == m_idMap.end() || it->second->sequence != sequence)
                 {
-                    const auto repeat = it->second->interval.has_value();
-                    if (repeat)
-                    {
-                        function = it->second->function;
-                    }
-                    else
+                    // Cleared before the callback could run, or the id has since
+                    // been reused by an unrelated timeout.
+                    return;
+                }
+
+                interval = it->second->interval;
+                scheduledTime = it->second->time;
+
+                if (interval.has_value())
+                {
+                    function = it->second->function;
+                }
+                else
+                {
+                    const auto timeout = std::move(m_idMap.extract(id).mapped());
+                    function = std::move(timeout->function);
+                }
+            }
+
+            if (function)
+            {
+                try
+                {
+                    function->Call({});
+                }
+                catch (const Napi::Error& error)
+                {
+                    // A throwing tick must not silently stop the interval, which
+                    // is both the pre-existing behavior and what browsers do.
+                    // Re-arm first, then re-raise the error as a pending JS
+                    // exception so JsRuntime::Dispatch still surfaces it.
+                    if (interval.has_value())
                     {
-                        const auto timeout = std::move(m_idMap.extract(id).mapped());
-                        function = std::move(timeout->function);
+                        Rearm(id, sequence, scheduledTime, *interval);
                     }
+
+                    error.ThrowAsJavaScriptException();
+                    return;
                 }
             }
 
-            if (function)
+            if (interval.has_value())
             {
-                function->Call({});
+                Rearm(id, sequence, scheduledTime, *interval);
             }
         });
     }
+
+    // Re-arms a repeating timeout. Called on the JS thread once the callback has
+    // returned, so a repeating timeout can never have more than one invocation
+    // queued at a time.
+    void TimeoutDispatcher::Rearm(TimeoutId id, uint64_t sequence, TimePoint scheduledTime, std::chrono::milliseconds interval)
+    {
+        std::unique_lock<std::recursive_mutex> lk{m_mutex};
+
+        const auto it = m_idMap.find(id);
+        if (it == m_idMap.end() || it->second->sequence != sequence)
+        {
+            // Cleared from within its own callback, or the id has since been
+            // reused by an unrelated timeout.
+            return;
+        }
+
+        // Anchor the next deadline to the previous scheduled time so that a long
+        // running callback does not accumulate drift, but never schedule into the
+        // past.
+        const auto now = Now();
+        auto nextTime = scheduledTime + interval;
+        if (nextTime < now)
+        {
+            nextTime = now;
+        }
+
+        const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time;
+        it->second->time = nextTime;
+        m_timeMap.insert({nextTime, it->second.get()});
+
+        if (nextTime <= earliestTime)
+        {
+            // The timer thread parks while m_timeMap is empty, which is the case
+            // whenever this timeout was the only one pending.
+            m_condVariable.notify_one();
+        }
+    }
 }

Polyfills/Scheduling/Source/TimeoutDispatcher.h

@@ -32,12 +32,14 @@ namespace Babylon::Polyfills::Internal
 
         TimeoutId NextTimeoutId();
         void ThreadFunction();
-        void CallFunction(TimeoutId id);
+        void CallFunction(TimeoutId id, uint64_t sequence);
+        void Rearm(TimeoutId id, uint64_t sequence, TimePoint scheduledTime, std::chrono::milliseconds interval);
 
         Babylon::JsRuntime& m_runtime;
         std::recursive_mutex m_mutex{};
         std::condition_variable_any m_condVariable{};
         TimeoutId m_lastTimeoutId{0};
+        uint64_t m_lastSequence{0};
         std::unordered_map<TimeoutId, std::unique_ptr<Timeout>> m_idMap;
         std::multimap<TimePoint, Timeout*> m_timeMap;
         std::atomic<bool> m_shutdown{false};

Tests/UnitTests/Scripts/tests.ts

@@ -614,6 +614,52 @@ describe("setInterval", function () {
             }
         }, 10);
     });
+
+    it("should not starve other queued work when the interval has no delay", function (done) {
+        // Regression test: a repeating timeout used to be re-armed on the timer
+        // thread immediately, before its callback had run on the JS thread. With a
+        // zero delay that produced an unbounded backlog of queued callbacks which
+        // starved every other item on the JS dispatch queue, so this setTimeout
+        // would never fire.
+        let finished = false;
+        const intervalId = setInterval(() => { });
+
+        const timeoutId = setTimeout(() => {
+            finished = true;
+            clearInterval(intervalId);
+            done();
+        }, 100);
+
+        setTimeout(() => {
+            if (!finished) {
+                clearInterval(intervalId);
+                clearTimeout(timeoutId);
+                done(new Error("setTimeout was starved by a zero delay setInterval"));
+            }
+        }, 2000);
+    });
+
+    it("should stop when cleared from within its own callback", function (done) {
+        // Exercises the re-arm path: a repeating timeout is now re-armed only
+        // after its callback returns, so a clear from inside the callback must
+        // win and no further ticks may occur.
+        let ticks = 0;
+        let id = 0;
+        id = setInterval(() => {
+            ticks++;
+            clearInterval(id);
+        }, 10);
+
+        setTimeout(() => {
+            try {
+                expect(ticks).to.equal(1);
+                done();
+            }
+            catch (e) {
+                done(e);
+            }
+        }, 200);
+    });
 });
 
 describe("clearInterval", function () {

Tests/UnitTests/Shared/Shared.cpp

@@ -111,6 +111,60 @@ TEST(JavaScript, All)
     EXPECT_EQ(exitCode, 0);
 }
 
+// The unit test host's UnhandledExceptionHandler fails the whole JavaScript
+// suite, so a throwing timer callback cannot be exercised from tests.ts. This
+// covers it natively instead.
+TEST(Scheduling, IntervalSurvivesThrowingCallback)
+{
+    // Regression: repeating timeouts are re-armed after their callback returns
+    // rather than before it runs, so an exception escaping a tick must not
+    // silently stop the interval. Browsers keep the interval running and report
+    // the error, and that is also what this dispatcher did previously.
+    std::promise<int32_t> tickCountPromise;
+    std::atomic<int32_t> unhandledErrorCount{0};
+
+    Babylon::AppRuntime::Options options{};
+    options.UnhandledExceptionHandler = [&unhandledErrorCount](const Napi::Error&) {
+        ++unhandledErrorCount;
+    };
+
+    Babylon::AppRuntime runtime{options};
+
+    runtime.Dispatch([&tickCountPromise](Napi::Env env) {
+        Babylon::Polyfills::Scheduling::Initialize(env);
+
+        auto reportTicks = Napi::Function::New(
+            env, [&tickCountPromise](const Napi::CallbackInfo& info) {
+                tickCountPromise.set_value(info[0].As<Napi::Number>().Int32Value());
+            },
+            "reportTicks");
+        env.Global().Set("reportTicks", reportTicks);
+    });
+
+    Babylon::ScriptLoader loader{runtime};
+    loader.Eval(R"(
+        var ticks = 0;
+        var id = setInterval(function () {
+            ticks++;
+            if (ticks === 3) {
+                clearInterval(id);
+                reportTicks(ticks);
+                return;
+            }
+            throw new Error('tick failed');
+        }, 1);
+    )",
+        "");
+
+    auto tickCountFuture{tickCountPromise.get_future()};
+    ASSERT_EQ(tickCountFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready)
+        << "the interval stopped after a tick threw";
+    EXPECT_EQ(tickCountFuture.get(), 3);
+
+    // The first two ticks threw, and those errors must still be surfaced.
+    EXPECT_EQ(unhandledErrorCount.load(), 2);
+}
+
 TEST(Console, Log)
 {
     Babylon::AppRuntime runtime{};