Skip to content

Commit 421a7ba

Browse files
bghgaryCopilot
andcommitted
Add deterministic test for WorkQueue destructor deadlock race
Add a test that deterministically triggers the lost-wakeup race condition in WorkQueue::~WorkQueue(). Uses a global arcana testing hook to sleep while holding the queue mutex before wait(), guaranteeing the worker is between the condition check and wait() when the destructor fires. The test is expected to FAIL (deadlock timeout) with the current code, proving the bug exists. The fix will be in a separate PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 08b5ecc commit 421a7ba

File tree

3 files changed

+74
-4
lines changed

3 files changed

+74
-4
lines changed

CMakeLists.txt

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ FetchContent_Declare(AndroidExtensions
1414
GIT_TAG 2d5af72259cc73e5f249d3c99bee2010be9cb042
1515
EXCLUDE_FROM_ALL)
1616
FetchContent_Declare(arcana.cpp
17-
GIT_REPOSITORY https://github.com/microsoft/arcana.cpp.git
18-
GIT_TAG 7c6be8aaa29effbc8ee1a2217388fb6e399150d2
17+
GIT_REPOSITORY https://github.com/bghgary/arcana.cpp.git
18+
GIT_TAG fa054fda979fb8da657ff27121d4cdd35905bd65
1919
EXCLUDE_FROM_ALL)
2020
FetchContent_Declare(asio
2121
GIT_REPOSITORY https://github.com/chriskohlhoff/asio.git
@@ -113,6 +113,10 @@ endif()
113113

114114
# --------------------------------------------------
115115

116+
if(JSRUNTIMEHOST_TESTS)
117+
add_compile_definitions(ARCANA_TESTING_HOOKS)
118+
endif()
119+
116120
FetchContent_MakeAvailable_With_Message(arcana.cpp)
117121
set_property(TARGET arcana PROPERTY FOLDER Dependencies)
118122

Tests/UnitTests/Shared/Shared.cpp

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,12 @@
1111
#include <Babylon/Polyfills/Blob.h>
1212
#include <Babylon/Polyfills/TextDecoder.h>
1313
#include <gtest/gtest.h>
14+
#include <arcana/threading/blocking_concurrent_queue.h>
15+
#include <atomic>
16+
#include <chrono>
1417
#include <future>
1518
#include <iostream>
19+
#include <thread>
1620

1721
namespace
1822
{
@@ -125,6 +129,66 @@ TEST(Console, Log)
125129
done.get_future().get();
126130
}
127131

132+
TEST(AppRuntime, DestroyDoesNotDeadlock)
133+
{
134+
// Deterministic test for the race condition in the AppRuntime destructor.
135+
//
136+
// A global hook sleeps WHILE HOLDING the queue mutex, right before
137+
// condition_variable::wait(). We synchronize so the worker is definitely
138+
// in the hook before triggering destruction.
139+
//
140+
// Old (broken) code: cancel() + notify_all() fire without the mutex,
141+
// so the notification is lost while the worker sleeps → deadlock.
142+
// Fixed code: Append(cancel) calls push() which NEEDS the mutex,
143+
// so it blocks until the worker enters wait() → notification delivered.
144+
for (int i = 0; i < 3; i++)
145+
{
146+
// Shared state for hook synchronization
147+
std::atomic<bool> hookEnabled{false};
148+
std::atomic<bool> hookSignaled{false};
149+
std::promise<void> workerInHook;
150+
151+
// Set the global callback. It checks hookEnabled so we control
152+
// when it actually sleeps.
153+
g_beforeWaitCallback = [&]() {
154+
if (hookEnabled.load() && !hookSignaled.exchange(true))
155+
workerInHook.set_value();
156+
if (hookEnabled.load())
157+
std::this_thread::sleep_for(std::chrono::milliseconds(200));
158+
};
159+
160+
auto runtime = std::make_unique<Babylon::AppRuntime>();
161+
162+
// Dispatch work and wait for completion
163+
std::promise<void> ready;
164+
runtime->Dispatch([&ready](Napi::Env) {
165+
ready.set_value();
166+
});
167+
ready.get_future().wait();
168+
169+
// Enable the hook and dispatch a no-op to wake the worker,
170+
// ensuring it cycles through the hook on its way back to idle
171+
hookEnabled.store(true);
172+
runtime->Dispatch([](Napi::Env) {});
173+
174+
// Wait for the worker to be in the hook (holding mutex, sleeping)
175+
workerInHook.get_future().wait();
176+
177+
// NOW destroy
178+
auto destroyFuture = std::async(std::launch::async, [&runtime]() {
179+
runtime.reset();
180+
});
181+
182+
auto status = destroyFuture.wait_for(std::chrono::seconds(5));
183+
184+
// Reset hook
185+
g_beforeWaitCallback = []() {};
186+
187+
ASSERT_NE(status, std::future_status::timeout)
188+
<< "Deadlock detected on iteration " << i;
189+
}
190+
}
191+
128192
int RunTests()
129193
{
130194
testing::InitGoogleTest();

Tests/UnitTests/Win32/App.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
#include "../Shared/Shared.h"
22
#include <Windows.h>
33
#include "Babylon/DebugTrace.h"
4+
#include <gtest/gtest.h>
45

5-
int main()
6+
int main(int argc, char** argv)
67
{
78
SetConsoleOutputCP(CP_UTF8);
89

910
Babylon::DebugTrace::EnableDebugTrace(true);
1011
Babylon::DebugTrace::SetTraceOutput([](const char* trace) { OutputDebugStringA(trace); });
1112

12-
return RunTests();
13+
testing::InitGoogleTest(&argc, argv);
14+
return RUN_ALL_TESTS();
1315
}

0 commit comments

Comments
 (0)