Skip to content

Conversation

@adrian-niculescu
Copy link
Contributor

@adrian-niculescu adrian-niculescu commented Nov 20, 2025

This PR improves runtime initialization correctness and modernizes V8 API usage:

Context Management (RAII):
Adds Context::Scope RAII pattern to RunMainScript for proper context handling. The Context::Scope ensures the context is correctly exited when the scope ends, even if exceptions occur. This is the recommended V8 C++ API pattern per v8-locker.h documentation.

Note: Runtime::Init continues to use manual context->Enter() because the context must remain entered for the runtime's entire lifetime, not just during initialization.

Runtime ID Generation:
Introduces a static atomic counter (nextRuntimeId_) that generates unique IDs for each Runtime instance during initialization. This provides reliable runtime identification for both main and worker threads using thread-safe atomic operations with memory_order_relaxed (appropriate for ID generation).

The patterns align with modern C++ best practices and V8 API recommendations.

@adrian-niculescu adrian-niculescu changed the title feat(runtime): improve initialization with RAII patterns and runtime IDs feat: improve initialization with RAII patterns and runtime IDs Nov 20, 2025
@adrian-niculescu
Copy link
Contributor Author

adrian-niculescu commented Nov 20, 2025

Noticed the test failure now. On it.
L.E.: Fixed! Sorry, @NathanWalker! Reverted to context->Enter() in Runtime::Init- the context must outlive initialization. No matching Exit() needed; V8's isolate disposal handles cleanup.

…nd runtime IDs

- Add Context::Scope RAII pattern to RunMainScript for proper context handling
- Introduce atomic runtime ID generation for unique runtime identification
- Ensures proper context scope management during script execution
- Runtime IDs use thread-safe atomic operations for main and worker threads
@adrian-niculescu adrian-niculescu force-pushed the feat/runtime-init-improvements branch from c501ec0 to f36ecd7 Compare November 20, 2025 23:14
@NathanWalker NathanWalker merged commit 968043e into NativeScript:main Nov 21, 2025
3 checks passed
@adrian-niculescu adrian-niculescu deleted the feat/runtime-init-improvements branch November 21, 2025 09:25
@edusperoni
Copy link
Collaborator

The runtime ID change introduces a new bug where the main thread runtime is now considered a Worker. Also, you'll get duplicate runtime ids because the worker ids and runtime ids aren't generated from the same atomic int.

runtime->SetWorkerId(worker->WorkerId());

@NathanWalker please hold off merging until at least a proper review comes in.

I'm reverting this for now

@adrian-niculescu
Copy link
Contributor Author

The runtime ID change introduces a new bug where the main thread runtime is now considered a Worker. Also, you'll get duplicate runtime ids because the worker ids and runtime ids aren't generated from the same atomic int.

runtime->SetWorkerId(worker->WorkerId());

@NathanWalker please hold off merging until at least a proper review comes in.

I'm reverting this for now

Will look into this and come back to you.

@adrian-niculescu
Copy link
Contributor Author

adrian-niculescu commented Nov 21, 2025

The runtime ID change introduces a new bug where the main thread runtime is now considered a Worker. Also, you'll get duplicate runtime ids because the worker ids and runtime ids aren't generated from the same atomic int.

runtime->SetWorkerId(worker->WorkerId());

@NathanWalker please hold off merging until at least a proper review comes in.

I'm reverting this for now

@edusperoni, I apologize! You are right that the change is wrong in the current implementation due to IsRuntimeWorker() returning true for an id > 0.
I want to add that the change was not completely idiotic, just misplaced (should not have pushed it here): our fork that implemented support for ESM modules before you (you just recently did this in the 9.0.0 release) because we wanted to use dynamic imports; this was at a time when you didn't need this as bundling takes away the need for most situations where an import is needed. In our implementation the runtime id was used as a map key in the ESModule loader to ensure each runtime in the process gets its own one-time ESM setup. Sorry!

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.

3 participants