Merge AppRuntime and WorkQueue to fix race condition

Author: bghgary

Core/AppRuntime/CMakeLists.txt

@@ -2,10 +2,10 @@ set(SOURCES
     "Include/Babylon/Dispatchable.h"
     "Include/Babylon/AppRuntime.h"
     "Source/AppRuntime.cpp"
-    "Source/AppRuntime_${NAPI_JAVASCRIPT_ENGINE}.cpp"
-    "Source/AppRuntime_${BABYLON_NATIVE_PLATFORM}.${BABYLON_NATIVE_PLATFORM_IMPL_EXT}"
-    "Source/WorkQueue.cpp"
-    "Source/WorkQueue.h")
+    "Source/AppRuntimeImpl.h"
+    "Source/AppRuntimeImpl.cpp"
+    "Source/AppRuntimeImpl_${NAPI_JAVASCRIPT_ENGINE}.cpp"
+    "Source/AppRuntimeImpl_${BABYLON_NATIVE_PLATFORM}.${BABYLON_NATIVE_PLATFORM_IMPL_EXT}")
 
 add_library(AppRuntime ${SOURCES})
 warnings_as_errors(AppRuntime)

Core/AppRuntime/Include/Babylon/AppRuntime.h

@@ -1,15 +1,16 @@
 #pragma once
 
+#include <napi/env.h>
+
 #include "Dispatchable.h"
-#include <napi/napi.h>
 
 #include <memory>
 #include <functional>
 #include <exception>
 
 namespace Babylon
 {
-    class WorkQueue;
+    class AppRuntimeImpl;
 
     class AppRuntime final
     {
@@ -18,33 +19,16 @@ namespace Babylon
         AppRuntime(std::function<void(const std::exception&)> unhandledExceptionHandler);
         ~AppRuntime();
 
+        // Move semantics
+        AppRuntime(AppRuntime&&) noexcept;
+        AppRuntime& operator=(AppRuntime&&) noexcept;
+
         void Suspend();
         void Resume();
 
         void Dispatch(Dispatchable<void(Napi::Env)> callback);
 
     private:
-        // These three methods are the mechanism by which platform- and JavaScript-specific
-        // code can be "injected" into the execution of the JavaScript thread. These three
-        // functions are implemented in separate files, thus allowing implementations to be
-        // mixed and matched by the build system based on the platform and JavaScript engine
-        // being targeted, without resorting to virtuality. An important nuance of these
-        // functions is that they are all intended to call each other: RunPlatformTier MUST
-        // call RunEnvironmentTier, which MUST create the initial Napi::Env and pass it to
-        // Run. This arrangement allows not only for an arbitrary assemblage of platforms,
-        // but it also allows us to respect the requirement by certain platforms (notably V8)
-        // that certain program state be allocated and stored only on the stack.
-        void RunPlatformTier();
-        void RunEnvironmentTier(const char* executablePath = ".");
-        void Run(Napi::Env);
-
-        // This method is called from Dispatch to allow platform-specific code to add
-        // extra logic around the invocation of a dispatched callback.
-        void Execute(Dispatchable<void()> callback);
-
-        static void DefaultUnhandledExceptionHandler(const std::exception& error);
-
-        std::unique_ptr<WorkQueue> m_workQueue{};
-        std::function<void(const std::exception&)> m_unhandledExceptionHandler{};
+        std::unique_ptr<AppRuntimeImpl> m_impl;
     };
 }

Core/AppRuntime/Source/AppRuntime.cpp

@@ -1,65 +1,36 @@
 #include "AppRuntime.h"
-#include "WorkQueue.h"
-#include <Babylon/JsRuntime.h>
+#include "AppRuntimeImpl.h"
 
 namespace Babylon
 {
     AppRuntime::AppRuntime()
-        : AppRuntime{DefaultUnhandledExceptionHandler}
+        : m_impl{std::make_unique<AppRuntimeImpl>()}
     {
     }
 
     AppRuntime::AppRuntime(std::function<void(const std::exception&)> unhandledExceptionHandler)
-        : m_workQueue{std::make_unique<WorkQueue>([this] { RunPlatformTier(); })}
-        , m_unhandledExceptionHandler{unhandledExceptionHandler}
+        : m_impl{std::make_unique<AppRuntimeImpl>(unhandledExceptionHandler)}
     {
-        Dispatch([this](Napi::Env env) {
-            JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
-        });
     }
 
-    AppRuntime::~AppRuntime()
-    {
-        // Notify the JsRuntime on the JavaScript thread that the JavaScript
-        // runtime shutdown sequence has begun. The JsRuntimeScheduler will
-        // use this signal to gracefully cancel asynchronous operations.
-        Dispatch([](Napi::Env env) {
-            JsRuntime::NotifyDisposing(JsRuntime::GetFromJavaScript(env));
-        });
-    }
+    AppRuntime::~AppRuntime() = default;
 
-    void AppRuntime::Run(Napi::Env env)
-    {
-        m_workQueue->Run(env);
-    }
+    // Move semantics
+    AppRuntime::AppRuntime(AppRuntime&&) noexcept = default;
+    AppRuntime& AppRuntime::operator=(AppRuntime&&) noexcept = default;
 
     void AppRuntime::Suspend()
     {
-        m_workQueue->Suspend();
+        m_impl->Suspend();
     }
 
     void AppRuntime::Resume()
     {
-        m_workQueue->Resume();
+        m_impl->Resume();
     }
 
-    void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> func)
+    void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> callback)
     {
-        m_workQueue->Append([this, func{std::move(func)}](Napi::Env env) mutable {
-            Execute([this, env, func{std::move(func)}]() mutable {
-                try
-                {
-                    func(env);
-                }
-                catch (const std::exception& error)
-                {
-                    m_unhandledExceptionHandler(error);
-                }
-                catch (...)
-                {
-                    std::abort();
-                }
-            });
-        });
+        m_impl->Dispatch(std::move(callback));
     }
 }

