Skip to content

Clean up deadlock regression test - #151

Merged
bghgary merged 14 commits into
BabylonJS:mainfrom
bghgary:fix/deadlock-test-cleanup
Apr 13, 2026
Merged

Clean up deadlock regression test#151
bghgary merged 14 commits into
BabylonJS:mainfrom
bghgary:fix/deadlock-test-cleanup

Conversation

@bghgary

@bghgarybghgary commented Apr 9, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

Clean up the deadlock regression test from #147.

Changes

  • Run entire test on a separate thread so gtest can detect deadlock via timeout
  • Install hook after initialization (no hookEnabled flag needed)
  • Hook only fires once (early return after first invocation) to prevent secondary deadlock from the hook sleeping on subsequent wait() calls
  • Use plain bool for hookSignaled (only accessed by worker thread)
  • Expanded comment on the sleep explaining non-determinism and its purpose
  • Update comments and flow diagram to match code

Dependency Updates

Verification

@bghgary
Clean up deadlock regression test
46bc3e7
- Run entire test on a separate thread so gtest can detect deadlock
via timeout without hanging the process
- Install hook after initialization instead of using enable flag
- Use plain bool for hookSignaled (single-thread access)
- Move unique_ptr into test thread (single owner)
- Update comments and flow diagram to match code
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 9, 2026 17:36
CopilotAI reviewed Apr 9, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the AppRuntime destructor deadlock regression test (originally added in #147) to make deadlock detection reliable by letting the gtest thread time out while the destructor runs on a separate worker thread.

Changes:

  • Move the runtime lifecycle (create → init → hook → destroy) into a dedicated thread so the main test thread can detect deadlock via timeout.
  • Install the arcana before-wait hook after initialization and simplify hook signaling state.
  • Update test comments/flow diagram to match the new control flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadTests/UnitTests/Shared/Shared.cpp Outdated
Comment threadTests/UnitTests/Shared/Shared.cpp
Comment threadTests/UnitTests/Shared/Shared.cpp
bghgary added a commit to bghgary/JsRuntimeHost that referenced this pull request Apr 9, 2026
@bghgary
Revert WorkQueue destructor to old broken code + updated test
15bb2f1
Reverts the deadlock fix to demonstrate the test detects it.
Test updated with cleanup from BabylonJS#151.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgaryand others added 10 commits April 9, 2026 13:36
@bghgary
Add comment explaining non-deterministic sleep window
f0ab95d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Update arcana.cpp to use renamed test hooks
2e2ec55
Bump arcana.cpp GIT_TAG to b9bf9d85fce37d5fc9dbfc4a4dc5e1531bee215a
which renames ARCANA_TESTING_HOOKS to ARCANA_TEST_HOOKS and moves hooks
to arcana::test_hooks::blocking_concurrent_queue namespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Retrigger CI
f05ea06
@bghgary
Fix segfault on deadlock test timeout path
59d3744
Move shared state (hookSignaled, workerInHook, testDone) from stack
to heap via shared_ptr<State>. On timeout the test thread is detached,
so it must not reference stack variables that are destroyed when the
test function returns.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Revert accidental package-lock.json change
ff96f4a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Merge remote-tracking branch 'origin/main' into fix/deadlock-test-cle…
95c2e0b
…anup
@bghgary
Terminate on deadlock timeout instead of continuing
b114f66
FAIL() only returns from the test function — gtest continues running.
The detached deadlocked thread corrupts process state, causing segfaults
during teardown on Linux. Use quick_exit(1) instead since the process
is unrecoverable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Fix test deadlock: only sleep on first hook invocation
ad29a3d
The hook fires on every blocking_tick wait, not just the first. After
the worker processes the destructor's no-op wake-up and loops back, the
hook fired again, sleeping 200ms with no pending push to wake the worker
afterward — causing a real deadlock in the test itself.
Return early after the first invocation so subsequent wait() calls are
not delayed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Simplify test: use stack variables instead of heap State
3677bb4
With quick_exit(1) on timeout, the process terminates immediately so
stack variables are never destroyed while the detached thread runs.
No need for shared_ptr heap allocation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Revert quick_exit back to FAIL()
8b44a9e
The segfault was caused by the hook sleeping on every invocation, not
by FAIL() being unsafe. With the hook fixed to only fire once, the
timeout path should never be reached.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary
bghgary commented Apr 13, 2026
View reviewed changes

@bghgarybghgary left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synchronous review comments.

Comment threadTests/UnitTests/Shared/Shared.cpp Outdated
Comment threadTests/UnitTests/Shared/Shared.cpp
bghgaryand others added 3 commits April 13, 2026 08:10
@bghgary
Address review: improve sleep comment explaining non-determinism
b10dbbd
Expanded comment to clarify that the sleep is not truly deterministic
and explain its purpose: hold the mutex long enough for runtime.reset()
to reach push() while the mutex is still held.
The test_hooks::dispatcher namespace suggestion is noted as a follow-up
for arcana.cpp.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bghgary
Merge remote-tracking branch 'origin/main' into fix/deadlock-test-cle…
5aabdfb
…anup
@bghgary
Merge remote-tracking branch 'origin/main' into fix/deadlock-test-cle…
978a009
…anup
bkaradzic-microsoft
bkaradzic-microsoft approved these changes Apr 13, 2026
View reviewed changes

@bkaradzic-microsoftbkaradzic-microsoft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO!

@bghgary
bghgary merged commit 901208a into BabylonJS:mainApr 13, 2026
19 checks passed
@bghgarybghgary mentioned this pull request Apr 13, 2026
Closed
@bghgary
bghgary deleted the fix/deadlock-test-cleanup branch April 14, 2026 19:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@bkaradzic-microsoftbkaradzic-microsoftbkaradzic-microsoft approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bghgary@bkaradzic-microsoft