Fix WorkQueue destructor deadlock by merging into AppRuntime

.gitignore

@@ -1 +1,2 @@
 /Build
+Tests/UnitTests/dist/

CMakeLists.txt

@@ -15,7 +15,7 @@ FetchContent_Declare(AndroidExtensions
     EXCLUDE_FROM_ALL)
 FetchContent_Declare(arcana.cpp
     GIT_REPOSITORY https://github.com/microsoft/arcana.cpp.git
-    GIT_TAG 7c6be8aaa29effbc8ee1a2217388fb6e399150d2
+    GIT_TAG d5dd03cc6dd138fc17c277f61abbe2bc388444af
     EXCLUDE_FROM_ALL)
 FetchContent_Declare(asio
     GIT_REPOSITORY https://github.com/chriskohlhoff/asio.git
@@ -113,6 +113,10 @@ endif()
 
 # --------------------------------------------------
 
+if(JSRUNTIMEHOST_TESTS)
+    add_compile_definitions(ARCANA_TESTING_HOOKS)
+endif()
+
 FetchContent_MakeAvailable_With_Message(arcana.cpp)
 set_property(TARGET arcana PROPERTY FOLDER Dependencies)
 

Core/AppRuntime/CMakeLists.txt

@@ -9,9 +9,7 @@ set(SOURCES
     "Include/Babylon/AppRuntime.h"
     "Source/AppRuntime.cpp"
     "Source/AppRuntime_${NAPI_JAVASCRIPT_ENGINE}.cpp"
-    "Source/AppRuntime_${JSRUNTIMEHOST_PLATFORM}.${IMPL_EXT}"
-    "Source/WorkQueue.cpp"
-    "Source/WorkQueue.h")
+    "Source/AppRuntime_${JSRUNTIMEHOST_PLATFORM}.${IMPL_EXT}")
 
 add_library(AppRuntime ${SOURCES})
 warnings_as_errors(AppRuntime)

Core/AppRuntime/Include/Babylon/AppRuntime.h

@@ -6,14 +6,18 @@
 
 #include <napi/utilities.h>
 
+#include <arcana/threading/dispatcher.h>
+
 #include <memory>
 #include <functional>
 #include <exception>
+#include <optional>
+#include <mutex>
+#include <thread>
+#include <type_traits>
 
 namespace Babylon
 {
-    class WorkQueue;
-
     class AppRuntime final
     {
     public:
@@ -43,6 +47,23 @@ namespace Babylon
         static void BABYLON_API DefaultUnhandledExceptionHandler(const Napi::Error& error);
 
     private:
+        template<typename CallableT>
+        void Append(CallableT callable)
+        {
+            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
@@ -61,7 +82,11 @@ namespace Babylon
         // extra logic around the invocation of a dispatched callback.
         void Execute(Dispatchable<void()> callback);
 
-        std::unique_ptr<WorkQueue> m_workQueue;
         Options m_options;
+        std::optional<Napi::Env> m_env{};
+        std::optional<std::scoped_lock<std::mutex>> m_suspensionLock{};
+        arcana::cancellation_source m_cancelSource{};
+        arcana::manual_dispatcher<128> m_dispatcher{};
+        std::thread m_thread;
     };
 }

Core/AppRuntime/Source/AppRuntime.cpp

@@ -1,5 +1,4 @@
 #include "AppRuntime.h"
-#include "WorkQueue.h"
 #include <cassert>
 
 namespace Babylon
@@ -10,8 +9,8 @@ namespace Babylon
     }
 
     AppRuntime::AppRuntime(Options options)
