Skip to content

Move JavaScript creation responsibilities outside of N-API#149

Merged
syntheticmagus merged 19 commits intoBabylonJS:masterfrom
syntheticmagus:moveJavaScriptResponsibilitiesOutsideOfNapi
Feb 12, 2020
Merged

Move JavaScript creation responsibilities outside of N-API#149
syntheticmagus merged 19 commits intoBabylonJS:masterfrom
syntheticmagus:moveJavaScriptResponsibilitiesOutsideOfNapi

Conversation

@syntheticmagus
Copy link
Copy Markdown
Contributor

No description provided.

@syntheticmagus syntheticmagus marked this pull request as ready for review February 10, 2020 20:54
explicit Env(const char* executablePath, std::function<void(std::function<void()>)> executeOnScriptThread);
Env(const Env&) = delete;
~Env();
template<typename ...Ts> Napi::Env Attach(Ts... args);
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.

If we are expecting to push this back to node, I'm not sure the usage of variadic template arguments will be accepted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Talked offline. It wouldn't be Node's first use of variadics, so I think we'll be okay. If we're not, though, it shouldn't be too difficult to switch to another option (single template type, separate headers, or even [choke] arglist) when the objection is raised.

Comment thread Library/Source/RuntimeImplV8.cpp Outdated
Comment on lines +46 to +60
v8::Isolate* CreateIsolate(const char* executablePath)
{
Module::Initialize(executablePath);

v8::Isolate::CreateParams create_params;
create_params.array_buffer_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
return v8::Isolate::New(create_params);
}

void DestroyIsolate(v8::Isolate* isolate)
{
// todo : GetArrayBufferAllocator not available?
//delete isolate->GetArrayBufferAllocator();
isolate->Dispose();
}
Copy link
Copy Markdown
Contributor

@bghgary bghgary Feb 12, 2020

Choose a reason for hiding this comment

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

not sure these helper functions are useful anymore. perhaps inline them in BaseThreadProcedure?

@syntheticmagus syntheticmagus merged commit 3c9e393 into BabylonJS:master Feb 12, 2020
@syntheticmagus syntheticmagus deleted the moveJavaScriptResponsibilitiesOutsideOfNapi branch February 12, 2020 21:47
bghgary added a commit to bghgary/BabylonNative that referenced this pull request Apr 13, 2026
Includes:
- BabylonJS#147: Fix WorkQueue destructor deadlock
- BabylonJS#149: Merge WorkQueue into AppRuntime
- BabylonJS#150: Fix WebSocket test flake + UrlLib/AndroidExtensions updates
- BabylonJS#151: Clean up deadlock regression test
- BabylonJS#152: Fix performance test flake

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary added a commit that referenced this pull request Apr 14, 2026
[Created by Copilot on behalf of @bghgary]

Bump JsRuntimeHost, arcana.cpp, and AndroidExtensions to latest.

## JsRuntimeHost (8086014)
- **#147**: Fix WorkQueue destructor deadlock (cancel + no-op wake)
- **#149**: Merge WorkQueue into AppRuntime (eliminates split-lifetime
issues)
- **#150**: Fix WebSocket test flake + update UrlLib/AndroidExtensions
for consistent onClose-after-onError
- **#151**: Clean up deadlock regression test (one-shot hook, expanded
comments)
- **#152**: Fix performance test flake (remove upper-bound setTimeout
assertion)
- **#154**: Hide arcana behind pimpl pattern in AppRuntime (arcana
headers no longer exposed publicly)
- **#155**: Update package-lock.json

## arcana.cpp (b9bf9d8)
Includes test hooks for the deadlock regression test
(`ARCANA_TEST_HOOKS` / `arcana::test_hooks::blocking_concurrent_queue`).

## AndroidExtensions (2e85a8d)
Removes the redundant `errorCallback` from `onClose` in the Java
WebSocket wrapper (AndroidExtensions#16). Matches the version
JsRuntimeHost depends on via UrlLib.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants