Skip to content

Commit 7371f11

Browse files
bghgaryCopilot
andauthored
Merge WorkQueue into AppRuntime (#149)
Merge WorkQueue into AppRuntime to eliminate split-lifetime issues. ## Motivation WorkQueue was a separate class with its own thread, dispatcher, and cancel source. AppRuntime captured `this` in lambdas passed to WorkQueue, creating split-lifetime issues where the worker thread could access partially-destroyed AppRuntime members during shutdown. ## Changes - Thread, dispatcher, cancel source, env, and suspension lock are now direct AppRuntime members - Destruction order is deterministic (no cross-object lifetime issues) - WorkQueue.h and WorkQueue.cpp removed - AppRuntime_JSI.cpp: TaskRunnerAdapter now uses AppRuntime::Dispatch instead of WorkQueue::Append, and uses std::move(task) instead of task.release() for shared_ptr construction ## Behavioral note JSI tasks now route through Dispatch, which wraps them in the Execute/exception-handler. Previously, uncaught exceptions from JSI tasks would crash the process. They are now handled by UnhandledExceptionHandler like all other dispatched work. The deadlock fix from #147 is preserved. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent aab852d commit 7371f11

File tree

6 files changed

+73
-135
lines changed

6 files changed

+73
-135
lines changed

Core/AppRuntime/CMakeLists.txt

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@ set(SOURCES
99
"Include/Babylon/AppRuntime.h"
1010
"Source/AppRuntime.cpp"
1111
"Source/AppRuntime_${NAPI_JAVASCRIPT_ENGINE}.cpp"
12-
"Source/AppRuntime_${JSRUNTIMEHOST_PLATFORM}.${IMPL_EXT}"
13-
"Source/WorkQueue.cpp"
14-
"Source/WorkQueue.h")
12+
"Source/AppRuntime_${JSRUNTIMEHOST_PLATFORM}.${IMPL_EXT}")
1513

1614
add_library(AppRuntime ${SOURCES})
1715
warnings_as_errors(AppRuntime)

Core/AppRuntime/Include/Babylon/AppRuntime.h

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,19 @@
66

77
#include <napi/utilities.h>
88

9+
#include <arcana/threading/cancellation.h>
10+
#include <arcana/threading/dispatcher.h>
11+
912
#include <memory>
1013
#include <functional>
1114
#include <exception>
15+
#include <optional>
16+
#include <mutex>
17+
#include <thread>
18+
#include <type_traits>
1219

1320
namespace Babylon
1421
{
15-
class WorkQueue;
16-
1722
class AppRuntime final
1823
{
1924
public:
@@ -43,6 +48,23 @@ namespace Babylon
4348
static void BABYLON_API DefaultUnhandledExceptionHandler(const Napi::Error& error);
4449

4550
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+
4668
// These three methods are the mechanism by which platform- and JavaScript-specific
4769
// code can be "injected" into the execution of the JavaScript thread. These three
4870
// functions are implemented in separate files, thus allowing implementations to be
@@ -62,6 +84,10 @@ namespace Babylon
6284
void Execute(Dispatchable<void()> callback);
6385

6486
Options m_options;
65-
std::unique_ptr<WorkQueue> m_workQueue;
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;
6692
};
6793
}

Core/AppRuntime/Source/AppRuntime.cpp

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#include "AppRuntime.h"
2-
#include "WorkQueue.h"
32
#include <cassert>
43

54
namespace Babylon
@@ -11,7 +10,7 @@ namespace Babylon
1110

1211
AppRuntime::AppRuntime(Options options)
1312
: m_options{std::move(options)}
14-
, m_workQueue{std::make_unique<WorkQueue>([this] { RunPlatformTier(); })}
13+
, m_thread{[this] { RunPlatformTier(); }}
1514
{
1615
Dispatch([this](Napi::Env env) {
1716
JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); });
@@ -20,26 +19,58 @@ namespace Babylon
2019

