Skip to content

Commit fe9c37f

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 fe9c37f

File tree

5 files changed

+76
-4
lines changed

5 files changed

+76
-4
lines changed

CMakeLists.txt

Lines changed: 2 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 8328898f159c1778cb16fa41c7ad57994961e5e8
1919
EXCLUDE_FROM_ALL)
2020
FetchContent_Declare(asio
2121
GIT_REPOSITORY https://github.com/chriskohlhoff/asio.git

Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ add_library(UnitTestsJNI SHARED
2121
${UNIT_TESTS_DIR}/Shared/Shared.cpp)
2222

2323
target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")
24+
target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TESTING_HOOKS)
2425

2526
target_include_directories(UnitTestsJNI
2627
PRIVATE ${UNIT_TESTS_DIR})

Tests/UnitTests/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ endif()
4545

4646
add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS})
4747
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")
48+
target_compile_definitions(UnitTests PRIVATE ARCANA_TESTING_HOOKS)
4849

4950
if(ENABLE_SANITIZERS)
5051
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_SANITIZERS_ENABLED=1)

Tests/UnitTests/Shared/Shared.cpp

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,14 @@
1111
#include <Babylon/Polyfills/Blob.h>
1212
#include <Babylon/Polyfills/TextDecoder.h>
1313
#include <gtest/gtest.h>
14+
#ifdef ARCANA_TESTING_HOOKS
15+
#include <arcana/threading/blocking_concurrent_queue.h>
16+
#endif
17+
#include <atomic>
18+
#include <chrono>
1419
#include <future>
1520
#include <iostream>
21+
#include <thread>
1622

1723
namespace
1824
{
@@ -125,6 +131,68 @@ TEST(Console, Log)
125131
done.get_future().get();
126132
}
127133

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