From c52b54822804e69c27a3225d0753193659a9e480 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 4 Aug 2026 12:10:29 -0700 Subject: [PATCH 1/2] Fix setInterval starving the JS dispatch queue A repeating timeout was re-armed on the timer thread immediately, before its callback had been run on the JS thread: 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); With a zero delay the re-inserted entry is scheduled at Now(), which is always already due, so the timer thread's wait_until never blocks. It spins, queueing callbacks onto the JS dispatch queue far faster than the JS thread can drain them. The queue is unbounded, so it grows without limit and every other item on it is starved arbitrarily long -- other timers, and native async completions posted through JsRuntimeScheduler. This is easy to hit: setInterval(fn) with no delay argument yields a delay of 0, and driving a render loop that way is a common pattern. In Babylon Native it made shader compilation completions never arrive, so scene readiness callbacks never fired and the app hung with no error. Re-arm repeating timeouts on the JS thread once the callback has actually returned, so an interval can have at most one invocation pending at a time, as in a browser. The next deadline is anchored to the previous scheduled time rather than the completion time so that a slow callback does not accumulate drift, and is clamped to not be in the past. A monotonic sequence number distinguishes a timeout from a later one that reuses the same id, so an in-flight callback can never re-arm its replacement, and a clear issued from within a callback is honoured. This also removes a null dereference: the old code read m_idMap[id]->interval with operator[], which default-constructs a null unique_ptr when the id is absent and then dereferences it. Zero-delay intervals still run at full speed; they simply no longer build a backlog. In a local benchmark a zero-delay interval reached 38,000 ticks in the 100ms before a competing setTimeout(100) fired, where previously that setTimeout never fired at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- .../Scheduling/Source/TimeoutDispatcher.cpp | 93 ++++++++++++++----- .../Scheduling/Source/TimeoutDispatcher.h | 3 +- Tests/UnitTests/Scripts/tests.ts | 46 +++++++++ 3 files changed, 119 insertions(+), 23 deletions(-) diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp index b869e23f..92127300 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp @@ -19,6 +19,10 @@ 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 function; @@ -26,8 +30,9 @@ namespace Babylon::Polyfills::Internal std::optional interval; - Timeout(TimeoutId id, std::shared_ptr function, TimePoint time, std::optional interval) + Timeout(TimeoutId id, uint64_t sequence, std::shared_ptr function, TimePoint time, std::optional 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(id, std::move(function), time, repeat ? std::make_optional(delay) : std::nullopt)}); + const auto result = m_idMap.insert({id, std::make_unique(id, ++m_lastSequence, std::move(function), time, repeat ? std::make_optional(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,25 +176,33 @@ 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 function{}; + std::optional interval{}; + TimePoint scheduledTime{}; { std::unique_lock 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 - { - const auto timeout = std::move(m_idMap.extract(id).mapped()); - function = std::move(timeout->function); - } + // 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); } } @@ -193,6 +210,38 @@ namespace Babylon::Polyfills::Internal { function->Call({}); } + + if (!interval.has_value()) + { + return; + } + + // Re-arm only now that the callback has completed. Anchor the next + // deadline to the previous scheduled time so a long-running callback + // does not accumulate drift, but never schedule into the past. + std::unique_lock 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. + return; + } + + 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) + { + m_condVariable.notify_one(); + } }); } } diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.h b/Polyfills/Scheduling/Source/TimeoutDispatcher.h index 98cbd289..0f1ce0d4 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.h +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.h @@ -32,12 +32,13 @@ namespace Babylon::Polyfills::Internal TimeoutId NextTimeoutId(); void ThreadFunction(); - void CallFunction(TimeoutId id); + void CallFunction(TimeoutId id, uint64_t sequence); 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> m_idMap; std::multimap m_timeMap; std::atomic m_shutdown{false}; diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..727f17ec 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/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 () { From 0c8666a8ee9eb0e4d925ced2869c5aa528a01b89 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 4 Aug 2026 13:48:33 -0700 Subject: [PATCH 2/2] Keep intervals running when a tick throws Re-arming after the callback introduced a behavior change: an exception escaping function->Call unwinds out of the dispatch lambda before the re-arm runs, so an interval stopped permanently the first time a tick threw. Previously the re-arm happened on the timer thread before the call, so a throwing tick was harmless. Browsers report the error and keep the interval running, so restore that. Catch Napi::Error around the call, re-arm, then re-raise it as a pending JS exception so JsRuntime::Dispatch's pending-exception check still surfaces it unchanged. The re-arm logic moves into a TimeoutDispatcher::Rearm helper so both the normal and the throwing path share it. Covered by a new native test, Scheduling.IntervalSurvivesThrowingCallback. It has to be native because the unit test host's UnhandledExceptionHandler fails the entire JavaScript suite, so a throwing timer cannot be exercised from tests.ts. Without the catch the test fails by timing out after the first throw. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- .../Scheduling/Source/TimeoutDispatcher.cpp | 81 ++++++++++++------- .../Scheduling/Source/TimeoutDispatcher.h | 1 + Tests/UnitTests/Shared/Shared.cpp | 54 +++++++++++++ 3 files changed, 109 insertions(+), 27 deletions(-) diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp index 92127300..1dea6585 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp @@ -208,40 +208,67 @@ namespace Babylon::Polyfills::Internal if (function) { - function->Call({}); + 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()) + { + Rearm(id, sequence, scheduledTime, *interval); + } + + error.ThrowAsJavaScriptException(); + return; + } } - if (!interval.has_value()) + if (interval.has_value()) { - return; + Rearm(id, sequence, scheduledTime, *interval); } + }); + } - // Re-arm only now that the callback has completed. Anchor the next - // deadline to the previous scheduled time so a long-running callback - // does not accumulate drift, but never schedule into the past. - std::unique_lock 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. - return; - } + // 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 lk{m_mutex}; - const auto now = Now(); - auto nextTime = scheduledTime + *interval; - if (nextTime < now) - { - nextTime = now; - } + 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; + } - const auto earliestTime = m_timeMap.empty() ? TimePoint::max() : m_timeMap.cbegin()->second->time; - it->second->time = nextTime; - m_timeMap.insert({nextTime, it->second.get()}); + // 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; + } - if (nextTime <= earliestTime) - { - m_condVariable.notify_one(); - } - }); + 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(); + } } } diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.h b/Polyfills/Scheduling/Source/TimeoutDispatcher.h index 0f1ce0d4..0ab4b135 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.h +++ b/Polyfills/Scheduling/Source/TimeoutDispatcher.h @@ -33,6 +33,7 @@ namespace Babylon::Polyfills::Internal TimeoutId NextTimeoutId(); void ThreadFunction(); 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{}; diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a920fa1f..d3f76131 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/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 tickCountPromise; + std::atomic 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().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{};