Add setInterval and tests (#104)

ryantrem · Ryan Tremblay · web-flow · commit 6c1fba46d854 · 2025-02-20T17:58:50.000Z
This PR adds a `setInterval` implementation. This is largely copied over from the related change in Babylon Native: BabylonJS/BabylonNative@b2d5b2e#diff-23ceed54341331df9f4df6a9141772cd3613446bef20c7d96c62f5bebdbcf426 However, with the tests, I did discover a pre-existing timing/threading problem. Specifically, if the callback was scheduled in the JS thread via `m_runtime.Dispatch`, it is possible that the JS code could call `clearTimeout` before the dispatched callback is executed. This would result in the callback being called even after a call to `clearTimeout`. This became more obvious with `setInterval`, especially with small intervals. I changed the logic so that now the entries are removed from the time map on the timer thread, but the entries in the timer id map are left in place and removed only by calls on the JS thread - either a call to `clearTimeout`, or in the callback on the JS thread. This way, we know for sure in the callback on the JS thread whether the timer has been cleared. --------- Co-authored-by: Ryan Tremblay <ryantrem@microsoft.com>
diff --git a/.github/jobs/ios.yml b/.github/jobs/ios.yml
@@ -6,7 +6,7 @@ parameters:
 
 jobs:
 - job: ${{parameters.name}}
-  timeoutInMinutes: 15
+  timeoutInMinutes: 30
 
   pool:
     vmImage: ${{parameters.vmImage}}
diff --git a/Polyfills/Scheduling/Source/Scheduling.cpp b/Polyfills/Scheduling/Source/Scheduling.cpp
@@ -4,8 +4,10 @@ namespace
 {
     constexpr auto JS_SET_TIMEOUT_NAME = "setTimeout";
     constexpr auto JS_CLEAR_TIMEOUT_NAME = "clearTimeout";
+    constexpr auto JS_SET_INTERVAL_NAME = "setInterval";
+    constexpr auto JS_CLEAR_INTERVAL_NAME = "clearInterval";
 
-    Napi::Value SetTimeout(const Napi::CallbackInfo& info, Babylon::Polyfills::Internal::TimeoutDispatcher& timeoutDispatcher)
+    Napi::Value SetTimeout(const Napi::CallbackInfo& info, Babylon::Polyfills::Internal::TimeoutDispatcher& timeoutDispatcher, bool repeat)
     {
         auto function =
             info[0].IsFunction()
@@ -14,7 +16,7 @@ namespace
 
         auto delay = std::chrono::milliseconds{info[1].ToNumber().Int32Value()};
 
-        return Napi::Value::From(info.Env(), timeoutDispatcher.Dispatch(function, delay));
+        return Napi::Value::From(info.Env(), timeoutDispatcher.Dispatch(function, delay, repeat));
     }
 
     void ClearTimeout(const Napi::CallbackInfo& info, Babylon::Polyfills::Internal::TimeoutDispatcher& timeoutDispatcher)
@@ -33,14 +35,14 @@ namespace Babylon::Polyfills::Scheduling
     void BABYLON_API Initialize(Napi::Env env)
     {
         auto global = env.Global();
+        auto timeoutDispatcher = std::make_shared<Internal::TimeoutDispatcher>(JsRuntime::GetFromJavaScript(env));
+
         if (global.Get(JS_SET_TIMEOUT_NAME).IsUndefined() && global.Get(JS_CLEAR_TIMEOUT_NAME).IsUndefined())
         {
-            auto timeoutDispatcher = std::make_shared<Internal::TimeoutDispatcher>(JsRuntime::GetFromJavaScript(env));
-
             global.Set(JS_SET_TIMEOUT_NAME,
                 Napi::Function::New(
                     env, [timeoutDispatcher](const Napi::CallbackInfo& info) {
-                        return SetTimeout(info, *timeoutDispatcher);
+                        return SetTimeout(info, *timeoutDispatcher, false);
                     },
                     JS_SET_TIMEOUT_NAME));
 
@@ -51,5 +53,22 @@ namespace Babylon::Polyfills::Scheduling
                     },
                     JS_CLEAR_TIMEOUT_NAME));
         }
+
+        if (global.Get(JS_SET_INTERVAL_NAME).IsUndefined() && global.Get(JS_CLEAR_INTERVAL_NAME).IsUndefined())
+        {
+            global.Set(JS_SET_INTERVAL_NAME,
+                Napi::Function::New(
+                    env, [timeoutDispatcher](const Napi::CallbackInfo& info) {
+                        return SetTimeout(info, *timeoutDispatcher, true);
+                    },
+                    JS_SET_INTERVAL_NAME));
+
+            global.Set(JS_CLEAR_INTERVAL_NAME,
+                Napi::Function::New(
+                    env, [timeoutDispatcher](const Napi::CallbackInfo& info) {
+                        ClearTimeout(info, *timeoutDispatcher);
+                    },
+                    JS_CLEAR_INTERVAL_NAME));
+        }
     }
 }