-        : m_workQueue{std::make_unique<WorkQueue>([this] { RunPlatformTier(); })}
-        , m_options{std::move(options)}
+        : m_options{std::move(options)}
+        , m_thread{[this] { RunPlatformTier(); }}
     {
         Dispatch([this](Napi::Env env) {
             JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
@@ -20,26 +19,53 @@ namespace Babylon
 
     AppRuntime::~AppRuntime()
     {
+        if (m_suspensionLock.has_value())
+        {
+            m_suspensionLock.reset();
+        }
+
+        // Dispatch cancellation as a work item so the worker thread processes it
+        // naturally via blocking_tick, avoiding the race condition where an external
+        // cancel signal can be missed by the condition variable wait.
+        Append([this](Napi::Env) {
+            m_cancelSource.cancel();
+        });
+
+        m_thread.join();
     }
 
     void AppRuntime::Run(Napi::Env env)
     {
-        m_workQueue->Run(env);
+        m_env = std::make_optional(env);
+
+        m_dispatcher.set_affinity(std::this_thread::get_id());
+
+        while (!m_cancelSource.cancelled())
+        {
+            m_dispatcher.blocking_tick(m_cancelSource);
+        }
+
+        // The dispatcher can be non-empty if something is dispatched after cancellation.
+        m_dispatcher.clear();
     }
 
     void AppRuntime::Suspend()
     {
-        m_workQueue->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 AppRuntime::Resume()
     {
-        m_workQueue->Resume();
+        m_suspensionLock.reset();
     }
 
     void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> func)
     {
-        m_workQueue->Append([this, func{std::move(func)}](Napi::Env env) mutable {
+        Append([this, func{std::move(func)}](Napi::Env env) mutable {
             Execute([this, env, func{std::move(func)}]() mutable {
                 try
                 {

Core/AppRuntime/Source/AppRuntime_JSI.cpp

@@ -1,5 +1,4 @@
 #include "AppRuntime.h"
-#include "WorkQueue.h"
 
 #include <napi/env.h>
 #include <V8JsiRuntime.h>
@@ -10,24 +9,23 @@ namespace
     class TaskRunnerAdapter : public v8runtime::JSITaskRunner
     {
     public:
-        TaskRunnerAdapter(Babylon::WorkQueue& workQueue)
-            : m_workQueue(workQueue)
+        TaskRunnerAdapter(Babylon::AppRuntime& appRuntime)
+            : m_appRuntime(appRuntime)
         {
         }
 
         void postTask(std::unique_ptr<v8runtime::JSITask> task) override
         {
-            std::shared_ptr<v8runtime::JSITask> shared_task(task.release());
-            m_workQueue.Append([shared_task2 = std::move(shared_task)](Napi::Env) {
-                shared_task2->run();
+            m_appRuntime.Dispatch([task = std::shared_ptr<v8runtime::JSITask>(std::move(task))](Napi::Env) {
+                task->run();
             });
         }
 
     private:
         TaskRunnerAdapter(const TaskRunnerAdapter&) = delete;
         TaskRunnerAdapter& operator=(const TaskRunnerAdapter&) = delete;
 
-        Babylon::WorkQueue& m_workQueue;
+        Babylon::AppRuntime& m_appRuntime;
     };
 }
 
@@ -37,7 +35,7 @@ namespace Babylon
     {
         v8runtime::V8RuntimeArgs args{};
         args.inspectorPort = 5643;
-        args.foreground_task_runner = std::make_shared<TaskRunnerAdapter>(*m_workQueue);
+        args.foreground_task_runner = std::make_shared<TaskRunnerAdapter>(*this);
         const auto runtime = v8runtime::makeV8Runtime(std::move(args));
 
         const auto env = Napi::Attach(*runtime);

Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt

@@ -9,6 +9,7 @@ get_filename_component(UNIT_TESTS_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../../.."
 get_filename_component(TESTS_DIR "${UNIT_TESTS_DIR}/.." ABSOLUTE)
 get_filename_component(REPO_ROOT_DIR "${TESTS_DIR}/.." ABSOLUTE)
 
+set(JSRUNTIMEHOST_TESTS ON CACHE BOOL "" FORCE)
 add_subdirectory(${REPO_ROOT_DIR} "${CMAKE_CURRENT_BINARY_DIR}/JsRuntimeHost")
 
 FetchContent_MakeAvailable_With_Message(googletest)