Merge with new timeout code with work queue fixes

Author: bghgary

Core/AppRuntime/Source/AppRuntime.cpp

@@ -12,7 +12,8 @@ namespace Babylon
         : m_workQueue{std::make_unique<WorkQueue>([this] { RunPlatformTier(); })}
         , m_unhandledExceptionHandler{unhandledExceptionHandler}
     {
-        Dispatch([this](Napi::Env env) {
+        Dispatch([this](Napi::Env env)
+        {
             JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
         });
     }
@@ -45,8 +46,10 @@ namespace Babylon
 
     void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> func)
     {
-        m_workQueue->Append([this, func{std::move(func)}](Napi::Env env) mutable {
-            Execute([this, env, func{std::move(func)}]() mutable {
+        m_workQueue->Append([this, func{std::move(func)}](Napi::Env env) mutable
+        {
+            Execute([this, env, func{std::move(func)}]() mutable
+            {
                 try
                 {
                     func(env);

Core/AppRuntime/Source/WorkQueue.cpp

@@ -48,8 +48,10 @@ namespace Babylon
             m_dispatcher.blocking_tick(m_cancellationSource);
         }
 
-        // There should not be any outstanding work during the shutdown sequence
-        // which should be the only way exit the while loop above.
+        // Drain the queue to complete work dispatched after cancellation.
+        m_dispatcher.tick(arcana::cancellation::none());
+
+        // There should no longer be any outstanding work once the queue is drained.
         assert(m_dispatcher.empty());
     }
 }

Core/AppRuntime/Source/WorkQueue.h

@@ -22,7 +22,7 @@ namespace Babylon
             if (m_cancellationSource.cancelled())
             {
                 // There is likely a coding error if this exception is thrown.
-                throw std::runtime_error{"Cannot append to the work queue after it is canceled"};
+                throw std::runtime_error{"Cannot append to the work queue while shutting down"};
             }
 
             // Manual dispatcher queueing requires a copyable CallableT, we use a shared pointer trick to make a
@@ -57,6 +57,6 @@ namespace Babylon
         arcana::cancellation_source m_cancellationSource{};
         arcana::manual_dispatcher<128> m_dispatcher{};
 
-        std::thread m_thread;
+        std::thread m_thread{};
     };
 }

Core/JsRuntime/Include/Babylon/JsRuntime.h

@@ -42,12 +42,20 @@ namespace Babylon
         // Gets the JsRuntime from the given N-API environment.
         static JsRuntime& GetFromJavaScript(Napi::Env);
 
+        struct IDisposingCallback
+        {
+            virtual void Disposing() = 0;
+        };
+
         // Notifies the JsRuntime that the JavaScript environment will begin shutting down.
         // Calling this function will signal callbacks registered with RegisterDisposing.
         static void NotifyDisposing(JsRuntime&);
 
         // Registers a callback for when the JavaScript environment will begin shutting down.
-        static void RegisterDisposing(JsRuntime&, std::function<void()>);
+        static void RegisterDisposing(JsRuntime&, IDisposingCallback*);
+
+        // Unregisters a callback for when the JavaScript environment will begin shutting down.
+        static void UnregisterDisposing(JsRuntime&, IDisposingCallback*);
 
         // Dispatches work onto the JavaScript thread and provides access to the N-API
         // environment.
@@ -58,6 +66,6 @@ namespace Babylon
 
         std::mutex m_mutex{};
         DispatchFunctionT m_dispatchFunction{};
-        std::vector<std::function<void()>> m_disposingCallbacks{};
+        std::vector<IDisposingCallback*> m_disposingCallbacks{};
     };
 }

Core/JsRuntime/Include/Babylon/JsRuntimeScheduler.h

@@ -53,22 +53,26 @@ namespace Babylon
     //      the continuation, must capture a persistent reference to the N-API object itself to prevent the GC from
     //      collecting the N-API object during the asynchronous operation. Failing to do so will result in a hang
     //      when `Rundown()` is called in the N-API class destructor.
-    class JsRuntimeScheduler
+    class JsRuntimeScheduler : public JsRuntime::IDisposingCallback
     {
     public:
         explicit JsRuntimeScheduler(JsRuntime& runtime)
             : m_runtime{&runtime}
             , m_scheduler{*this}
         {
-            JsRuntime::RegisterDisposing(*m_runtime, [this]()
-            {
-                m_runtime = nullptr;
-            });
+            std::scoped_lock lock{m_mutex};
+            JsRuntime::RegisterDisposing(*m_runtime, this);
         }
 
         ~JsRuntimeScheduler()
         {
             assert(m_count == 0 && "Schedulers for the JavaScript thread are not yet invoked");
+
+            std::scoped_lock lock{m_mutex};
+            if (m_runtime != nullptr)
+            {
+                JsRuntime::UnregisterDisposing(*m_runtime, this);
+            }
         }
 
         // Wait until all of the schedulers are invoked.
@@ -88,6 +92,12 @@ namespace Babylon
         }
 
     private:
+        void Disposing() override
+        {
+            std::scoped_lock lock{m_mutex};
+            m_runtime = nullptr;
+        }
+
         class SchedulerImpl
         {
         public:
@@ -108,6 +118,8 @@ namespace Babylon
         template<typename CallableT>
         void Dispatch(CallableT&& callable)
         {
+            std::scoped_lock lock{m_mutex};
+
             if (m_runtime != nullptr)
             {
                 m_runtime->Dispatch([callable{std::forward<CallableT>(callable)}](Napi::Env)
@@ -123,7 +135,10 @@ namespace Babylon
             --m_count;
         }
 
-        JsRuntime* m_runtime;
+        mutable std::mutex m_mutex{};
+        JsRuntime* m_runtime{};
+        std::function<void()> m_disposingCallback{};
+
         SchedulerImpl m_scheduler;
         std::atomic<int> m_count{0};
         arcana::manual_dispatcher<128> m_disposingDispatcher{};

Core/JsRuntime/Source/JsRuntime.cpp

@@ -1,4 +1,5 @@
 #include "JsRuntime.h"
+#include <cassert>
 
 namespace Babylon
 {
@@ -43,15 +44,27 @@ namespace Babylon
     void JsRuntime::NotifyDisposing(JsRuntime& runtime)
     {
         auto callbacks = std::move(runtime.m_disposingCallbacks);
-        for (const auto& callback : callbacks)
+        for (auto* callback : callbacks)
         {
-            callback();
+            callback->Disposing();
         }
     }
 
-    void JsRuntime::RegisterDisposing(JsRuntime& runtime, std::function<void()> callback)
+    void JsRuntime::RegisterDisposing(JsRuntime& runtime, IDisposingCallback* callback)
     {
-        runtime.m_disposingCallbacks.push_back(std::move(callback));
+        auto& callbacks = runtime.m_disposingCallbacks;
+        assert(std::find(callbacks.begin(), callbacks.end(), callback) == callbacks.end());
+        callbacks.push_back(callback);
+    }
+
+    void JsRuntime::UnregisterDisposing(JsRuntime& runtime, IDisposingCallback* callback)
+    {
+        auto& callbacks = runtime.m_disposingCallbacks;
+        auto it = std::find(callbacks.begin(), callbacks.end(), callback);
+        if (it != callbacks.end())
+        {
+            callbacks.erase(it);
+        }
     }
 
     void JsRuntime::Dispatch(std::function<void(Napi::Env)> function)

Plugins/NativeEngine/Source/NativeEngine.cpp

@@ -1067,26 +1067,25 @@ namespace Babylon
         });
     }
 
-    void NativeEngine::CopyTexture(const Napi::CallbackInfo& /*info*/)
-    {
-        //const auto textureDestination = info[0].As<Napi::Pointer<Graphics::Texture>>().Get();
-        //const auto textureSource = info[1].As<Napi::Pointer<Graphics::Texture>>().Get();
-
-        // TODO: this code makes no sense
-        //arcana::make_task(m_graphicsUpdate.Scheduler(), m_cancellationSource, [this, textureDestination, textureSource]()
-        //{
-        //    return arcana::make_task(m_runtimeScheduler.Get(), m_cancellationSource, [this, textureDestination, textureSource, updateToken = m_graphicsUpdate.GetUpdateToken()]() mutable
-        //    {
-        //        bgfx::Encoder* encoder = updateToken.GetEncoder();
-        //        GetBoundFrameBuffer(*encoder).Blit(*encoder, textureDestination->Handle(), 0, 0, textureSource->Handle());
-        //    }).then(arcana::inline_scheduler, m_cancellationSource, [this, thisRef = Napi::Persistent(info.Env())](const arcana::expected<void, std::exception_ptr>& result)
-        //    {
-        //        if (result.has_error())
-        //        {
-        //            Napi::Error::New(Env(), result.error()).ThrowAsJavaScriptException();
-        //        }
-        //    });
-        //});
+    void NativeEngine::CopyTexture(const Napi::CallbackInfo& info)
+    {
+        const auto textureDestination = info[0].As<Napi::Pointer<Graphics::Texture>>().Get();
+        const auto textureSource = info[1].As<Napi::Pointer<Graphics::Texture>>().Get();
+
+        arcana::make_task(m_update.Scheduler(), m_cancellationSource, [this, textureDestination, textureSource]() mutable
+        {
+            return arcana::make_task(m_runtimeScheduler.Get(), m_cancellationSource, [this, textureDestination, textureSource, updateToken = m_update.GetUpdateToken()]() mutable
+            {
+                bgfx::Encoder* encoder = updateToken.GetEncoder();
+                GetBoundFrameBuffer(*encoder).Blit(*encoder, textureDestination->Handle(), 0, 0, textureSource->Handle());
+            }).then(arcana::inline_scheduler, m_cancellationSource, [this](const arcana::expected<void, std::exception_ptr>& result)
+            {
+                if (result.has_error())
+                {
+                    Napi::Error::New(Env(), result.error()).ThrowAsJavaScriptException();
+                }
+            });
+        });
     }
 
     void NativeEngine::LoadRawTexture(const Napi::CallbackInfo& info)

Polyfills/Window/Source/TimeoutDispatcher.cpp

@@ -34,23 +34,26 @@ namespace Babylon::Polyfills::Internal
         Timeout(Timeout&&) = delete;
     };
 
-    TimeoutDispatcher::TimeoutDispatcher(Babylon::JsRuntimeScheduler& runtimeScheduler)
-        : m_runtimeScheduler{runtimeScheduler}
+    TimeoutDispatcher::TimeoutDispatcher(Babylon::JsRuntime& runtime)
+        : m_runtimeScheduler{runtime}
         , m_thread{std::thread{&TimeoutDispatcher::ThreadFunction, this}}
     {
     }
 
     TimeoutDispatcher::~TimeoutDispatcher()
     {
         {
-            std::unique_lock<std::mutex> lk{m_mutex};
+            std::unique_lock<std::mutex> lock{m_mutex};
             m_idMap.clear();
             m_timeMap.clear();
         }
 
-        m_shutdown = true;
+        m_cancellationSource.cancel();
         m_condVariable.notify_one();
         m_thread.join();
+
+        // Wait for async operations to complete.
+        m_runtimeScheduler.Rundown();
     }
 
     TimeoutDispatcher::TimeoutId TimeoutDispatcher::Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay)
@@ -71,7 +74,8 @@ namespace Babylon::Polyfills::Internal
 
         if (time <= earliestTime)
         {
-            m_runtimeScheduler.Get()([this]() {
+            m_runtimeScheduler.Get()([this]()
+            {
                 m_condVariable.notify_one();
             });
         }
@@ -123,11 +127,11 @@ namespace Babylon::Polyfills::Internal
 
     void TimeoutDispatcher::ThreadFunction()
     {
-        while (!m_shutdown)
+        while (!m_cancellationSource.cancelled())
         {
-            std::unique_lock<std::mutex> lk{m_mutex};
-            TimePoint nextTimePoint{};
+            std::unique_lock<std::mutex> lock{m_mutex};
 
+            TimePoint nextTimePoint{};
             while (!m_timeMap.empty())
             {
                 nextTimePoint = m_timeMap.begin()->second->time;
@@ -136,7 +140,7 @@ namespace Babylon::Polyfills::Internal
                     break;
                 }
 
-                m_condVariable.wait_until(lk, nextTimePoint);
+                m_condVariable.wait_until(lock, nextTimePoint);
             }
 
             while (!m_timeMap.empty() && m_timeMap.begin()->second->time == nextTimePoint)
@@ -147,9 +151,9 @@ namespace Babylon::Polyfills::Internal
                 m_idMap.erase(timeout->id);
             }
 
-            while (!m_shutdown && m_timeMap.empty())
+            while (!m_cancellationSource.cancelled() && m_timeMap.empty())
             {
-                m_condVariable.wait(lk);
+                m_condVariable.wait(lock);
             }
         }
     }
@@ -159,7 +163,9 @@ namespace Babylon::Polyfills::Internal
         if (function)
         {
             m_runtimeScheduler.Get()([function = std::move(function)]()
-                { function->Call({}); });
+            {
+                function->Call({});
+            });
         }
     }
 }

Polyfills/Window/Source/TimeoutDispatcher.h

@@ -19,7 +19,7 @@ namespace Babylon::Polyfills::Internal
         struct Timeout;
 
     public:
-        TimeoutDispatcher(Babylon::JsRuntimeScheduler& runtimeScheduler);
+        TimeoutDispatcher(Babylon::JsRuntime& runtime);
         ~TimeoutDispatcher();
 
         TimeoutId Dispatch(std::shared_ptr<Napi::FunctionReference> function, std::chrono::milliseconds delay);
@@ -32,13 +32,14 @@ namespace Babylon::Polyfills::Internal
         void ThreadFunction();
         void CallFunction(std::shared_ptr<Napi::FunctionReference> function);
 
-        Babylon::JsRuntimeScheduler& m_runtimeScheduler;
-        std::mutex m_mutex{};
+        Babylon::JsRuntimeScheduler m_runtimeScheduler;
+
+        mutable std::mutex m_mutex{};
         std::condition_variable m_condVariable{};
         TimeoutId m_lastTimeoutId{0};
-        std::unordered_map<TimeoutId, std::unique_ptr<Timeout>> m_idMap;
-        std::multimap<TimePoint, Timeout*> m_timeMap;
-        std::atomic<bool> m_shutdown{false};
-        std::thread m_thread;
+        std::unordered_map<TimeoutId, std::unique_ptr<Timeout>> m_idMap{};
+        std::multimap<TimePoint, Timeout*> m_timeMap{};
+        arcana::cancellation_source m_cancellationSource{};
+        std::thread m_thread{};
     };
 }

Polyfills/Window/Source/Window.cpp

@@ -71,19 +71,10 @@ namespace Babylon::Polyfills::Internal
 
     Window::Window(const Napi::CallbackInfo& info)
         : Napi::ObjectWrap<Window>{info}
-        , m_runtimeScheduler{JsRuntime::GetFromJavaScript(info.Env())}
-        , m_timeoutDispatcher{m_runtimeScheduler}
+        , m_timeoutDispatcher{JsRuntime::GetFromJavaScript(info.Env())}
     {
     }
 
-    Window::~Window()
-    {
-        m_cancelSource.cancel();
-
-        // Wait for async operations to complete.
-        m_runtimeScheduler.Rundown();
-    }
-
     Napi::Value Window::SetTimeout(const Napi::CallbackInfo& info)
     {
         auto& window = *static_cast<Window*>(info.Data());