Core/AppRuntime/Source/AppRuntimeImpl.cpp

@@ -0,0 +1,88 @@
+#include "AppRuntimeImpl.h"
+#include <Babylon/JsRuntime.h>
+
+namespace Babylon
+{
+    AppRuntimeImpl::AppRuntimeImpl(std::function<void(const std::exception&)> unhandledExceptionHandler)
+        : m_unhandledExceptionHandler{std::move(unhandledExceptionHandler)}
+        , m_thread{[this] { RunPlatformTier(); }}
+    {
+        Dispatch([this](Napi::Env env) {
+            JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
+        });
+    }
+
+    AppRuntimeImpl::~AppRuntimeImpl()
+    {
+        if (m_suspensionLock.has_value())
+        {
+            m_suspensionLock.reset();
+        }
+
+        Dispatch([this](Napi::Env env) {
+            // Notify the JsRuntime on the JavaScript thread that the JavaScript runtime shutdown sequence has
+            // begun. The JsRuntimeScheduler will use this signal to gracefully cancel asynchronous operations.
+            JsRuntime::NotifyDisposing(JsRuntime::GetFromJavaScript(env));
+
+            // Cancel on the JavaScript thread to signal the Run function to gracefully end. It must be
+            // dispatched and not canceled directly to ensure that existing work is executed and executed in
+            // the correct order.
+            m_cancellationSource.cancel();
+        });
+
+        m_thread.join();
+    }
+
+    void AppRuntimeImpl::Suspend()
+    {
+        auto suspensionMutex = std::make_shared<std::mutex>();
+        m_suspensionLock.emplace(*suspensionMutex);
+        Append([suspensionMutex = std::move(suspensionMutex)](Napi::Env) {
+            std::scoped_lock lock{*suspensionMutex};
+        });
+    }
+
+    void AppRuntimeImpl::Resume()
+    {
+        m_suspensionLock.reset();
+    }
+
+    void AppRuntimeImpl::Dispatch(Dispatchable<void(Napi::Env)> func)
+    {
+        Append([this, func{std::move(func)}](Napi::Env env) mutable {
+            Execute([this, env, func{std::move(func)}]() mutable {
+                try
+                {
+                    func(env);
+                }
+                catch (const std::exception& error)
+                {
+                    m_unhandledExceptionHandler(error);
+                }
+                catch (...)
+                {
+                    std::abort();
+                }
+            });
+        });
+    }
+
+    void AppRuntimeImpl::Run(Napi::Env env)
+    {
+        m_env = std::make_optional(env);
+
+        m_dispatcher.set_affinity(std::this_thread::get_id());
+
+        while (!m_cancellationSource.cancelled())
+        {
+            m_dispatcher.blocking_tick(m_cancellationSource);
+        }
+
+        // The dispatcher can be non-empty if something is dispatched after cancellation.
+        // For example, Chakra's JsSetPromiseContinuationCallback may potentially dispatch
+        // a continuation after cancellation.
+        m_dispatcher.clear();
+
+        m_env.reset();
+    }
+}

Core/AppRuntime/Source/AppRuntimeImpl.h