diff --git a/Polyfills/Scheduling/Source/Scheduling.h b/Polyfills/Scheduling/Source/Scheduling.h
@@ -12,14 +12,5 @@ namespace Babylon::Polyfills::Internal
     {
     public:
         static void Initialize(Napi::Env env);
-
-        Scheduling(const Napi::CallbackInfo& info);
-
-    private:
-        JsRuntime& m_runtime;
-        std::optional<TimeoutDispatcher> m_timeoutDispatcher;
-
-        static Napi::Value SetTimeout(const Napi::CallbackInfo& info);
-        static void ClearTimeout(const Napi::CallbackInfo& info);
     };
 }
diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp b/Polyfills/Scheduling/Source/TimeoutDispatcher.cpp
@@ -1,6 +1,7 @@
 #include "TimeoutDispatcher.h"
 
 #include <cassert>
+#include <optional>
 
 namespace Babylon::Polyfills::Internal
 {
@@ -23,10 +24,13 @@ namespace Babylon::Polyfills::Internal
 
         TimePoint time;
 
-        Timeout(TimeoutId id, 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)
             : id{id}
             , function{std::move(function)}
             , time{time}
+            , interval{interval}
         {
         }
 
@@ -43,7 +47,7 @@ namespace Babylon::Polyfills::Internal
     TimeoutDispatcher::~TimeoutDispatcher()
     {
         {
-            std::unique_lock<std::mutex> lk{m_mutex};
+            std::unique_lock<std::recursive_mutex> lk{m_mutex};
             m_idMap.clear();
             m_timeMap.clear();
         }
@@ -53,43 +57,47 @@ namespace Babylon::Polyfills::Internal
         m_thread.join();
     }
 
-    TimeoutDispatcher::TimeoutId TimeoutDispatcher::Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay)
+    TimeoutDispatcher::TimeoutId TimeoutDispatcher::Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat)
+    {
+        return DispatchImpl(function, delay, repeat, 0);
+    }
+
+    TimeoutDispatcher::TimeoutId TimeoutDispatcher::DispatchImpl(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat, TimeoutId id)
     {
         if (delay.count() < 0)
         {
             delay = std::chrono::milliseconds{0};
         }
 
-        std::unique_lock<std::mutex> lk{m_mutex};
+        std::unique_lock<std::recursive_mutex> lk{m_mutex};
 
-        const auto id = NextTimeoutId();
-        const auto earliestTime = m_timeMap.empty() ? TimePoint::max()
-                                                    : m_timeMap.cbegin()->second->time;
+        if (id == 0)
+        {
+            id = NextTimeoutId();
+        }
+        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)});
+        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)});
         m_timeMap.insert({time, result.first->second.get()});
 
         if (time <= earliestTime)
         {
-            m_runtime.Dispatch([this](Napi::Env) {
-                m_condVariable.notify_one();
-            });
+            m_condVariable.notify_one();
         }
 
         return id;
     }
 
     void TimeoutDispatcher::Clear(TimeoutId id)
     {
-        std::unique_lock<std::mutex> lk{m_mutex};
+        std::unique_lock<std::recursive_mutex> lk{m_mutex};
         const auto itId = m_idMap.find(id);
         if (itId != m_idMap.end())
         {
             const auto& timeout = itId->second;
             const auto timeRange = m_timeMap.equal_range(timeout->time);
 
-            assert(timeRange.first != m_timeMap.end() && "m_idMap and m_timeMap are out of sync");
-
+            // Remove any pending entries that have not yet been dispatched.
             for (auto itTime = timeRange.first; itTime != timeRange.second; itTime++)
             {
                 if (itTime->second->id == id)
@@ -125,7 +133,7 @@ namespace Babylon::Polyfills::Internal
     {
         while (!m_shutdown)
         {
-            std::unique_lock<std::mutex> lk{m_mutex};
+            std::unique_lock<std::recursive_mutex> lk{m_mutex};
             TimePoint nextTimePoint{};
 
             while (!m_timeMap.empty())
@@ -141,11 +149,15 @@ namespace Babylon::Polyfills::Internal
 
             while (!m_timeMap.empty() && m_timeMap.begin()->second->time == nextTimePoint)
             {
-                const auto* timeout = m_timeMap.begin()->second;
-                auto function = std::move(timeout->function);
+                const auto id = m_timeMap.begin()->second->id;
                 m_timeMap.erase(m_timeMap.begin());
-                m_idMap.erase(timeout->id);
-                CallFunction(std::move(function));
+                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);
             }
 
             while (!m_shutdown && m_timeMap.empty())
@@ -155,13 +167,32 @@ namespace Babylon::Polyfills::Internal
         }
     }
 
-    void TimeoutDispatcher::CallFunction(std::shared_ptr<Napi::FunctionReference> function)
+    void TimeoutDispatcher::CallFunction(TimeoutId id)
     {
-        if (function)
-        {
-            m_runtime.Dispatch([function = std::move(function)](Napi::Env) {
+        m_runtime.Dispatch([id, this](Napi::Env) {
+            std::shared_ptr<Napi::FunctionReference> function{};
+            {
+                std::unique_lock<std::recursive_mutex> lk{m_mutex};
+                const auto it = m_idMap.find(id);
+                if (it != m_idMap.end())
+                {
+                    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);
+                    }
+                }
+            }
+
+            if (function)
+            {
                 function->Call({});
-            });
-        }
+            }
+        });
     }
 }