2120
AppRuntime::~AppRuntime()
2221
{
22+
if (m_suspensionLock.has_value())
23+
{
24+
m_suspensionLock.reset();
25+
}
26+
27+
// Cancel immediately so pending work is dropped promptly, then append
28+
// a no-op work item to wake the worker thread from blocking_tick. The
29+
// no-op goes through push() which acquires the queue mutex, avoiding
30+
// the race where a bare notify_all() can be missed by wait().
31+
//
32+
// NOTE: This preserves the existing shutdown behavior where pending
33+
// callbacks are dropped on cancellation. A more complete solution
34+
// would add cooperative shutdown (e.g. NotifyDisposing/Rundown) so
35+
// consumers can finish cleanup work before the runtime is destroyed.
36+
m_cancelSource.cancel();
37+
Append([](Napi::Env) {});
38+
39+
m_thread.join();
2340
}
2441

2542
void AppRuntime::Run(Napi::Env env)
2643
{
27-
m_workQueue->Run(env);
44+
m_env = std::make_optional(env);
45+
46+
m_dispatcher.set_affinity(std::this_thread::get_id());
47+
48+
while (!m_cancelSource.cancelled())
49+
{
50+
m_dispatcher.blocking_tick(m_cancelSource);
51+
}
52+
53+
// The dispatcher can be non-empty if something is dispatched after cancellation.
54+
m_dispatcher.clear();
2855
}
2956

3057
void AppRuntime::Suspend()
3158
{
32-
m_workQueue->Suspend();
59+
auto suspensionMutex = std::make_shared<std::mutex>();
60+
m_suspensionLock.emplace(*suspensionMutex);
61+
Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) {
62+
std::scoped_lock lock{*suspensionMutex};
63+
});
3364
}
3465

3566
void AppRuntime::Resume()
3667
{
37-
m_workQueue->Resume();
68+
m_suspensionLock.reset();
3869
}
3970

4071
void AppRuntime::Dispatch(Dispatchable<void(Napi::Env)> func)
4172
{
42-
m_workQueue->Append([this, func{std::move(func)}](Napi::Env env) mutable {
73+
Append([this, func{std::move(func)}](Napi::Env env) mutable {
4374
Execute([this, env, func{std::move(func)}]() mutable {
4475
try
4576
{

Core/AppRuntime/Source/AppRuntime_JSI.cpp

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#include "AppRuntime.h"
2-
#include "WorkQueue.h"
32

43
#include <napi/env.h>
54
#include <V8JsiRuntime.h>
@@ -10,15 +9,15 @@ namespace
109
class TaskRunnerAdapter : public v8runtime::JSITaskRunner
1110
{
1211
public:
13-
TaskRunnerAdapter(Babylon::WorkQueue& workQueue)
14-
: m_workQueue(workQueue)
12+
TaskRunnerAdapter(Babylon::AppRuntime& runtime)
13+
: m_runtime(runtime)
1514
{
1615
}
1716

1817
void postTask(std::unique_ptr<v8runtime::JSITask> task) override
1918
{
20-
std::shared_ptr<v8runtime::JSITask> shared_task(task.release());
21-
m_workQueue.Append([shared_task2 = std::move(shared_task)](Napi::Env) {
19+
std::shared_ptr<v8runtime::JSITask> shared_task(std::move(task));
20+
m_runtime.Dispatch([shared_task2 = std::move(shared_task)](Napi::Env) {
2221
shared_task2->run();
2322
});
2423
}
@@ -27,7 +26,7 @@ namespace
2726
TaskRunnerAdapter(const TaskRunnerAdapter&) = delete;
2827
TaskRunnerAdapter& operator=(const TaskRunnerAdapter&) = delete;
2928

30-
Babylon::WorkQueue& m_workQueue;
29+
Babylon::AppRuntime& m_runtime;
3130
};
3231
}
3332

@@ -37,7 +36,7 @@ namespace Babylon
3736
{
3837
v8runtime::V8RuntimeArgs args{};
3938
args.inspectorPort = 5643;
40-
args.foreground_task_runner = std::make_shared<TaskRunnerAdapter>(*m_workQueue);
39+
args.foreground_task_runner = std::make_shared<TaskRunnerAdapter>(*this);
4140
const auto runtime = v8runtime::makeV8Runtime(std::move(args));
4241

4342
const auto env = Napi::Attach(*runtime);

Core/AppRuntime/Source/WorkQueue.cpp

Lines changed: 0 additions & 58 deletions
This file was deleted.

Core/AppRuntime/Source/WorkQueue.h

Lines changed: 0 additions & 58 deletions
This file was deleted.

0 commit comments

Comments
 (0)