Deterministic test proving AppRuntime destructor deadlock

CMakeLists.txt

@@ -14,8 +14,8 @@ FetchContent_Declare(AndroidExtensions
     GIT_TAG 2d5af72259cc73e5f249d3c99bee2010be9cb042
     EXCLUDE_FROM_ALL)
 FetchContent_Declare(arcana.cpp
-    GIT_REPOSITORY https://github.com/microsoft/arcana.cpp.git
-    GIT_TAG 7c6be8aaa29effbc8ee1a2217388fb6e399150d2
+    GIT_REPOSITORY https://github.com/bghgary/arcana.cpp.git
+    GIT_TAG d5804f8a51de0e37d78bdea469c21823eb7d27ce
     EXCLUDE_FROM_ALL)
 FetchContent_Declare(asio
     GIT_REPOSITORY https://github.com/chriskohlhoff/asio.git
@@ -110,6 +110,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,17 @@
 
 #include <napi/utilities.h>
 
+#include <arcana/threading/dispatcher.h>
+
 #include <memory>
 #include <functional>
 #include <exception>
+#include <optional>
+#include <mutex>
+#include <thread>
 
 namespace Babylon
 {
-    class WorkQueue;
-
     class AppRuntime final
     {
     public:
@@ -43,6 +46,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 +81,19 @@ 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;
+
+#ifdef ARCANA_TESTING_HOOKS
+    public:
+        void SetBeforeWaitCallback(std::function<void()> callback)
+        {
+            m_dispatcher.set_before_wait_callback(std::move(callback));
+        }
+#endif
     };
 }

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,49 @@ namespace Babylon
 
     AppRuntime::~AppRuntime()
     {
+        if (m_suspensionLock.has_value())
+        {
+            m_suspensionLock.reset();
+        }
+
+        m_cancelSource.cancel();
+        m_dispatcher.cancelled();
+
+        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/CMakeLists.txt

@@ -44,7 +44,8 @@ elseif(UNIX AND NOT ANDROID)
 endif()
 
 add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS})
-target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")
+target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}" ARCANA_TESTING_HOOKS)
+target_compile_definitions(AppRuntime PRIVATE ARCANA_TESTING_HOOKS)
 
 target_link_libraries(UnitTests
     PRIVATE AppRuntime

Tests/UnitTests/Shared/Shared.cpp

@@ -11,8 +11,11 @@
 #include <Babylon/Polyfills/Blob.h>
 #include <Babylon/Polyfills/TextDecoder.h>
 #include <gtest/gtest.h>
+#include <chrono>
 #include <future>
 #include <iostream>
+#include <memory>
+#include <thread>
 
 namespace
 {
@@ -119,6 +122,62 @@ TEST(Console, Log)
     done.get_future().get();
 }
 
+TEST(AppRuntime, DestroyDoesNotDeadlock)
+{
+    // Deterministic test for the race condition in the AppRuntime destructor.
+    //
+    // A per-instance hook sleeps WHILE HOLDING the queue mutex, right before
+    // condition_variable::wait(). We synchronize so the worker is definitely
+    // in the hook before triggering destruction.
+    //
+    // Old (broken) code: cancel() + notify_all() fire without the mutex,
+    //   so the notification is lost while the worker sleeps → deadlock.
+    // Fixed code: Append(cancel) calls push() which NEEDS the mutex,
+    //   so it blocks until the worker enters wait() → notification delivered.
+    for (int i = 0; i < 3; i++)
+    {
+        // Shared state for hook synchronization
+        std::atomic<bool> hookEnabled{false};
+        std::atomic<bool> hookSignaled{false};
+        std::promise<void> workerInHook;
+
+        auto runtime = std::make_unique<Babylon::AppRuntime>();
+
+        // Set the callback once, before any dispatches. The callback checks
+        // hookEnabled so we control when it actually sleeps.
+        runtime->SetBeforeWaitCallback([&]() {
+            if (hookEnabled.load() && !hookSignaled.exchange(true))
+                workerInHook.set_value();
+            if (hookEnabled.load())
+                std::this_thread::sleep_for(std::chrono::milliseconds(200));
+        });
+
+        // Dispatch work and wait for completion
+        std::promise<void> ready;
+        runtime->Dispatch([&ready](Napi::Env) {
+            ready.set_value();
+        });
+        ready.get_future().wait();
+
+        // Enable the hook and dispatch a no-op to wake the worker,
+        // ensuring it cycles through the hook on its way back to idle
+        hookEnabled.store(true);
+        runtime->Dispatch([](Napi::Env) {});
+
+        // Wait for the worker to be in the hook (holding mutex, sleeping)
+        workerInHook.get_future().wait();
+
+        // NOW destroy
+        auto destroyFuture = std::async(std::launch::async, [&runtime]() {
+            runtime.reset();
+        });
+
+        auto status = destroyFuture.wait_for(std::chrono::seconds(5));
+        ASSERT_NE(status, std::future_status::timeout)
+            << "Deadlock detected on iteration " << i;
+    }
+}
+
 int RunTests()
 {
     testing::InitGoogleTest();

Tests/UnitTests/Win32/App.cpp

@@ -1,13 +1,15 @@
 #include "../Shared/Shared.h"
 #include <Windows.h>
 #include "Babylon/DebugTrace.h"
+#include <gtest/gtest.h>
 
-int main()
+int main(int argc, char** argv)
 {
     SetConsoleOutputCP(CP_UTF8);
 
     Babylon::DebugTrace::EnableDebugTrace(true);
     Babylon::DebugTrace::SetTraceOutput([](const char* trace) { OutputDebugStringA(trace); });
 
-    return RunTests();
+    testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
 }