@@ -0,0 +1,68 @@
+#pragma once
+
+#include "AppRuntime.h"
+#include <optional>
+#include <mutex>
+#include <arcana/threading/dispatcher.h>
+
+namespace Babylon
+{
+    class AppRuntimeImpl
+    {
+    public:
+        AppRuntimeImpl(std::function<void(const std::exception&)> unhandledExceptionHandler = DefaultUnhandledExceptionHandler);
+        ~AppRuntimeImpl();
+
+        void Suspend();
+        void Resume();
+
+        void Dispatch(Dispatchable<void(Napi::Env)> func);
+
+    private:
+        static void DefaultUnhandledExceptionHandler(const std::exception& error);
+
+        template<typename CallableT>
+        void Append(CallableT callable)
+        {
+            // Manual dispatcher queueing requires a copyable CallableT, we use a shared pointer trick to make a
+            // copyable callable if necessary.
+            if constexpr (std::is_copy_constructible<CallableT>::value)
+            {
+                m_dispatcher.queue([this, callable = std::move(callable)]() {
+                    callable(m_env.value());
+                });
+            }
+            else
+            {
+                m_dispatcher.queue([this, callablePtr = std::make_shared<CallableT>(std::move(callable))]() {
+                    (*callablePtr)(m_env.value());
+                });
+            }
+        }
+
+        // These three methods are the mechanism by which platform- and JavaScript-specific
+        // code can be "injected" into the execution of the JavaScript thread. These three
+        // functions are implemented in separate files, thus allowing implementations to be
+        // mixed and matched by the build system based on the platform and JavaScript engine
+        // being targeted, without resorting to virtuality. An important nuance of these
+        // functions is that they are all intended to call each other: RunPlatformTier MUST
+        // call RunEnvironmentTier, which MUST create the initial Napi::Env and pass it to
+        // Run. This arrangement allows not only for an arbitrary assemblage of platforms,
+        // but it also allows us to respect the requirement by certain platforms (notably V8)
+        // that certain program state be allocated and stored only on the stack.
+        void RunPlatformTier();
+        void RunEnvironmentTier(const char* executablePath = ".");
+        void Run(Napi::Env);
+
+        // This method is called from Dispatch to allow platform-specific code to add
+        // extra logic around the invocation of a dispatched callback.
+        void Execute(Dispatchable<void()> callback);
+
+        std::function<void(const std::exception&)> m_unhandledExceptionHandler{};
+        std::optional<Napi::Env> m_env{};
+        std::optional<std::scoped_lock<std::mutex>> m_suspensionLock{};
+        arcana::cancellation_source m_cancellationSource{};
+        arcana::manual_dispatcher<128> m_dispatcher{};
+        std::thread m_thread{};
+    };
+}

Core/AppRuntime/Source/AppRuntime_Android.cpp renamed to Core/AppRuntime/Source/AppRuntimeImpl_Android.cpp

@@ -1,23 +1,23 @@
-#include "AppRuntime.h"
+#include "AppRuntimeImpl.h"
 #include <exception>
 #include <sstream>
 #include <android/log.h>
 
 namespace Babylon
 {
-    void AppRuntime::RunPlatformTier()
+    void AppRuntimeImpl::RunPlatformTier()
     {
         RunEnvironmentTier();
     }
 
-    void AppRuntime::DefaultUnhandledExceptionHandler(const std::exception& error)
+    void AppRuntimeImpl::DefaultUnhandledExceptionHandler(const std::exception& error)
     {
         std::stringstream ss{};
         ss << "Uncaught Error: " << error.what() << std::endl;
         __android_log_write(ANDROID_LOG_ERROR, "BabylonNative", ss.str().data());
     }
 
-    void AppRuntime::Execute(Dispatchable<void()> callback)
+    void AppRuntimeImpl::Execute(Dispatchable<void()> callback)
     {
         callback();
     }

Core/AppRuntime/Source/AppRuntime_Chakra.cpp renamed to Core/AppRuntime/Source/AppRuntimeImpl_Chakra.cpp

@@ -1,4 +1,4 @@
-#include "AppRuntime.h"
+#include "AppRuntimeImpl.h"
 
 #include <napi/env.h>
 
@@ -20,25 +20,23 @@ namespace Babylon
         }
     }
 
-    void AppRuntime::RunEnvironmentTier(const char*)
+    void AppRuntimeImpl::RunEnvironmentTier(const char*)
     {
         JsRuntimeHandle jsRuntime;
         ThrowIfFailed(JsCreateRuntime(JsRuntimeAttributeNone, nullptr, &jsRuntime));
         JsContextRef context;
         ThrowIfFailed(JsCreateContext(jsRuntime, &context));
         ThrowIfFailed(JsSetCurrentContext(context));
-        ThrowIfFailed(JsSetPromiseContinuationCallback(
-            [](JsValueRef task, void* callbackState) {
-                auto* pThis = reinterpret_cast<AppRuntime*>(callbackState);
-                ThrowIfFailed(JsAddRef(task, nullptr));
-                pThis->Dispatch([task](auto) {
-                    JsValueRef global;
-                    ThrowIfFailed(JsGetGlobalObject(&global));
-                    ThrowIfFailed(JsCallFunction(task, &global, 1, nullptr));
-                    ThrowIfFailed(JsRelease(task, nullptr));
-                });
-            },
-            this));
+        ThrowIfFailed(JsSetPromiseContinuationCallback([](JsValueRef task, void* callbackState) {
+            auto* pThis = reinterpret_cast<AppRuntimeImpl*>(callbackState);
+            ThrowIfFailed(JsAddRef(task, nullptr));
+            pThis->Dispatch([task](auto) {
+                JsValueRef global;
+                ThrowIfFailed(JsGetGlobalObject(&global));
+                ThrowIfFailed(JsCallFunction(task, &global, 1, nullptr));
+                ThrowIfFailed(JsRelease(task, nullptr));
+            });
+        }, this));
         ThrowIfFailed(JsProjectWinRTNamespace(L"Windows"));
 
 #if defined(_DEBUG)