Hide arcana behind pimpl in AppRuntime#154
Merged
bghgary merged 3 commits intoBabylonJS:mainfrom Apr 14, 2026
Merged
Conversation
Move arcana types (cancellation_source, manual_dispatcher), thread, env, suspension lock, and the Append template into a private Impl class defined in AppRuntime.cpp. This keeps arcana out of the public header so consumers don't need arcana's include directories. The worker thread is joined before Impl is destroyed, preserving the same lifetime guarantees as before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves arcana-dependent implementation details out of the public Babylon::AppRuntime header and into a private PIMPL (AppRuntime::Impl) defined in AppRuntime.cpp, avoiding arcana threading headers becoming transitive dependencies for consumers.
Changes:
- Introduces
AppRuntime::ImplinAppRuntime.cppto own the arcana dispatcher/cancellation/thread state and the internalAppendhelper. - Updates
AppRuntimemethods to delegate tom_implfor dispatching, suspension, cancellation, and thread joining. - Removes arcana threading includes and arcana-dependent members/templates from
Core/AppRuntime/Include/Babylon/AppRuntime.h.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Core/AppRuntime/Source/AppRuntime.cpp | Adds AppRuntime::Impl containing arcana threading state and routes runtime operations through it. |
| Core/AppRuntime/Include/Babylon/AppRuntime.h | Removes arcana headers and implementation details from the public API surface and introduces a forward-declared PIMPL pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The pimpl change made AppRuntime movable (unique_ptr is movable), but moving an AppRuntime with a running thread and captured this pointers in lambdas would be undefined behavior. Explicitly delete both copy and move to match the previous implicit behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[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.hheader. This exposedarcana/threading/cancellation.handarcana/threading/dispatcher.has transitive dependencies for all consumers, breaking builds in BabylonNative where arcana's include directories weren't propagated (PRIVATElink).The Fix
Move the arcana-dependent members (
cancellation_source,manual_dispatcher,thread,env,suspension lock) and theAppendtemplate into a privateImplclass defined inAppRuntime.cpp. The public header no longer includes any arcana headers.m_optionsstays onAppRuntimesince platform-specific.cppfiles access it directly.Explicitly delete copy and move constructors/operators — the pimpl
unique_ptrwould otherwise make AppRuntime movable, but moving with a running thread and capturedthispointers would be UB.Thread Safety
The worker thread is joined in
~AppRuntime()beforeunique_ptr<Impl>is destroyed, preserving the same lifetime guarantees as before. No split-lifetime issues.Verified
DestroyDoesNotDeadlocktest passes (210ms)