diff --git a/.gitignore b/.gitignore index b84ede5a..0260b044 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ /Build +Tests/UnitTests/dist/ diff --git a/CMakeLists.txt b/CMakeLists.txt index 33b28392..116efa6b 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 7aaeb398..ba0ffa71 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -61,7 +61,7 @@ namespace Babylon // extra logic around the invocation of a dispatched callback. void Execute(Dispatchable callback); - std::unique_ptr m_workQueue; Options m_options; + std::unique_ptr m_workQueue; }; } diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index a6d89492..b3a90b1b 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -10,8 +10,8 @@ namespace Babylon } AppRuntime::AppRuntime(Options options) - : m_workQueue{std::make_unique([this] { RunPlatformTier(); })} - , m_options{std::move(options)} + : m_options{std::move(options)} + , m_workQueue{std::make_unique([this] { RunPlatformTier(); })} { Dispatch([this](Napi::Env env) { JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); }); diff --git a/Core/AppRuntime/Source/WorkQueue.cpp b/Core/AppRuntime/Source/WorkQueue.cpp index 4c1c5f21..8ff5efe1 100644 --- a/Core/AppRuntime/Source/WorkQueue.cpp +++ b/Core/AppRuntime/Source/WorkQueue.cpp @@ -14,8 +14,17 @@ namespace Babylon Resume(); } + // Cancel immediately so pending work is dropped promptly, then append + // a no-op work item to wake the worker thread from blocking_tick. The + // no-op goes through push() which acquires the queue mutex, avoiding + // the race where a bare notify_all() can be missed by wait(). + // + // NOTE: This preserves the existing shutdown behavior where pending + // callbacks are dropped on cancellation. A more complete solution + // would add cooperative shutdown (e.g. NotifyDisposing/Rundown) so + // consumers can finish cleanup work before the runtime is destroyed. m_cancelSource.cancel(); - m_dispatcher.cancelled(); + Append([](Napi::Env) {}); m_thread.join(); } diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index b513a866..ac5032ed 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/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) @@ -21,6 +22,7 @@ add_library(UnitTestsJNI SHARED ${UNIT_TESTS_DIR}/Shared/Shared.cpp) target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TESTING_HOOKS) target_include_directories(UnitTestsJNI PRIVATE ${UNIT_TESTS_DIR}) diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1b9b27db..e09da4fd 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -11,8 +11,12 @@ #include #include #include +#include +#include +#include #include #include +#include namespace { @@ -125,6 +129,86 @@ TEST(Console, Log) done.get_future().get(); } +TEST(AppRuntime, DestroyDoesNotDeadlock) +{ + // Deterministic test for the race condition in the AppRuntime destructor. + // + // A global 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: cancel() + Append(no-op), where push() NEEDS the mutex, + // so it blocks until the worker enters wait() → notification delivered. + + // Shared state for hook synchronization + std::atomic hookEnabled{false}; + std::atomic hookSignaled{false}; + std::promise workerInHook; + + // Set the callback. It checks hookEnabled so we control + // when it actually sleeps. + arcana::set_before_wait_callback([&]() { + if (hookEnabled.load() && !hookSignaled.exchange(true)) + { + workerInHook.set_value(); + } + if (hookEnabled.load()) + { + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + } + }); + + auto runtime = std::make_unique(); + + // Dispatch work and wait for completion + std::promise 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) + auto hookStatus = workerInHook.get_future().wait_for(std::chrono::seconds(5)); + if (hookStatus == std::future_status::timeout) + { + // Hook didn't fire — no deadlock risk, clean up normally + arcana::set_before_wait_callback([]() {}); + FAIL() << "Worker thread did not enter before-wait hook"; + } + + // Worker is in the hook (holding mutex, sleeping). Destroy on a + // detachable thread so the test doesn't hang if the destructor deadlocks. + auto runtimePtr = std::make_shared>(std::move(runtime)); + std::promise destroyDone; + auto destroyFuture = destroyDone.get_future(); + std::thread destroyThread([runtimePtr, &destroyDone]() { + runtimePtr->reset(); + destroyDone.set_value(); + }); + + auto status = destroyFuture.wait_for(std::chrono::seconds(5)); + if (status == std::future_status::timeout) + { + destroyThread.detach(); + } + else + { + destroyThread.join(); + } + + arcana::set_before_wait_callback([]() {}); + + ASSERT_NE(status, std::future_status::timeout) + << "Deadlock detected: AppRuntime destructor did not complete within 5 seconds"; +} + int RunTests() { testing::InitGoogleTest(); diff --git a/Tests/UnitTests/Win32/App.cpp b/Tests/UnitTests/Win32/App.cpp index 84aaac46..7e60a6df 100644 --- a/Tests/UnitTests/Win32/App.cpp +++ b/Tests/UnitTests/Win32/App.cpp @@ -1,13 +1,15 @@ #include "../Shared/Shared.h" #include #include "Babylon/DebugTrace.h" +#include -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(); }