diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp index b869e23f..1dea6585 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,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 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 + // 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 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(); + } + } } diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.h b/Polyfills/Scheduling/Source/TimeoutDispatcher.h index 98cbd289..0ab4b135 100644 --- a/Polyfills/Scheduling/Source/TimeoutDispatcher.h +++ b/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> 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 () { 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{};