Skip to content

Commit 8086014

Browse files
bghgaryCopilot
andauthored
Hide arcana behind pimpl in AppRuntime (#154)
[Created by Copilot on behalf of @bghgary] Hide arcana implementation details behind a pimpl pattern in AppRuntime. ## The Bug PR #149 (Merge WorkQueue into AppRuntime) moved arcana includes into the public `AppRuntime.h` header. This exposed `arcana/threading/cancellation.h` and `arcana/threading/dispatcher.h` as transitive dependencies for all consumers, breaking builds in BabylonNative where arcana's include directories weren't propagated (`PRIVATE` link). ## The Fix Move the arcana-dependent members (`cancellation_source`, `manual_dispatcher`, `thread`, `env`, `suspension lock`) and the `Append` template into a private `Impl` class defined in `AppRuntime.cpp`. The public header no longer includes any arcana headers. `m_options` stays on `AppRuntime` since platform-specific `.cpp` files access it directly. Explicitly delete copy and move constructors/operators — the pimpl `unique_ptr` would otherwise make AppRuntime movable, but moving with a running thread and captured `this` pointers would be UB. ## Thread Safety The worker thread is joined in `~AppRuntime()` before `unique_ptr<Impl>` is destroyed, preserving the same lifetime guarantees as before. No split-lifetime issues. ## Verified - All 137 JS tests pass locally (Win32 V8) - `DestroyDoesNotDeadlock` test passes (210ms) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent db7ae43 commit 8086014

2 files changed

Lines changed: 63 additions & 44 deletions

File tree

Core/AppRuntime/Include/Babylon/AppRuntime.h

Lines changed: 11 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,9 @@
66

77
#include <napi/utilities.h>
88

9-
#include <arcana/threading/cancellation.h>
10-
#include <arcana/threading/dispatcher.h>
11-
129
#include <memory>
1310
#include <functional>
1411
#include <exception>
15-
#include <optional>
16-
#include <mutex>
17-
#include <thread>
18-
#include <type_traits>
1912

2013
namespace Babylon
2114
{
@@ -39,6 +32,14 @@ namespace Babylon
3932
AppRuntime(Options options);
4033
~AppRuntime();
4134

35+
// Copy semantics
36+
AppRuntime(const AppRuntime&) = delete;
37+
AppRuntime& operator=(const AppRuntime&) = delete;
38+
39+
// Move semantics
40+
AppRuntime(AppRuntime&&) = delete;
41+
AppRuntime& operator=(AppRuntime&&) = delete;
42+
4243
void Suspend();
4344
void Resume();
4445

@@ -48,23 +49,6 @@ namespace Babylon
4849
static void BABYLON_API DefaultUnhandledExceptionHandler(const Napi::Error& error);
4950

5051
private:
51-
template<typename CallableT>
52-
void Append(CallableT callable)
53-
{
54-
if constexpr (std::is_copy_constructible<CallableT>::value)
55-
{
56-
m_dispatcher.queue([this, callable = std::move(callable)]() {
57-
callable(m_env.value());
58-
});
59-
}
60-
else
61-
{
62-
m_dispatcher.queue([this, callablePtr = std::make_shared<CallableT>(std::move(callable))]() {
63-
(*callablePtr)(m_env.value());
64-
});
65-
}
66-
}
67-
6852
// These three methods are the mechanism by which platform- and JavaScript-specific
6953
// code can be "injected" into the execution of the JavaScript thread. These three
7054
// functions are implemented in separate files, thus allowing implementations to be
@@ -84,10 +68,8 @@ namespace Babylon
8468
void Execute(Dispatchable<void()> callback);
8569

8670
Options m_options;
87-
std::optional<Napi::Env> m_env{};
88-
std::optional<std::scoped_lock<std::mutex>> m_suspensionLock{};
89-
arcana::cancellation_source m_cancelSource{};
90-
arcana::manual_dispatcher<128> m_dispatcher{};
91-
std::thread m_thread;
71+
72+
class Impl;
73+
std::unique_ptr<Impl> m_impl;
9274
};
9375
}

Core/AppRuntime/Source/AppRuntime.cpp

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,64 @@
11
#include "AppRuntime.h"
2+
3+
#include <arcana/threading/cancellation.h>
4+
#include <arcana/threading/dispatcher.h>
5+
26
#include <cassert>
7+
#include <optional>
8+
#include <mutex>
9+
#include <thread>
10+
#include <type_traits>
311