diff --git a/Polyfills/Scheduling/Source/TimeoutDispatcher.h b/Polyfills/Scheduling/Source/TimeoutDispatcher.h
@@ -22,19 +22,21 @@ namespace Babylon::Polyfills::Internal
         TimeoutDispatcher(Babylon::JsRuntime& runtime);
         ~TimeoutDispatcher();
 
-        TimeoutId Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay);
+        TimeoutId Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat = false);
         void Clear(TimeoutId id);
 
     private:
         using TimePoint = std::chrono::time_point<std::chrono::steady_clock, std::chrono::microseconds>;
 
+        TimeoutId DispatchImpl(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay, bool repeat, TimeoutId id);
+
         TimeoutId NextTimeoutId();
         void ThreadFunction();
-        void CallFunction(std::shared_ptr<Napi::FunctionReference> function);
+        void CallFunction(TimeoutId id);
 
         Babylon::JsRuntime& m_runtime;
-        std::mutex m_mutex{};
-        std::condition_variable m_condVariable{};
+        std::recursive_mutex m_mutex{};
+        std::condition_variable_any m_condVariable{};
         TimeoutId m_lastTimeoutId{0};
         std::unordered_map<TimeoutId, std::unique_ptr<Timeout>> m_idMap;
         std::multimap<TimePoint, Timeout*> m_timeMap;
diff --git a/Tests/UnitTests/Scripts/tests.js b/Tests/UnitTests/Scripts/tests.js
@@ -299,20 +299,84 @@ describe("setTimeout", function () {
 });
 
 describe("clearTimeout", function () {
-    this.timeout(0);
+    this.timeout(1000);
+
     it("should stop the timeout matching the given timeout id", function (done) {
         const id = setTimeout(() => {
             done(new Error("Timeout was not cleared"));
         }, 0);
         clearTimeout(id);
         setTimeout(done, 100);
     });
+
     it("should do nothing if the given timeout id is undefined", function (done) {
         setTimeout(() => { done(); }, 0);
         clearTimeout(undefined);
     });
+
+    it("should be interchangeable with clearInterval", function (done) {
+        const id = setTimeout(() => {
+            done(new Error("Interval was not cleared"));
+        }, 0);
+        clearInterval(id);
+        setTimeout(done, 100);
+    });
 });
 
+describe("setInterval", function () {
+    this.timeout(1000);
+
+    it("should return an id greater than zero", function () {
+        const id = setInterval(() => { }, 0);
+        clearInterval(id);
+        expect(id).to.be.greaterThan(0);
+    });
+
+    it("should call the given function at the given interval", function (done) {
+        let startTime = new Date().getTime();
+        let tickCount = 0;
+        const id = setInterval(() => {
+            try {
+                tickCount++;
+                expect(new Date().getTime() - startTime).to.be.at.least(tickCount * 10);
+                if (tickCount > 2) {
+                    clearInterval(id);
+                    done();
+                }
+            }
+            catch (e) {
+                console.log(`finished with error: ${e}`);
+                clearInterval(id);
+                done(e);
+            }
+        }, 10);
+    });
+});
+
+describe("clearInterval", function () {
+    this.timeout(1000);
+
+    it("should stop the interval matching the given interval id", function (done) {
+        const id = setInterval(() => {
+            done(new Error("Interval was not cleared"));
+        }, 0);
+        clearInterval(id);
+        setTimeout(done, 100);
+    });
+
+    it("should do nothing if the given interval id is undefined", function (done) {
+        setTimeout(() => { done(); }, 0);
+        clearInterval(undefined);
+    });
+
+    it("should be interchangeable with clearTimeout", function (done) {
+        const id = setInterval(() => {
+            done(new Error("Interval was not cleared"));
+        }, 0);
+        clearTimeout(id);
+        setTimeout(done, 100);
+    });
+});
 
 // Websocket
 if (hostPlatform !== "Unix") {
@@ -662,6 +726,7 @@ describe("Console", function () {
         expect(() => console.log("%d %f %s", 0 / 0, 0 / 0, 0 / 0)).to.not.throw();
     });
 });
+
 function runTests() {
     mocha.run(failures => {
         // Test program will wait for code to be set before exiting
