From c54627a0aca73c68475495ffa0934597537a5120 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 23 May 2023 15:04:44 -0700 Subject: [PATCH] Merge AppRuntime and WorkQueue to fix race condition --- Core/AppRuntime/CMakeLists.txt | 8 +- Core/AppRuntime/Include/Babylon/AppRuntime.h | 32 ++----- Core/AppRuntime/Source/AppRuntime.cpp | 51 +++-------- Core/AppRuntime/Source/AppRuntimeImpl.cpp | 88 +++++++++++++++++++ Core/AppRuntime/Source/AppRuntimeImpl.h | 68 ++++++++++++++ ...Android.cpp => AppRuntimeImpl_Android.cpp} | 0 ...e_Chakra.cpp => AppRuntimeImpl_Chakra.cpp} | 26 +++--- ...Runtime_JSI.cpp => AppRuntimeImpl_JSI.cpp} | 0 ....cpp => AppRuntimeImpl_JavaScriptCore.cpp} | 0 ...ntime_Unix.cpp => AppRuntimeImpl_Unix.cpp} | 0 ...ppRuntime_V8.cpp => AppRuntimeImpl_V8.cpp} | 0 ...ime_Win32.cpp => AppRuntimeImpl_Win32.cpp} | 8 +- ...ime_WinRT.cpp => AppRuntimeImpl_WinRT.cpp} | 0 ...ppRuntime_iOS.mm => AppRuntimeImpl_iOS.mm} | 0 ...ntime_macOS.mm => AppRuntimeImpl_macOS.mm} | 0 Core/AppRuntime/Source/WorkQueue.cpp | 59 ------------- Core/AppRuntime/Source/WorkQueue.h | 49 ----------- 17 files changed, 195 insertions(+), 194 deletions(-) create mode 100644 Core/AppRuntime/Source/AppRuntimeImpl.cpp create mode 100644 Core/AppRuntime/Source/AppRuntimeImpl.h rename Core/AppRuntime/Source/{AppRuntime_Android.cpp => AppRuntimeImpl_Android.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_Chakra.cpp => AppRuntimeImpl_Chakra.cpp} (66%) rename Core/AppRuntime/Source/{AppRuntime_JSI.cpp => AppRuntimeImpl_JSI.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_JavaScriptCore.cpp => AppRuntimeImpl_JavaScriptCore.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_Unix.cpp => AppRuntimeImpl_Unix.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_V8.cpp => AppRuntimeImpl_V8.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_Win32.cpp => AppRuntimeImpl_Win32.cpp} (73%) rename Core/AppRuntime/Source/{AppRuntime_WinRT.cpp => AppRuntimeImpl_WinRT.cpp} (100%) rename Core/AppRuntime/Source/{AppRuntime_iOS.mm => AppRuntimeImpl_iOS.mm} (100%) rename Core/AppRuntime/Source/{AppRuntime_macOS.mm => AppRuntimeImpl_macOS.mm} (100%) delete mode 100644 Core/AppRuntime/Source/WorkQueue.cpp delete mode 100644 Core/AppRuntime/Source/WorkQueue.h diff --git a/Core/AppRuntime/CMakeLists.txt b/Core/AppRuntime/CMakeLists.txt index 400783488..b7e24b38e 100644 --- a/Core/AppRuntime/CMakeLists.txt +++ b/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) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 70ddd5921..ab0a9f000 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -1,7 +1,8 @@ #pragma once +#include + #include "Dispatchable.h" -#include #include #include @@ -9,7 +10,7 @@ namespace Babylon { - class WorkQueue; + class AppRuntimeImpl; class AppRuntime final { @@ -18,33 +19,16 @@ namespace Babylon AppRuntime(std::function unhandledExceptionHandler); ~AppRuntime(); + // Move semantics + AppRuntime(AppRuntime&&) noexcept; + AppRuntime& operator=(AppRuntime&&) noexcept; + void Suspend(); void Resume(); void Dispatch(Dispatchable 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 callback); - - static void DefaultUnhandledExceptionHandler(const std::exception& error); - - std::unique_ptr m_workQueue{}; - std::function m_unhandledExceptionHandler{}; + std::unique_ptr m_impl; }; } diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index 0e6833a2c..ca0ea96f5 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -1,65 +1,36 @@ #include "AppRuntime.h" -#include "WorkQueue.h" -#include +#include "AppRuntimeImpl.h" namespace Babylon { AppRuntime::AppRuntime() - : AppRuntime{DefaultUnhandledExceptionHandler} + : m_impl{std::make_unique()} { } AppRuntime::AppRuntime(std::function unhandledExceptionHandler) - : m_workQueue{std::make_unique([this] { RunPlatformTier(); })} - , m_unhandledExceptionHandler{unhandledExceptionHandler} + : m_impl{std::make_unique(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 func) + void AppRuntime::Dispatch(Dispatchable 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)); } } diff --git a/Core/AppRuntime/Source/AppRuntimeImpl.cpp b/Core/AppRuntime/Source/AppRuntimeImpl.cpp new file mode 100644 index 000000000..9e28faf1e --- /dev/null +++ b/Core/AppRuntime/Source/AppRuntimeImpl.cpp @@ -0,0 +1,88 @@ +#include "AppRuntimeImpl.h" +#include + +namespace Babylon +{ + AppRuntimeImpl::AppRuntimeImpl(std::function 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(); + 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 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(); + } +} diff --git a/Core/AppRuntime/Source/AppRuntimeImpl.h b/Core/AppRuntime/Source/AppRuntimeImpl.h new file mode 100644 index 000000000..a3bc12104 --- /dev/null +++ b/Core/AppRuntime/Source/AppRuntimeImpl.h @@ -0,0 +1,68 @@ +#pragma once + +#include "AppRuntime.h" +#include +#include +#include + +namespace Babylon +{ + class AppRuntimeImpl + { + public: + AppRuntimeImpl(std::function unhandledExceptionHandler = DefaultUnhandledExceptionHandler); + ~AppRuntimeImpl(); + + void Suspend(); + void Resume(); + + void Dispatch(Dispatchable func); + + private: + static void DefaultUnhandledExceptionHandler(const std::exception& error); + + template + 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::value) + { + m_dispatcher.queue([this, callable = std::move(callable)]() { + callable(m_env.value()); + }); + } + else + { + m_dispatcher.queue([this, callablePtr = std::make_shared(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 callback); + + std::function m_unhandledExceptionHandler{}; + std::optional m_env{}; + std::optional> m_suspensionLock{}; + arcana::cancellation_source m_cancellationSource{}; + arcana::manual_dispatcher<128> m_dispatcher{}; + std::thread m_thread{}; + }; +} diff --git a/Core/AppRuntime/Source/AppRuntime_Android.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_Android.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_Android.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_Android.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_Chakra.cpp similarity index 66% rename from Core/AppRuntime/Source/AppRuntime_Chakra.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_Chakra.cpp index d8200afc6..6d829e763 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntimeImpl_Chakra.cpp @@ -1,4 +1,4 @@ -#include "AppRuntime.h" +#include "AppRuntimeImpl.h" #include @@ -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(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(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) diff --git a/Core/AppRuntime/Source/AppRuntime_JSI.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_JSI.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_JSI.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_JSI.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_JavaScriptCore.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_JavaScriptCore.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_Unix.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_Unix.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_Unix.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_Unix.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_V8.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_V8.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_V8.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_Win32.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_Win32.cpp similarity index 73% rename from Core/AppRuntime/Source/AppRuntime_Win32.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_Win32.cpp index 5fcee5698..753de97d9 100644 --- a/Core/AppRuntime/Source/AppRuntime_Win32.cpp +++ b/Core/AppRuntime/Source/AppRuntimeImpl_Win32.cpp @@ -1,4 +1,4 @@ -#include "AppRuntime.h" +#include "AppRuntimeImpl.h" #include #include @@ -10,7 +10,7 @@ namespace Babylon { - void AppRuntime::RunPlatformTier() + void AppRuntimeImpl::RunPlatformTier() { winrt::check_hresult(Windows::Foundation::Initialize(RO_INIT_MULTITHREADED)); @@ -20,14 +20,14 @@ namespace Babylon RunEnvironmentTier(executablePath); } - void AppRuntime::DefaultUnhandledExceptionHandler(const std::exception& error) + void AppRuntimeImpl::DefaultUnhandledExceptionHandler(const std::exception& error) { std::stringstream ss{}; ss << "Uncaught Error: " << error.what() << std::endl; OutputDebugStringA(ss.str().data()); } - void AppRuntime::Execute(Dispatchable callback) + void AppRuntimeImpl::Execute(Dispatchable callback) { callback(); } diff --git a/Core/AppRuntime/Source/AppRuntime_WinRT.cpp b/Core/AppRuntime/Source/AppRuntimeImpl_WinRT.cpp similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_WinRT.cpp rename to Core/AppRuntime/Source/AppRuntimeImpl_WinRT.cpp diff --git a/Core/AppRuntime/Source/AppRuntime_iOS.mm b/Core/AppRuntime/Source/AppRuntimeImpl_iOS.mm similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_iOS.mm rename to Core/AppRuntime/Source/AppRuntimeImpl_iOS.mm diff --git a/Core/AppRuntime/Source/AppRuntime_macOS.mm b/Core/AppRuntime/Source/AppRuntimeImpl_macOS.mm similarity index 100% rename from Core/AppRuntime/Source/AppRuntime_macOS.mm rename to Core/AppRuntime/Source/AppRuntimeImpl_macOS.mm diff --git a/Core/AppRuntime/Source/WorkQueue.cpp b/Core/AppRuntime/Source/WorkQueue.cpp deleted file mode 100644 index 9b3b2cc00..000000000 --- a/Core/AppRuntime/Source/WorkQueue.cpp +++ /dev/null @@ -1,59 +0,0 @@ -#include "WorkQueue.h" - -namespace Babylon -{ - WorkQueue::WorkQueue(std::function threadProcedure) - : m_thread{std::move(threadProcedure)} - { - } - - WorkQueue::~WorkQueue() - { - if (m_suspensionLock.has_value()) - { - Resume(); - } - - // Dispatch a cancel 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_dispatcher([this]() { - m_cancellationSource.cancel(); - }); - - m_thread.join(); - } - - void WorkQueue::Suspend() - { - auto suspensionMutex = std::make_shared(); - m_suspensionLock.emplace(*suspensionMutex); - Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) { - std::scoped_lock lock{*suspensionMutex}; - }); - } - - void WorkQueue::Resume() - { - m_suspensionLock.reset(); - } - - void WorkQueue::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(); - } -} diff --git a/Core/AppRuntime/Source/WorkQueue.h b/Core/AppRuntime/Source/WorkQueue.h deleted file mode 100644 index a731c1ee9..000000000 --- a/Core/AppRuntime/Source/WorkQueue.h +++ /dev/null @@ -1,49 +0,0 @@ -#pragma once - -#include -#include -#include - -#include -#include -#include - -namespace Babylon -{ - class WorkQueue - { - public: - WorkQueue(std::function threadProcedure); - ~WorkQueue(); - - template - 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::value) - { - m_dispatcher.queue([this, callable = std::move(callable)]() { - callable(m_env.value()); - }); - } - else - { - m_dispatcher.queue([this, callablePtr = std::make_shared(std::move(callable))]() { - (*callablePtr)(m_env.value()); - }); - } - } - - void Suspend(); - void Resume(); - void Run(Napi::Env); - - private: - std::optional m_env{}; - std::optional> m_suspensionLock{}; - arcana::cancellation_source m_cancellationSource{}; - arcana::manual_dispatcher<128> m_dispatcher{}; - std::thread m_thread{}; - }; -}