412
namespace Babylon
513
{
14+
class AppRuntime::Impl
15+
{
16+
public:
17+
template<typename CallableT>
18+
void Append(CallableT callable)
19+
{
20+
if constexpr (std::is_copy_constructible<CallableT>::value)
21+
{
22+
m_dispatcher.queue([this, callable = std::move(callable)]() {
23+
callable(m_env.value());
24+
});
25+
}
26+
else
27+
{
28+
m_dispatcher.queue([this, callablePtr = std::make_shared<CallableT>(std::move(callable))]() {
29+
(*callablePtr)(m_env.value());
30+
});
31+
}
32+
}
33+
34+
std::optional<Napi::Env> m_env{};
35+
std::optional<std::scoped_lock<std::mutex>> m_suspensionLock{};
36+
arcana::cancellation_source m_cancelSource{};
37+
arcana::manual_dispatcher<128> m_dispatcher{};
38+
std::thread m_thread;
39+
};
40+
641
AppRuntime::AppRuntime() :
742
AppRuntime{{}}
843
{
944
}
1045

1146
AppRuntime::AppRuntime(Options options)
1247
: m_options{std::move(options)}
13-
, m_thread{[this] { RunPlatformTier(); }}
48+
, m_impl{std::make_unique<Impl>()}
1449
{
50+
m_impl->m_thread = std::thread{[this] { RunPlatformTier(); }};
51+
1552
Dispatch([this](Napi::Env env) {
1653
JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
1754
});
1855
}
1956

2057
AppRuntime::~AppRuntime()
2158
{
22-
if (m_suspensionLock.has_value())
59+
if (m_impl->m_suspensionLock.has_value())
2360
{
24-
m_suspensionLock.reset();
61+
m_impl->m_suspensionLock.reset();
2562
}
2663

2764
// Cancel immediately so pending work is dropped promptly, then append
@@ -33,44 +70,44 @@ namespace Babylon
3370
// callbacks are dropped on cancellation. A more complete solution
3471
// would add cooperative shutdown (e.g. NotifyDisposing/Rundown) so
3572
// consumers can finish cleanup work before the runtime is destroyed.
36-
m_cancelSource.cancel();
37-
Append([](Napi::Env) {});
73+
m_impl->m_cancelSource.cancel();
74+
m_impl->Append([](Napi::Env) {});
3875

39-
m_thread.join();
76+
m_impl->m_thread.join();
4077
}
4178

4279
void AppRuntime::Run(Napi::Env env)
4380
{
44-
m_env = std::make_optional(env);
81+
m_impl->m_env = std::make_optional(env);
4582

46-
m_dispatcher.set_affinity(std::this_thread::get_id());
83+
m_impl->m_dispatcher.set_affinity(std::this_thread::get_id());
4784

48-
while (!m_cancelSource.cancelled())
85+
while (!m_impl->m_cancelSource.cancelled())
4986
{
50-
m_dispatcher.blocking_tick(m_cancelSource);
87+
m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource);
5188
}
5289

5390
// The dispatcher can be non-empty if something is dispatched after cancellation.
54-
m_dispatcher.clear();
91+
m_impl->m_dispatcher.clear();
5592
}
5693

5794
void AppRuntime::Suspend()
5895
{
5996
auto suspensionMutex = std::make_shared<std::mutex>();
60-
m_suspensionLock.emplace(*suspensionMutex);
61-
Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) {
97+
m_impl->m_suspensionLock.emplace(*suspensionMutex);
98+
m_impl->Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) {
6299
std::scoped_lock lock{*suspensionMutex};
63100
});
64101
}
65102

66103
void AppRuntime::Resume()
67104
{
68-
m_suspensionLock.reset();
105+
m_impl->m_suspensionLock.reset();
69106
}
70107

71108
void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> func)
72109
{
73-
Append([this, func{std::move(func)}](Napi::Env env) mutable {
110+
m_impl->Append([this, func{std::move(func)}](Napi::Env env) mutable {
74111
Execute([this, env, func{std::move(func)}]() mutable {
75112
try
76113
{

0 commit comments

Comments
 (0)