Experimental JS scripting integration#182
Conversation
WalkthroughReplaces the Lua/sol2 scripting stack with a Node.js/V8-based JavaScript runtime: removes Lua engines, bindings, sandbox, manifest/graph subsystems; adds NodeEngine, V8 builtins (vectors, quaternion, color, events, messages, imports/exports, console), package.json-based manifests, and refactors ResourceManager and scripting modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server as Server Instance
participant RM as ResourceManager
participant FS as Filesystem
participant Node as NodeEngine (V8)
Server->>RM: StartAllResources()
RM->>FS: Discover resources (scan for package.json)
FS-->>RM: list of resource paths
RM->>RM: BuildDependencyGraph()
RM->>Node: Initialize/Create Environment per resource (ESModule or CJS)
Node-->>RM: Environment ready (isolate/context)
RM->>Node: ExecuteFile(entryPoint)
Node-->>RM: Module loaded (or pending ESModule)
RM->>Server: emit resource started events via Events API
Server->>Node: On resource event (via Events/Exports/Imports)
sequenceDiagram
autonumber
participant Client as Client Instance
participant RM as ResourceManager
participant Node as NodeEngine (V8)
participant Network as Network Peer
Client->>RM: OnServerResourceList(list)
RM->>FS: ensure package.json exists / download if needed
RM-->>Client: mark resources synced
Client->>Node: Tick()/ProcessPendingResponses()
Node->>Node: JS event loop + Promises processed
Node->>Network: send RPCs for events/messages (via Messages/Exports)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/scripting/resource/resource.h (1)
131-135: Auto-restart doc doesn’t match implementation.The comment says the policy uses manifest config, but the current implementation uses fixed values (see
Resource::CanAutoRestart()inresource.cpp). Either wire the config or update the doc to avoid misleading behavior.
🤖 Fix all issues with AI agents
In `@code/framework/src/scripting/builtins/color.cpp`:
- Around line 3-5: The build can fail because toHex (in
code/framework/src/scripting/builtins/color.cpp) uses std::min and std::max but
<algorithm> is not included; add `#include` <algorithm> near the existing includes
(with <sstream>, <iomanip>, <cmath>) so toHex and any other uses of
std::min/std::max compile on strict toolchains.
In `@code/framework/src/scripting/builtins/events.cpp`:
- Around line 530-612: Events::EmitLocalCallback currently resolves the created
promise immediately after invoking handlers, so async handlers that return
promises are not awaited; modify EmitLocalCallback to mirror EmitInternal by
collecting each handler's return value, wrapping non-promise results as resolved
values, and aggregating them with Promise::All (or equivalent promise chaining)
so the resolver only resolves/rejects after all handler promises settle; use the
previously captured handlersToCall and eventArgs, invoke each function and
convert its return v8::Local<v8::Value> into a promise, store those promises in
a v8::Array, call v8::Promise::All on that array, and resolve/reject the
original resolver based on the aggregated promise outcome (ensure TryCatch
handling mirrors EmitInternal for error collection).
In `@code/framework/src/scripting/builtins/exports.cpp`:
- Around line 74-87: The code masks RegisterExport failures by always returning
true; change the logic in the export registration block (where
currentResource->RegisterExport(exportName, exportValue) is called and
args.GetReturnValue().Set(v8::True(isolate)) is set) to propagate real failure:
capture the bool result, log the manifest-only case as before, but if
RegisterExport returned false due to initialization/isolate issues (not "not in
manifest") then either set args.GetReturnValue().Set(v8::False(isolate)) or
throw a V8 exception instead of returning true—ensure you use the RegisterExport
result to decide between the debug log for manifest-missing vs propagating
failure to the caller.
- Around line 113-133: Ensure you validate isolate ownership before returning
cross-resource export values: in the Exports.get handler, after calling
resource->GetExportValue(exportName) check resource->GetIsolate() against the
current isolate and refuse/convert if they differ; either throw an exception
(safe short fix) or serialize/deserialize the value into the calling isolate
(e.g., use v8::ValueSerializer/Deserializer or JSON round-trip) and then call
args.GetReturnValue().Set(...) with the value bound to the calling isolate.
Identify this logic around the call sites for resource->GetExportValue,
resource->GetIsolate, isolate, and args.GetReturnValue().Set to implement the
check and the fallback conversion/throw.
In `@code/framework/src/scripting/builtins/messages.cpp`:
- Around line 172-194: Wrap the invocation of the message handler (where
handlerFn->Call(context, ... ) is executed) in logic that sets the current
resource context to the target resource and restores the previous context after
the call; specifically, before calling handlerFn (using payload and replyFn)
push/set the active resource to targetResource, then execute the
TryCatch/handlerFn->Call and the existing error/reject handling, and finally
restore the prior resource context (ensure this restoration runs even if
exceptions occur). Use the same pattern for the other occurrence around lines
245-257; reference symbols: handlerFn, payload, replyFn, context,
targetResource, tryCatch, _pendingRequests and _pendingRequestsMutex, and
requestId to locate and protect the invocation.
In `@code/framework/src/scripting/builtins/vector2.cpp`:
- Around line 21-107: The static _class in Vector2::GetClass is shared across
isolates, which is unsafe for v8pp types; change the storage to be per-isolate
(e.g., a map from v8::Isolate* to the unique_ptr or store the class object in
the isolate's private data) and update GetClass to look up/initialize the class
for the current isolate instead of a single static _class; ensure you also
attach the js_function_template static methods (func->Set(...)) to the
per-isolate class/template and mirror the same per-isolate change for other math
builtins (Vector3, Vector4, Quaternion, Color) to avoid cross-isolate reuse.
In `@code/framework/src/scripting/builtins/vector4.cpp`:
- Around line 21-136: The GetClass method currently reuses the static _class
across isolates which is unsafe; modify Vector4::GetClass to be isolate-aware by
replacing the single _class cache with a per-isolate cache (e.g.
std::map<v8::Isolate*, std::unique_ptr<v8pp::class_<Vector4>>> or an
unordered_map) or by storing the first-initialized isolate and asserting it
matches subsequent calls; update all references to _class inside
Vector4::GetClass to look up/insert the correct entry keyed by the v8::Isolate*
parameter and return the per-isolate v8pp::class_<Vector4> instance so each
isolate gets its own template (follow the same pattern used in
Vector2/Vector3/Quaternion/Color).
In `@code/framework/src/scripting/node_engine.cpp`:
- Around line 274-286: NodeEngine::ExecuteFile embeds absPathStr into a JS
require() string without normalizing backslashes, which causes Windows paths
like "C:\temp\file.js" to be treated as JS escape sequences; change the path
conversion so absPathStr is created from absPath.generic_string() (or otherwise
replace backslashes with '/') before building the code string, then pass that
normalized absPathStr into Execute(code, absPathStr) to ensure Node.js accepts
the path on all platforms (mirrors the approach used elsewhere in the codebase).
In `@code/framework/src/scripting/resource/resource_manager.cpp`:
- Around line 61-80: The debug log reads _resources[name]->GetVersion() outside
the lock, causing a race; while holding the _resourcesMutex in the
Add/registration code (where resource is created and inserted into _resources),
capture the version (e.g., call resource->GetVersion() or read
_resources[name]->GetVersion() before unlocking) into a local string variable,
then release the lock and use that local version in the
Logging::GetLogger(...)->debug(...) call; ensure you reference Resource,
_resources, and _resourcesMutex when making the change.
- Around line 99-112: ResourceManager::ValidateDependencies currently fails
immediately when a dependency is missing; update it to honor the configuration
flag (e.g., warnOnMissingDependency) by checking that flag inside the loop over
_dependencies and, if true, emit a warning (or append to outError as a non-fatal
message) for the missing depName instead of returning false, and only return
false if warnOnMissingDependency is false (or after collecting all missing deps
if you prefer aggregated error reporting); keep the existing locks (_graphMutex,
_resourcesMutex) and use the same identifiers (_dependencies, _resources,
outError, ValidateDependencies) so callers and concurrency semantics remain
unchanged.
- Around line 337-383: The resourceName and file path interpolations in
ResourceManager::ExecuteResourceScript (variables resourceName, escapedPath) and
in NodeEngine::ExecuteFile (absPathStr) are not safely escaped and Windows paths
are not converted to proper file:// URLs; add a helper (e.g.,
EscapeForSingleQuotedJSString) that escapes backslash, single-quote, newline,
carriage return and tab sequences, normalize paths with path.generic_string(),
convert filesystem paths to proper file URLs (use file:///C:/... for Windows
drive letters and file:// for POSIX), then replace usages where strings are
injected into JS (the import/require and
Framework.__internal.emitResourceStart/onLoadError calls) to use the escaped
resourceName and the canonical file URL escaped via the helper.
In `@code/framework/src/scripting/v8pp_types.h`:
- Around line 42-43: Declarations for convert<glm::vec2>::to_v8,
convert<glm::vec4>::to_v8, and convert<glm::quat>::to_v8 are missing
implementations which will cause linker errors; add implementations matching the
existing inline pattern used for glm::vec3 (or move all implementations to a
.cpp) by implementing static v8::Local<v8::Object> to_v8(v8::Isolate* isolate,
const glm::vec2&), to_v8(... const glm::vec4&), and to_v8(... const glm::quat&)
either inline in v8pp_types.h (mirroring the vec3 creation of a V8 Object and
setting x/y/z/w or x/y fields) or as definitions in a .cpp that include
v8pp_types.h so the linker can find convert<glm::vec2>::to_v8,
convert<glm::vec4>::to_v8, and convert<glm::quat>::to_v8.
🟡 Minor comments (13)
vendors/v8pp/CMakeLists.txt-16-21 (1)
16-21: Platform-specific plugin suffix hardcoded to macOS.
V8PP_PLUGIN_SUFFIXis set to.dylibwhich is macOS-specific. This should be platform-conditional.Proposed fix for cross-platform support
-set(V8PP_PLUGIN_SUFFIX ".dylib" CACHE STRING "v8pp plugin filename suffix") +if(WIN32) + set(V8PP_PLUGIN_SUFFIX ".dll" CACHE STRING "v8pp plugin filename suffix") +elseif(APPLE) + set(V8PP_PLUGIN_SUFFIX ".dylib" CACHE STRING "v8pp plugin filename suffix") +else() + set(V8PP_PLUGIN_SUFFIX ".so" CACHE STRING "v8pp plugin filename suffix") +endif()CLAUDE.md-39-39 (1)
39-39: Update documentation path reference fromscripting/js/toscripting/.The actual directory structure has JavaScript/TypeScript files directly under
code/framework/src/scripting/(containingnode_engine.h,v8_engine.h,engine.h, etc.) with no separatejs/subdirectory. The reference toscripting/js/should be corrected toscripting/.vendors/libnode/CMakeLists.txt-103-120 (1)
103-120: Symlinks use incorrect source path - should be relative or just the filename.The
file(CREATE_LINK)command creates absolute symlinks. When the source is also an absolute path to the same directory, the symlink should use a relative path or just the filename to ensure portability.Proposed fix for portable symlinks
# Create symlink for linker (libnode.dylib -> libnode.127.dylib) if(APPLE) if(NOT EXISTS "${LIBNODE_LIB_DIR}/libnode.dylib") file(CREATE_LINK - "${LIBNODE_LIB_DIR}/${LIBNODE_LIB_NAME}" + "${LIBNODE_LIB_NAME}" "${LIBNODE_LIB_DIR}/libnode.dylib" SYMBOLIC ) endif() elseif(UNIX) if(NOT EXISTS "${LIBNODE_LIB_DIR}/libnode.so") file(CREATE_LINK - "${LIBNODE_LIB_DIR}/${LIBNODE_LIB_NAME}" + "${LIBNODE_LIB_NAME}" "${LIBNODE_LIB_DIR}/libnode.so" SYMBOLIC ) endif() endif()code/framework/src/integrations/server/instance.cpp-396-410 (1)
396-410: Potential duplicate file addition for client entry point.The client entry point script is explicitly added at lines 389-394, but then the recursive directory iteration at lines 399-409 will add it again if it matches the
.js,.mjs,.ts, or.jsonextensions. This could cause duplicate entries in the asset streamer.Proposed fix - skip the already-added entry point
// If the client entry is in a subdirectory, add all JS files from that directory std::filesystem::path clientDir = clientEntryPath.parent_path(); if (clientDir != resourcePath && std::filesystem::exists(clientDir)) { for (const auto &entry : std::filesystem::recursive_directory_iterator(clientDir)) { if (!entry.is_regular_file()) continue; + + // Skip the client entry point (already added above) + if (entry.path() == clientEntryPath) continue; // Add JavaScript and TypeScript files const auto ext = entry.path().extension().string();code/framework/src/scripting/v8pp_types.h-24-27 (1)
24-27: Add#include <stdexcept>forstd::invalid_argument.The file uses
std::invalid_argumentin four places (lines 26, 57, 91, and 128) but doesn't explicitly include<stdexcept>. While the code may compile due to indirect includes, explicit inclusion of<stdexcept>is required for this exception type.Proposed fix
`#include` <v8pp/convert.hpp> `#include` <glm/glm.hpp> `#include` <glm/gtc/quaternion.hpp> +#include <stdexcept>code/framework/src/scripting/v8_engine.cpp-136-138 (1)
136-138: Potential integer overflow when casting string length toint.
code.length()returnssize_twhich is cast toint. For strings larger thanINT_MAX(~2GB), this causes undefined behavior. While unlikely in practice, V8's API acceptsintfor historical reasons.Suggested defensive check
+ if (code.length() > static_cast<size_t>(std::numeric_limits<int>::max())) { + _lastError = "Code string too large"; + return false; + } + v8::Local<v8::String> source = v8::String::NewFromUtf8(_isolate, code.c_str(), v8::NewStringType::kNormal, static_cast<int>(code.length())) .ToLocalChecked();code/framework/src/scripting/v8_engine.cpp-180-187 (1)
180-187:GetFrameworkObjectlacks error handling for missing or invalid global.If "Framework" doesn't exist on the global object or isn't an object,
ToLocalmay fail andAs<v8::Object>()on an empty/non-object value causes undefined behavior.Suggested safer implementation
v8::Local<v8::Object> V8Engine::GetFrameworkObject() const { v8::Local<v8::Context> context = _context.Get(_isolate); v8::Local<v8::Value> frameworkValue; - context->Global() - ->Get(context, v8::String::NewFromUtf8(_isolate, "Framework").ToLocalChecked()) - .ToLocal(&frameworkValue); - return frameworkValue.As<v8::Object>(); + if (!context->Global() + ->Get(context, v8::String::NewFromUtf8(_isolate, "Framework").ToLocalChecked()) + .ToLocal(&frameworkValue) || !frameworkValue->IsObject()) { + return v8::Local<v8::Object>(); + } + return frameworkValue.As<v8::Object>(); }code/framework/src/scripting/builtins/vector4.h-37-37 (1)
37-37: Division by zero not handled indiv().If
scalaris zero,glm::vec4 /= 0produces infinity/NaN. Consider adding a check or documenting the behavior.Suggested defensive check
- Vector4& div(float scalar) { _vec /= scalar; return *this; } + Vector4& div(float scalar) { + if (scalar != 0.0f) { + _vec /= scalar; + } + return *this; + }Alternatively, let it produce infinity/NaN (IEEE 754 behavior) but document this in the class comment.
code/framework/src/scripting/builtins/console.cpp-77-90 (1)
77-90: Unsafe casts when retrieving JSON.stringify.The chained
Get().ToLocalChecked().As<v8::Object/Function>()calls assume success without verification. IfJSONorstringifydon't exist (e.g., in a modified environment), this will crash.Suggested safer implementation
// Try to get JSON representation - v8::Local<v8::Object> json = context->Global() - ->Get(context, v8pp::to_v8(isolate, "JSON")).ToLocalChecked().As<v8::Object>(); - v8::Local<v8::Function> stringify = json - ->Get(context, v8pp::to_v8(isolate, "stringify")).ToLocalChecked().As<v8::Function>(); - - v8::Local<v8::Value> jsonArgs[1] = {obj}; - v8::MaybeLocal<v8::Value> jsonResult = stringify->Call(context, json, 1, jsonArgs); - - if (!jsonResult.IsEmpty()) { - ss << v8pp::from_v8<std::string>(isolate, jsonResult.ToLocalChecked()); - } else { + v8::Local<v8::Value> jsonValue; + if (context->Global()->Get(context, v8pp::to_v8(isolate, "JSON")).ToLocal(&jsonValue) && + jsonValue->IsObject()) { + v8::Local<v8::Object> json = jsonValue.As<v8::Object>(); + v8::Local<v8::Value> stringifyValue; + if (json->Get(context, v8pp::to_v8(isolate, "stringify")).ToLocal(&stringifyValue) && + stringifyValue->IsFunction()) { + v8::Local<v8::Function> stringify = stringifyValue.As<v8::Function>(); + v8::Local<v8::Value> jsonArgs[1] = {obj}; + v8::MaybeLocal<v8::Value> jsonResult = stringify->Call(context, json, 1, jsonArgs); + if (!jsonResult.IsEmpty()) { + ss << v8pp::from_v8<std::string>(isolate, jsonResult.ToLocalChecked()); + } else { + ss << "[Object]"; + } + } else { + ss << "[Object]"; + } + } else { ss << "[Object]"; }code/framework/src/scripting/builtins/color.cpp-47-55 (1)
47-55: Validate hex length before parsing.The current logic accepts strings longer than 8 and silently ignores extra chars. Enforcing length 6 or 8 avoids ambiguous inputs.
🛠️ Suggested fix
- try { - if (hex.length() >= 6) { - r = static_cast<float>(std::stoi(hex.substr(0, 2), nullptr, 16)) / 255.0f; - g = static_cast<float>(std::stoi(hex.substr(2, 2), nullptr, 16)) / 255.0f; - b = static_cast<float>(std::stoi(hex.substr(4, 2), nullptr, 16)) / 255.0f; - } - if (hex.length() >= 8) { - a = static_cast<float>(std::stoi(hex.substr(6, 2), nullptr, 16)) / 255.0f; - } - } catch (...) { - // Invalid hex, use defaults - } + if (hex.length() != 6 && hex.length() != 8) { + return Color(r, g, b, a); + } + try { + r = static_cast<float>(std::stoi(hex.substr(0, 2), nullptr, 16)) / 255.0f; + g = static_cast<float>(std::stoi(hex.substr(2, 2), nullptr, 16)) / 255.0f; + b = static_cast<float>(std::stoi(hex.substr(4, 2), nullptr, 16)) / 255.0f; + if (hex.length() == 8) { + a = static_cast<float>(std::stoi(hex.substr(6, 2), nullptr, 16)) / 255.0f; + } + } catch (...) { + // Invalid hex, use defaults + }code/framework/src/scripting/builtins/color.h-3-4 (1)
3-4: Mark theColorclass asfinal.The class should be marked
finalper coding guidelines. The v8pp includes are correctly configured in the build system and will resolve properly.Suggested change
-class Color { +class Color final {code/framework/src/scripting/builtins/quaternion.cpp-30-33 (1)
30-33: Guard zero-length axis infromAxisAngle.
glm::normalizeon a zero-length vector produces NaN components due to undefined behavior; return identity quaternion when axis length is 0 to avoid invalid quaternions.Suggested fix
Quaternion Quaternion::fromAxisAngle(const Vector3& axis, float angle) { - glm::vec3 normalizedAxis = glm::normalize(axis.vec()); - return Quaternion(glm::angleAxis(angle, normalizedAxis)); + glm::vec3 axisVec = axis.vec(); + float len = glm::length(axisVec); + if (len <= 0.0f) { + return Quaternion::identity(); + } + glm::vec3 normalizedAxis = axisVec / len; + return Quaternion(glm::angleAxis(angle, normalizedAxis)); }code/framework/src/integrations/server/scripting/module.cpp-156-170 (1)
156-170: Clear g_resourceManager in Shutdown to avoid a dangling pointer.If Shutdown is invoked without PreShutdown, the global pointer can outlive the ResourceManager and be dereferenced by internal callbacks. Resetting it in Shutdown provides a safe fallback.
🔧 Suggested fix
bool ServerScriptingModule::Shutdown() { _resourceManager.reset(); + g_resourceManager = nullptr; if (_nodeEngine) { _nodeEngine->Shutdown(); _nodeEngine.reset(); }
🧹 Nitpick comments (27)
code/tests/modules/resource_manager_ut.h (2)
48-52: Consider adding error handling for file operations.The file write operations don't check for failures. While this is test code, silent failures could lead to confusing test results if the filesystem has issues (permissions, disk full, etc.).
std::string packagePath = resourcePath + "/package.json"; std::ofstream packageFile(packagePath); + if (!packageFile.is_open()) { + throw std::runtime_error("Failed to create package.json at: " + packagePath); + } packageFile << packageJson; packageFile.close();
520-522: Consider test isolation for shared temp directory.The tests share a common temp directory path (
GetTestResourcePath()). While most tests callCleanup()at the end, if a test fails mid-execution, subsequent tests may encounter stale resources from previous runs. The tests rely on the final cleanup test (line 520-522) to clean up, but this only runs if all previous tests pass.Consider calling
TestManagerHelper::Cleanup()at the beginning of each test that creates resources, not just at the end, to ensure a clean state regardless of previous test failures.code/tests/modules/engine_ut.h (1)
49-69: Verify callback engine pointer identity.The test captures
callbackEnginebut only checks it's non-null. Consider also verifying it points to the same engine instance:EQUALS(callbackEngine, &engine);vendors/v8pp/CMakeLists.txt (1)
28-53: Fragile string-based patching relies on exact whitespace matching.The
string(REPLACE)depends on exact tab characters and formatting in the upstream file. If v8pp reformats this code in a future release, the patch will silently fail to apply.Consider:
- Using
file(STRINGS ... REGEX)with a more robust pattern- Applying a proper
.patchfile withexecute_process(... patch ...)- Documenting the expected v8pp version and noting that patches may need updating
The guard check for
VisitHandlesWithClassIdsis good, but success of the replacement isn't verified.code/framework/src/scripting/builtins/events.h (1)
93-100: Consider documenting ownership semantics for_resourceManager.The raw pointer
_resourceManagerhas unclear ownership. Since this is set viaRegister()and used throughout the class lifetime, consider:
- Adding a comment clarifying that the pointer is non-owning and must outlive the Events system
- Using a reference or
std::reference_wrapperif null is never validThis is a minor documentation concern for maintainability.
code/framework/src/scripting/builtins/imports.h (1)
18-37: Static_resourceManagerlimits to single-instance usage.The static
ResourceManager *_resourceManagermember means only oneResourceManagerinstance can be active globally. This is acceptable for a single-context application but could cause issues if multiple V8 contexts with different resource managers are needed in the future.Also consider marking the class as
finalsince it appears to be a utility class not intended for inheritance.Optional: Mark class as final
- class Imports { + class Imports final {code/framework/src/scripting/engine.cpp (1)
3-7: Consider removing theengine.cppfile if it contains only comments.The
Enginebase class defines only pure virtual methods (Init, Shutdown, Execute, ExecuteFile, InitFrameworkSDK, GetIsolate, GetContext) and inline utility methods (SetSDKRegisterCallback, GetLastError, IsInitialized). Since all implementation lives inengine.h, the.cppfile adds unnecessary build overhead. Remove it entirely unless your build system requires it as a placeholder.code/framework/src/scripting/resource/package_manifest.cpp (1)
24-30: Consider validating thatnameis non-empty.The code checks that
nameexists and is a string, but doesn't validate that it's not an empty string. An empty name could cause issues downstream.Proposed fix
if (!json.contains("name") || !json["name"].is_string()) { _error = "Missing required field: name"; return false; } _name = json["name"].get<std::string>(); + if (_name.empty()) { + _error = "Field 'name' cannot be empty"; + return false; + }code/framework/src/scripting/builtins/messages.h (1)
45-71: Consider documenting cleanup requirements for static V8 globals.The static
_handlers,_pendingRequests, and_responseQueuecontainers holdv8::Globalhandles. These need to be reset before the V8 isolate is disposed, otherwise they may cause crashes or memory issues.Consider adding a static
Shutdown()orCleanup()method to clear these containers, and document when it should be called in the resource lifecycle.code/framework/src/scripting/builtins/imports.cpp (1)
85-107: Cross-resource export values not yet implemented.The comments at lines 85-91 acknowledge that the current implementation only returns the list of available export names (
_availableExports), not the actual exported values. This is a known limitation due to V8 context isolation.Consider adding a TODO comment or tracking issue for completing cross-resource value transfer (possibly via value serialization or a shared context approach).
Would you like me to open an issue to track the implementation of actual cross-resource export value retrieval?
code/framework/src/scripting/v8_engine.h (1)
17-61: MarkV8Engineasfinalto seal the API against accidental subclassing.V8Engine is not subclassed anywhere in the codebase and is used compositionally, so marking it
finalis safe and follows the coding guidelines for C++.✏️ Proposed change
-class V8Engine : public Engine { +class V8Engine final : public Engine {code/framework/src/scripting/builtins/vector3.cpp (1)
21-126: Add isolate-safety assertion or per-isolate caching to align with V8 best practices.The static
_classcache pattern is used consistently across all builtin types (Vector3, Vector2, Quaternion, Color, Vector4), suggesting this is the framework's architectural standard for single-isolate scenarios. However,v8pp::class_objects are isolate-specific, and reusing the cache across different isolates would be unsafe.While the current framework design appears to enforce a single isolate per engine (each Engine holds one
v8::Isolate*, ResourceManager holds one Engine), the code lacks explicit safeguards. If future development introduces multi-isolate support or if the architecture changes, this pattern would become a latent bug.Consider either:
- Adding a compile-time or runtime assertion that the isolate matches the cached instance:
assert(isolate == _cachedIsolate)- Implementing per-isolate caching using a thread-safe map:
std::map<v8::Isolate*, std::unique_ptr<v8pp::class_<Vector3>>>- Documenting the single-isolate design assumption explicitly in the Engine class
code/framework/src/scripting/node_engine.h (1)
18-18: Consider markingNodeEngineasfinal.As per coding guidelines, classes should be marked
finalwhen possible. SinceNodeEngineis a concrete implementation and there's no apparent need for further derivation, marking itfinalenables compiler optimizations.Suggested change
- class NodeEngine : public Engine { + class NodeEngine final : public Engine {code/framework/src/scripting/v8_engine.cpp (1)
40-56: Shutdown is correctly guarded but allocator deletion could be safer.The
_initializedcheck prevents most issues, but consider nullifying before delete to be defensive against any re-entrant edge cases:Suggested ordering
- delete _createParams.array_buffer_allocator; - _createParams.array_buffer_allocator = nullptr; + auto* allocator = _createParams.array_buffer_allocator; + _createParams.array_buffer_allocator = nullptr; + delete allocator;code/framework/src/scripting/resource/package_manifest.h (2)
5-5: Unused include<optional>.The
<optional>header is included but not used in this file. Consider removing it to keep includes minimal.Suggested change
`#include` <string> `#include` <vector> -#include <optional>
38-38: Consider markingPackageManifestasfinal.As per coding guidelines, classes should be marked
finalwhen possible.PackageManifestis a concrete data class with no virtual methods.Suggested change
- class PackageManifest { + class PackageManifest final {code/framework/src/scripting/builtins/console.cpp (2)
115-173: Consider extracting common callback logic.The four callback functions (
LogCallback,WarnCallback,ErrorCallback,DebugCallback) share identical boilerplate. A template or helper function could reduce duplication.Example refactor approach
template<typename LogFunc> void ConsoleCallback(const v8::FunctionCallbackInfo<v8::Value> &args, LogFunc logFunc) { v8::Isolate *isolate = args.GetIsolate(); v8::HandleScope handleScope(isolate); ResourceManager *manager = static_cast<ResourceManager *>(args.Data().As<v8::External>()->Value()); std::string resourceName = GetResourceContextForConsole(isolate, manager); std::string message = Console::FormatArgs(isolate, args); auto logger = Logging::GetLogger(FRAMEWORK_INNER_SCRIPTING); if (!resourceName.empty()) { logFunc(logger, "[{}] {}", resourceName, message); } else { logFunc(logger, "{}", message); } }
10-15: Static_resourceManagerlimits to single engine instance.The static pointer assumes a single Console registration per process. If multiple V8 engines are created (e.g., for testing), later registrations will overwrite earlier ones. This matches the pattern in
Imports/Exports/Events, so it's consistent but worth documenting as a known limitation.code/framework/src/scripting/builtins/vector4.h (2)
15-15: Consider markingVector4asfinal.As per coding guidelines, classes should be marked
finalwhen possible.Vector4is a concrete wrapper class.Suggested change
-class Vector4 { +class Vector4 final {
42-45:Vector4lacksdistance()method present inVector3.The relevant code snippets show
Vector3has adistance(const Vector3&)method. For API consistency,Vector4might benefit from the same, though it may be intentionally omitted if 4D distance is less commonly needed.code/framework/src/scripting/builtins/color.h (1)
16-16: Consider markingColorasfinal.It’s a value wrapper with no virtual interface;
finalhelps prevent unintended inheritance. As per coding guidelines, use PascalCase for class names in C++ and mark classes asfinalwhen possible.♻️ Suggested change
-class Color { +class Color final {code/framework/src/scripting/builtins/vector3.h (1)
15-15: Consider markingVector3asfinal.It’s a value wrapper with no virtual interface;
finalhelps prevent unintended inheritance. As per coding guidelines, use PascalCase for class names in C++ and mark classes asfinalwhen possible.♻️ Suggested change
-class Vector3 { +class Vector3 final {code/framework/src/scripting/builtins/console.h (1)
21-21: Consider markingConsoleasfinal.It’s a static utility wrapper with no intended inheritance path. As per coding guidelines, use PascalCase for class names in C++ and mark classes as
finalwhen possible.♻️ Suggested change
-class Console { +class Console final {code/framework/src/scripting/builtins/vector2.h (1)
15-15: Consider markingVector2asfinal.It’s a value wrapper with no virtual interface;
finalhelps prevent unintended inheritance. As per coding guidelines, use PascalCase for class names in C++ and mark classes asfinalwhen possible.♻️ Suggested change
-class Vector2 { +class Vector2 final {code/framework/src/integrations/server/scripting/module.h (1)
27-30: Consider marking ServerScriptingModule as final.If this class isn’t intended for inheritance, marking it final clarifies the intent and can enable small compiler optimizations.
As per coding guidelines, Use PascalCase for class names in C++ and mark classes as `final` when possible.🔧 Proposed change
- class ServerScriptingModule { + class ServerScriptingModule final {code/framework/src/scripting/node_engine.cpp (1)
20-35: Optional: clear _lastError before init to avoid stale messages.It can be helpful to reset the error buffer at the start of Init so callers only see the most recent failure.
🔧 Suggested change
bool NodeEngine::Init() { + _lastError.clear(); if (_initialized) { return true; }code/framework/src/integrations/client/scripting/module.h (1)
38-40: Considerfinalif this module isn’t meant to be subclassed.Keeps inheritance intent explicit and aligns with project conventions.
As per coding guidelines: Use PascalCase for class names in C++ and mark classes as `final` when possible.♻️ Suggested tweak
-class ClientScriptingModule { +class ClientScriptingModule final {
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/framework/src/scripting/builtins/events.cpp`:
- Around line 69-88: The error message in RegisterHandler hardcodes "Events.on"
which is incorrect for callers using once; update RegisterHandler to compute a
caller-agnostic method name (e.g., set std::string methodName = once ?
"Events.once" : "Events.on" or use a generic "Events.*") and use that methodName
in the v8 exception text instead of the hardcoded "Events.on"; modify the
exception construction in RegisterHandler (referencing RegisterHandler,
EventHandler, and _globalHandlers) to include the chosen methodName so the
thrown message accurately reflects the caller.
- Around line 338-343: EmitInternal currently gets the global Promise and calls
ToLocalChecked().As<v8::Function>() on its allSettled property which will crash
if Promise is missing or allSettled is not a function; change the code in
EmitInternal to first v8::MaybeLocal<v8::Value> getPromise =
context->Global()->Get(context, v8pp::to_v8(isolate, "Promise")); check
getPromise.IsEmpty() and that getPromise.ToLocalChecked()->IsObject(), then
fetch allSettled via MaybeLocal and verify !allSettled.IsEmpty() and
allSettled->IsFunction() before casting to v8::Function; if any check fails,
handle gracefully (e.g., fallback to using Promise::Resolve with manual
aggregation or return an immediately resolved promise) and apply the identical
guard pattern to EmitLocalCallback where the same Promise/allSettled access
occurs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/framework/src/scripting/builtins/messages.cpp`:
- Around line 152-175: The reply handling uses a raw uint64_t* (requestIdPtr)
passed into replyCallback and later deleted again in the error path, causing
double-free and potential use-after-free when reply() is called multiple times;
replace the raw pointer with an ownership-tracking object (e.g.,
std::shared_ptr<uint64_t> or a small struct { uint64_t id; std::atomic<bool>
consumed; }) and pass that into V8 instead of requestIdPtr, update replyCallback
(the lambda replyCallback) to atomically check-and-set the consumed flag before
pushing PendingResponse into _responseQueue, and remove all manual delete calls
(and in the error handler check the consumed flag before creating/pushing an
error PendingResponse) so the id is only used once and ownership is managed
safely for replyFn and the error path (refer to requestIdPtr, replyCallback,
replyFn, PendingResponse, and _responseQueue).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/framework/src/scripting/builtins/messages.cpp`:
- Around line 141-210: The ReplyContext raw pointer can dangle when
_pendingRequests erases the entry; change replyFn to receive the requestId (as a
v8::BigInt) instead of an External ReplyContext pointer, and in the
replyCallback look up the PendingRequest under _pendingRequestsMutex using that
requestId; if not found return, otherwise operate on the PendingRequest
(check/set a consumed flag that you move into PendingRequest) and push the
response into _responseQueue; update code paths that create the function
(replace External::New(replyContext.get()) with a BigInt of requestId) and
remove direct dereferences of ReplyContext in replyCallback.
In `@vendors/libnode/CMakeLists.txt`:
- Around line 175-186: The v8 INTERFACE target currently claims to allow clients
to use V8 without linking Node.js but unconditionally links libnode via
target_link_libraries(v8 INTERFACE libnode), causing the contradiction; either
remove that unconditional link so the v8 target only exposes headers (delete the
target_link_libraries(v8 INTERFACE libnode) line) and leave a comment explaining
how consumers that need libnode can explicitly link libnode, or if libnode is
required for your consumers, update the comment to state that v8 will pull in
libnode and that V8Engine remains pure V8 but consumers must link libnode;
modify CMakeLists to implement one of these two consistent behaviors referencing
the v8 target, target_link_libraries call, and V8Engine usage.
🧹 Nitpick comments (1)
code/tests/CMakeLists.txt (1)
19-21: Consider using target dependency instead of file dependency.The
DEPENDS $<TARGET_FILE:FrameworkTests>uses a generator expression for the file path, butadd_custom_targetDEPENDS should reference targets directly for proper dependency tracking. The target already implicitly depends on the executable being built since the COMMAND uses it.♻️ Suggested simplification
add_custom_target(RunFrameworkTests COMMAND $<TARGET_FILE:FrameworkTests> - DEPENDS $<TARGET_FILE:FrameworkTests>) + DEPENDS FrameworkTests)
…ub/Framework into experimental/javascript-scripting
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/framework/src/scripting/builtins/events.cpp`:
- Around line 794-803: Add a mutex guard around accesses to
_additionalReservedEvents to avoid the data race: in Events::AddReservedEvents
and Events::IsEventReserved, acquire std::lock_guard<std::mutex>
lock(_handlersMutex); at the start of each method and then perform the insert
and find operations respectively. Keep the existing IsReservedEvent(eventName)
check but ensure the lock is held when querying _additionalReservedEvents to
match the protection used for _globalHandlers/_localHandlers.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@code/framework/src/integrations/server/scripting/module.cpp`:
- Around line 12-66: The global g_resourceManager causes cross-instance
interference; update EmitResourceStartCallback and OnLoadErrorCallback to obtain
the Framework::Scripting::ResourceManager* from V8 (e.g., args.Data() as a
v8::External or a context/internal field) instead of reading g_resourceManager,
and change RegisterInternalFunctions to accept a ResourceManager* and create a
v8::External with that pointer when constructing the FunctionTemplate(s) (or
store it on the created internalObj/context) so each ServerScriptingModule
instance binds its own ResourceManager to the callbacks.
In `@code/framework/src/scripting/node_engine.cpp`:
- Around line 200-305: The code unconditionally sets importModuleDynamically to
vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER in NodeEngine::Execute which allows
dynamic import() to bypass the sandbox; change the logic so that when the
sandbox has been applied (detect via the existing sandbox marker used by
ApplySandbox(), e.g. a flag or method on _setup like isSandboxed()/sandboxed()
or whatever ApplySandbox() sets), do NOT set importModuleDynamically to the
default loader (omit the property or set it to undefined/false), and only set
importModuleDynamically to USE_MAIN_CONTEXT_DEFAULT_LOADER when not sandboxed;
update the block that creates options (references: NodeEngine::Execute, options,
importModuleDynamically, vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER, and
ApplySandbox) to conditionally add that property based on the sandbox state.
- Around line 307-323: NodeEngine::ExecuteFile currently calls
std::filesystem::absolute() which can throw; change it to use the error_code
overload (std::error_code ec; auto absPath = std::filesystem::absolute(filepath,
ec);) and check ec before proceeding; on error set _lastError to a descriptive
message including ec.message() and return false, and only build escapedPath and
call Execute(code, absPathStr) when no filesystem error occurred. Ensure you
reference NodeEngine::ExecuteFile, EscapeForSingleQuotedJSString, and _lastError
when implementing this graceful failure path.
- Around line 95-129: InitializeNode() reads and sets the non-atomic flag
_platformInitialized and calls process-wide functions
v8::V8::InitializePlatform(...) and v8::V8::Initialize() without
synchronization; add a static std::mutex (e.g., static std::mutex
s_platformMutex) and guard the check-and-initialize sequence with a lock to
prevent concurrent initialization: perform a fast-path check of
_platformInitialized, then std::lock_guard<std::mutex> guard(s_platformMutex),
re-check _platformInitialized, and only then run
node::InitializeOncePerProcess(...), process _initResult, create _platform via
node::MultiIsolatePlatform::Create(...), and call
v8::V8::InitializePlatform(...) and v8::V8::Initialize(); ensure any early
returns set/leave _platformInitialized correctly under the lock.
🧹 Nitpick comments (6)
code/tests/modules/engine_ut.h (1)
49-69: Consider verifying callback receives the correct engine instance.The test verifies the callback is invoked and receives a non-null engine pointer, but doesn't verify that
callbackEngine == &engine. This would strengthen the test.🔧 Suggested improvement
EQUALS(callbackInvoked, true); NEQUALS(callbackEngine, nullptr); + EQUALS(callbackEngine, static_cast<Engine*>(&engine)); engine.Shutdown();code/tests/modules/js_features_ut.h (1)
22-51: Magic sentinel values could collide with valid JS results.The error sentinel values (-999999, -999998, -999997) could theoretically be returned by valid JavaScript code. Consider using a result type or out-parameter pattern instead.
Alternative approach using std::optional
// Alternative: Return std::optional<int32_t> where std::nullopt indicates error static std::optional<int32_t> RunJS(Framework::Scripting::NodeEngine &engine, const char *code) { // ... setup code ... if (tryCatch.HasCaught() || maybeResult.IsEmpty()) { return std::nullopt; } // ... success path ... return result->Int32Value(context).FromJust(); }However, for test code where the specific JS snippets are controlled, the current approach is pragmatic and unlikely to cause issues in practice.
code/framework/src/integrations/client/scripting/module.h (1)
119-122: Consider thread-safety implications ofSetServerResourceList.The method directly sets
_resourcesSynced = truewithout synchronization. IfAreResourcesSynced()is called from a different thread (e.g., render thread checking sync status), this could cause a data race.If multi-threaded access is expected, consider using
std::atomic<bool>for_resourcesSynced:- bool _resourcesSynced = false; + std::atomic<bool> _resourcesSynced{false};code/framework/src/integrations/server/scripting/module.cpp (1)
162-180: Potential double-reset of g_resourceManager in shutdown path.Both
PreShutdown()andShutdown()setg_resourceManager = nullptr. While harmless, it suggests the global management could be cleaner. If the global approach is kept, consider only resetting in one place.code/framework/src/scripting/node_engine.cpp (2)
8-26: Consolidate JS string escaping to avoid drift.This helper duplicates
EscapeForSingleQuotedJSStringincode/framework/src/scripting/resource/resource_manager.cpp(lines 13-28). Consider moving it to a shared utility so escaping rules stay consistent.
351-556: Clarify/align the sandbox module policy.
allowedModulesis never used, and the header comments mention restrictions (e.g.,osmethods,require.resolvelimits) that aren't enforced here. Consider enforcing an allowlist or removing the unused list and updating docs to match the actual policy.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/framework/src/integrations/client/instance.cpp`:
- Around line 408-420: The code currently calls
v8::String::NewFromUtf8(...).ToLocalChecked() and wraps parsing in a C++
try/catch, which can abort on invalid input and doesn't follow V8 error
patterns; replace this with proper MaybeLocal and TryCatch usage: call
v8::String::NewFromUtf8(...) and use .ToLocal(localString) (check the result)
before proceeding, and wrap v8::JSON::Parse with a v8::TryCatch to detect and
log V8 exceptions (use context and isolate), log a clear error via
Logging::GetLogger(FRAMEWORK_INNER_CLIENT) if NewFromUtf8 or JSON::Parse fails
(include payloadStr or error message), and remove the std::exception catch
block; ensure args.push_back(parsed) only after successful conversions and
parsing.
In `@code/framework/src/integrations/client/scripting/module.cpp`:
- Around line 28-69: ClientScriptingModule::Init currently skips calling
RegisterFrameworkBindings() when engineAlreadyInitialized is true, which leaves
a newly created _resourceManager without its Events registered after Reset();
change Init so RegisterFrameworkBindings() is invoked for the new
ResourceManager even when the engine was already initialized—e.g., after
creating _resourceManager (the std::make_unique call) call
RegisterFrameworkBindings() unconditionally or guarded by whether
_resourceManager was just (re)created, while keeping
_nodeEngine->InitFrameworkSDK() only on the first init (when
!engineAlreadyInitialized); ensure RegisterFrameworkBindings() runs before
returning so _resourceManager->GetEvents() is registered.
🧹 Nitpick comments (1)
code/framework/src/integrations/client/scripting/module.h (1)
166-169: Consider makingRegisterFrameworkBindings()private.This method operates on internal state and is called from
Init(). Unless there's an explicit need for external access (e.g., mod customization or testing), making it private would better encapsulate the implementation.♻️ Suggested change
Move the declaration under the
private:section at line 171:- /** - * Register the Framework SDK bindings in the engine. - */ - void RegisterFrameworkBindings(); - private: + /** + * Register the Framework SDK bindings in the engine. + */ + void RegisterFrameworkBindings(); + std::unique_ptr<Framework::Scripting::NodeEngine> _nodeEngine;
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/framework/src/integrations/client/instance.cpp`:
- Around line 388-429: This code calls V8 and libuv-bound handlers directly from
the network thread (using v8::Locker), which can race with the scripting/event
loop; instead, change the flow so the RPC handler does not call
resourceManager->GetEvents().EmitReserved(...) directly but pushes a small event
record (isolate-agnostic data: eventName string and payloadStr) into a
thread-safe queue owned by the scripting module/engine, and then return; update
NodeEngine::Tick() (or the scripting thread tick handler) to drain that queue on
the scripting/libuv thread, reconstruct the V8 values (parse JSON and create V8
handles) and call resourceManager->GetEvents().EmitReserved(isolate, context,
eventName, args) there within the V8 context; reference the EmitReserved call,
the network RPC handler block that currently parses payloadStr and calls
EmitReserved, and NodeEngine::Tick() as the place to move emission and
JSON-to-V8 parsing.
🧹 Nitpick comments (6)
code/framework/src/scripting/resource/package_manifest.cpp (1)
79-92: Consider logging a warning for malformed dependency entries.When a dependency object has an empty or missing
namefield (line 89), it's silently skipped. This could make debugging malformedpackage.jsonfiles harder for users.💡 Optional improvement
if (!rd.name.empty()) { _mafiahubConfig.resourceDependencies.push_back(rd); + } else { + // Optionally log a warning about malformed dependency }code/framework/src/scripting/resource/resource_manager.cpp (1)
11-47: Consider extracting shared utilities to avoid code duplication.
EscapeForSingleQuotedJSStringis duplicated here and innode_engine.cpp(lines 10-25 per the relevant code snippets). While both are in anonymous namespaces, this creates a maintenance burden.Consider moving these utilities to a shared header (e.g.,
js_utils.h) to avoid duplication:// js_utils.h namespace Framework::Scripting::Utils { std::string EscapeForSingleQuotedJSString(const std::string &input); std::string PathToFileURL(const std::filesystem::path &absPath); }vendors/CMakeLists.txt (1)
75-81: Consider gating libnode/v8pp behind a feature flag or platform guard.Unconditional inclusion can make non-JS or unsupported platform builds fail. A build option keeps the dependency surface optional and clearer.
♻️ Suggested gating option
+option(FW_ENABLE_JS_SCRIPTING "Build Node/V8-based scripting stack" ON) +if (FW_ENABLE_JS_SCRIPTING) add_subdirectory(libnode) add_subdirectory(v8pp) +endif()code/framework/src/scripting/builtins/messages.cpp (1)
301-318: Avoid resolving/rejecting promises while holding_pendingRequestsMutex.Resolving can queue microtasks and potentially re-enter messaging APIs; keeping the mutex held risks deadlocks and unnecessary contention. Consider extracting the resolver under lock and resolving after unlocking.
🔧 Suggested lock-scope reduction
for (auto &response : responses) { - std::lock_guard<std::mutex> lock(_pendingRequestsMutex); - auto it = _pendingRequests.find(response.requestId); - if (it == _pendingRequests.end()) { - continue; - } - - v8::Local<v8::Promise::Resolver> resolver = it->second.resolver.Get(isolate); - v8::Local<v8::Value> responseValue = response.response.Get(isolate); - - if (response.isError) { - resolver->Reject(context, responseValue).Check(); - } else { - resolver->Resolve(context, responseValue).Check(); - } - - _pendingRequests.erase(it); + v8::Local<v8::Promise::Resolver> resolver; + v8::Local<v8::Value> responseValue; + { + std::lock_guard<std::mutex> lock(_pendingRequestsMutex); + auto it = _pendingRequests.find(response.requestId); + if (it == _pendingRequests.end()) { + continue; + } + resolver = it->second.resolver.Get(isolate); + responseValue = response.response.Get(isolate); + _pendingRequests.erase(it); + } + + if (response.isError) { + resolver->Reject(context, responseValue).Check(); + } else { + resolver->Resolve(context, responseValue).Check(); + } }code/framework/src/scripting/builtins/events.cpp (2)
10-15: Align AllSettledCallbackData fields with private-member naming.These fields are part of a private struct; using the
_prefix keeps internal naming consistent with the repo’s C++ convention.As per coding guidelines: Use `_` prefix for private member variables with camelCase naming in C++.♻️ Proposed refactor
- struct Events::AllSettledCallbackData { - v8::Global<v8::Promise::Resolver> resolver; - std::string errorMessage; - Events *owner; // For removing from pending set on completion - std::atomic<bool> cancelled{false}; // Set when Events is destroyed - }; + struct Events::AllSettledCallbackData { + v8::Global<v8::Promise::Resolver> _resolver; + std::string _errorMessage; + Events *_owner; // For removing from pending set on completion + std::atomic<bool> _cancelled{false}; // Set when Events is destroyed + };- data->cancelled.store(true, std::memory_order_release); - data->resolver.Reset(); + data->_cancelled.store(true, std::memory_order_release); + data->_resolver.Reset();- callbackData->resolver.Reset(isolate, resolver); - callbackData->errorMessage = aggregateErrorMessage; - callbackData->owner = this; + callbackData->_resolver.Reset(isolate, resolver); + callbackData->_errorMessage = aggregateErrorMessage; + callbackData->_owner = this;- if (data->cancelled.load(std::memory_order_acquire)) { + if (data->_cancelled.load(std::memory_order_acquire)) { return; // Events destroyed, bail out safely } - v8::Local<v8::Promise::Resolver> res = data->resolver.Get(iso); - std::string errorMsg = data->errorMessage; + v8::Local<v8::Promise::Resolver> res = data->_resolver.Get(iso); + std::string errorMsg = data->_errorMessage;- data->owner->RemovePendingCallback(data); + data->_owner->RemovePendingCallback(data);
289-337: Reuse argv vector across loop iterations to reduce allocations.The
argvvector is read-only from V8's perspective and remains valid for the entire function scope, allowing it to be constructed once before the loop and passed to eachfunc->Call()without recreation or copying per handler.♻️ Proposed refactor
+ std::vector<v8::Local<v8::Value>> argv(args.begin(), args.end()); + const int argc = static_cast<int>(argv.size()); + v8::Local<v8::Value> *argvData = argv.empty() ? nullptr : argv.data(); + for (size_t i = 0; i < handlers.size(); ++i) { auto &[callback, logContext] = handlers[i]; v8::Local<v8::Function> func = callback.Get(isolate); v8::TryCatch tryCatch(isolate); - std::vector<v8::Local<v8::Value>> argv(args.begin(), args.end()); - v8::MaybeLocal<v8::Value> maybeResult = func->Call(context, context->Global(), - static_cast<int>(argv.size()), - argv.empty() ? nullptr : argv.data()); + v8::MaybeLocal<v8::Value> maybeResult = + func->Call(context, context->Global(), argc, argvData);
| // Emit to JavaScript resources via the Events system | ||
| auto resourceManager = _scriptingModule->GetResourceManager(); | ||
| if (!resourceManager) { | ||
| return; | ||
| } | ||
| catch (const std::exception &ex) { | ||
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->error("Failed to parse event payload: {}", ex.what()); | ||
|
|
||
| auto *engine = _scriptingModule->GetEngine(); | ||
| if (!engine || !engine->IsInitialized()) { | ||
| return; | ||
| } | ||
| const auto resourceManager = Framework::CoreModules::GetResourceManager(); | ||
| if (resourceManager) { | ||
| resourceManager->InvokeGlobalEvent(eventName, payload); | ||
|
|
||
| v8::Isolate *isolate = engine->GetIsolate(); | ||
| v8::Locker locker(isolate); | ||
| v8::Isolate::Scope isolateScope(isolate); | ||
| v8::HandleScope handleScope(isolate); | ||
| v8::Local<v8::Context> context = engine->GetContext(); | ||
| v8::Context::Scope contextScope(context); | ||
|
|
||
| // Parse JSON payload and emit event | ||
| std::vector<v8::Local<v8::Value>> args; | ||
| if (!payloadStr.empty()) { | ||
| v8::Local<v8::String> jsonStr; | ||
| if (!v8::String::NewFromUtf8(isolate, payloadStr.c_str()).ToLocal(&jsonStr)) { | ||
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->error("Failed to create V8 string from event payload: {}", payloadStr); | ||
| return; | ||
| } | ||
|
|
||
| v8::TryCatch tryCatch(isolate); | ||
| v8::Local<v8::Value> parsed; | ||
| if (!v8::JSON::Parse(context, jsonStr).ToLocal(&parsed)) { | ||
| if (tryCatch.HasCaught()) { | ||
| v8::String::Utf8Value errorMsg(isolate, tryCatch.Exception()); | ||
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->error("Failed to parse event payload JSON: {}", *errorMsg ? *errorMsg : "unknown error"); | ||
| } else { | ||
| Logging::GetLogger(FRAMEWORK_INNER_CLIENT)->error("Failed to parse event payload JSON: {}", payloadStr); | ||
| } | ||
| return; | ||
| } | ||
| args.push_back(parsed); | ||
| } | ||
|
|
||
| resourceManager->GetEvents().EmitReserved(isolate, context, eventName, args); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "EmitReserved" code/framework/srcRepository: MafiaHub/Framework
Length of output: 847
🏁 Script executed:
rg -n "RunOn|Dispatch|Queue|Enqueue" code/framework/src -A 2Repository: MafiaHub/Framework
Length of output: 8264
🏁 Script executed:
cd code/framework/src/integrations/client && grep -B 20 "EmitReserved" instance.cpp | head -40Repository: MafiaHub/Framework
Length of output: 1218
🏁 Script executed:
sed -n '540,600p' code/framework/src/scripting/builtins/events.cppRepository: MafiaHub/Framework
Length of output: 2638
🏁 Script executed:
sed -n '350,440p' code/framework/src/integrations/client/instance.cppRepository: MafiaHub/Framework
Length of output: 3888
🏁 Script executed:
grep -B 50 "EmitReserved" code/framework/src/integrations/client/instance.cpp | grep -E "(^[a-zA-Z_].*\(|OnEvent|Callback|Handler)" | tail -5Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
sed -n '480,550p' code/framework/src/scripting/builtins/events.cppRepository: MafiaHub/Framework
Length of output: 3405
🏁 Script executed:
rg -n "class.*Engine|class.*ScriptingModule" code/framework/src/scripting -A 10 | grep -E "(Queue|Dispatch|RunOn|schedule)" -B 5 -A 3Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
grep -r "ScriptingModule.*Get" code/framework/src/integrations/client/instance.cpp | head -3Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
rg -n "RegisterRPC" code/framework/src/integrations/client/instance.cpp -A 3 | head -20Repository: MafiaHub/Framework
Length of output: 306
🏁 Script executed:
grep -r "GetResourceManager\|ScriptingModule" code/framework/src/scripting -A 5 | grep -E "(Queue|Dispatch|RunOn|thread)" -B 3 -A 2 | head -30Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
cat code/framework/src/scripting/scripting_engine.h | grep -E "(class|thread|Queue|Dispatch)" -A 2 | head -40Repository: MafiaHub/Framework
Length of output: 141
🏁 Script executed:
rg -n "v8::Locker" code/framework/src -B 5 -A 10 | head -60Repository: MafiaHub/Framework
Length of output: 5002
🏁 Script executed:
rg -n "Tick\(\)" code/framework/src/scripting -B 2 -A 8 | head -50Repository: MafiaHub/Framework
Length of output: 1685
🏁 Script executed:
grep -r "class NodeEngine\|class.*Scripting" code/framework/src/scripting --include="*.h" | head -5Repository: MafiaHub/Framework
Length of output: 149
🏁 Script executed:
find code/framework/src/scripting -name "*.h" -type f | head -10Repository: MafiaHub/Framework
Length of output: 551
🏁 Script executed:
grep -r "Queue\|schedule" code/framework/src/scripting --include="*.h" --include="*.cpp" | grep -v "std::queue\|emptyQueueBehavior\|renderQueue\|responseQueue" | head -20Repository: MafiaHub/Framework
Length of output: 1355
🏁 Script executed:
rg -n "class ResourceManager" code/framework/src/scripting -A 20Repository: MafiaHub/Framework
Length of output: 14508
🏁 Script executed:
sed -n '1,100p' code/framework/src/scripting/resource/resource_manager.hRepository: MafiaHub/Framework
Length of output: 3107
🏁 Script executed:
grep -n "InvokeHandlersToPromiseArray" code/framework/src/scripting/builtins/events.cpp -A 30 | head -50Repository: MafiaHub/Framework
Length of output: 2860
🏁 Script executed:
rg -n "libuv|uv_queue|event_loop" code/framework/src/scripting --include="*.h" --include="*.cpp"Repository: MafiaHub/Framework
Length of output: 503
🏁 Script executed:
grep -r "RegisterRPC" code/framework/src/integrations -B 5 -A 2 | grep -E "(thread|Thread|async|Async|callback)" -B 2 -A 2 | head -30Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
grep -r "RegisterRPC" code/framework/src/integrations -A 10 | grep -E "(Locker|Queue|Dispatch|Tick|event_loop)" -B 3 -A 2 | head -40Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
grep -n "SetOnPlayerDisconnectedCallback\|RegisterRPC" code/framework/src/integrations/client/instance.cpp | head -10Repository: MafiaHub/Framework
Length of output: 282
🏁 Script executed:
sed -n '360,390p' code/framework/src/integrations/client/instance.cppRepository: MafiaHub/Framework
Length of output: 1175
🏁 Script executed:
grep -r "v8::Locker" code/framework/src -B 10 | grep -E "(RegisterRPC|Callback)" -B 8 | head -40Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
rg -n "Tick\(\)|PerformMicrotask" code/framework/src/integrations -A 3 -B 3Repository: MafiaHub/Framework
Length of output: 1516
🏁 Script executed:
grep -n "GetScriptingModule\|_scriptingModule" code/framework/src/integrations/client/instance.h | head -10Repository: MafiaHub/Framework
Length of output: 271
🏁 Script executed:
grep -r "Queue.*event\|enqueue.*script" code/framework/src -i | head -20Repository: MafiaHub/Framework
Length of output: 44
🏁 Script executed:
sed -n '130,160p' code/framework/src/integrations/client/scripting/module.cppRepository: MafiaHub/Framework
Length of output: 1300
🏁 Script executed:
grep -n "Update\|Tick" code/framework/src/integrations/client/instance.cpp | head -10Repository: MafiaHub/Framework
Length of output: 447
🌐 Web query:
V8 isolate locker thread safety cross-thread access
💡 Result:
-
An
Isolateis not generally thread-safe. V8 guarantees an isolate can be entered/used by at most one thread at a time; if multiple threads may touch the same isolate, you must usev8::Locker/v8::Unlockerto synchronize access. “Using” includes accessing handles/objects created in that isolate. [1], [2] -
v8::Lockergives the current thread permission to use the isolate (a critical section). While aLockeris in scope, that thread may enter the isolate and call V8 APIs; at most one thread can hold the isolate lock at once. [1] -
v8::Unlockeris only for temporarily releasing the isolate lock while doing non‑V8 work. Typical pattern is:isolate->Exit(); v8::Unlocker unlocker(isolate); /* no V8 */; isolate->Enter();. While unlocked, you must not call V8 APIs or touch V8 handles/objects from that thread. Also,Unlockercan’t be used unless you’re already inside aLockerscope. [1], [2] -
Cross-thread access pitfalls:
- You cannot safely use V8 handles/locals/persistent-wrapped objects across threads just by “not running at the same time”; V8 expects correct thread switching to be signaled via
Locker, otherwise crashes are “intended behavior.” [3] - If you want concurrency, common patterns are one isolate per thread (no locking needed if each isolate is single-thread-owned) or a single isolate with a strict global lock +
Lockeraround every V8 entry. [2], [3]
- You cannot safely use V8 handles/locals/persistent-wrapped objects across threads just by “not running at the same time”; V8 expects correct thread switching to be signaled via
-
What is safe cross-thread? Certain “control” operations may be exposed via a thread-safe isolate handle (e.g., terminating execution from another thread), but that’s separate from general JS/V8 object access. [4]
Sources:
[1] V8 v8::Locker / v8::Unlocker header docs (googlesource)
[2] V8 v8.h comments about isolate entry and Locker/Unlocker (chromium codereview / doxygen mirror)
[3] v8-users thread confirming crashes when using an isolate from another thread without Locker is intended
[4] IsolateHandle thread-safe usage (docs.rs / rusty_v8 API docs)
🌐 Web query:
V8 single threaded execution model Locker sufficiency
💡 Result:
V8’s execution model is single-threaded per v8::Isolate: at most one thread may “use” a given isolate at a time (and “use” includes touching handles or holding raw object pointers obtained from handles). (chromium.googlesource.com)
Is v8::Locker sufficient?
v8::Locker is necessary to make cross-thread isolate use correct, but it’s not a magic “thread-safety switch.” It provides a critical section so only one thread can use the isolate at once, and it also signals thread switches to V8 (which V8 requires even if you already serialized access with your own mutex). (chromium.googlesource.com)
When you do not need v8::Locker
If you guarantee the isolate is only ever accessed from one OS thread for its entire lifetime, you generally don’t need v8::Locker. (Many failures happen when code assumes this but actually calls V8 from a different thread later.) (stackoverflow.com)
What v8::Locker does not give you
- It does not make JS execute concurrently inside one isolate; it only serializes entry. (chromium.googlesource.com)
- It does not let you safely “unlock during a callback” and continue V8 work on another thread if there are still V8/JS frames, entered contexts, or live handles that assume the original thread’s ownership; that pattern is a common cause of crashes. (groups.google.com)
Practical rule
- Single thread forever: no locker needed (but enforce it strictly).
- Multiple threads may touch the same isolate (even at different times): you must use
v8::Locker/v8::Unlockercorrectly, and also ensure the thread properly enters the isolate (e.g.,v8::Isolate::Scope, plus the usual handle/context scopes as needed). (chromium.googlesource.com) - Real parallelism: use multiple isolates, one per worker thread (no sharing of V8 objects across isolates). (brendanashworth.github.io)
Citations:
- 1: https://chromium.googlesource.com/v8/v8/%2B/refs/heads/10.3-lkgr/include/v8-locker.h?utm_source=openai
- 2: https://chromium.googlesource.com/v8/v8/%2B/refs/heads/10.3-lkgr/include/v8-locker.h?utm_source=openai
- 3: https://stackoverflow.com/questions/15596083/what-is-the-correct-way-to-use-v8locker-and-why-must-i-use-it?utm_source=openai
- 4: https://chromium.googlesource.com/v8/v8/%2B/refs/heads/10.3-lkgr/include/v8-locker.h?utm_source=openai
- 5: https://groups.google.com/g/v8-users/c/o_uBt3RYZJE?utm_source=openai
- 6: https://chromium.googlesource.com/v8/v8/%2B/refs/heads/10.3-lkgr/include/v8-locker.h?utm_source=openai
- 7: https://brendanashworth.github.io/v8-docs/classv8_1_1_isolate.html?utm_source=openai
🌐 Web query:
Node.js v8 isolate thread safety libuv event loop
💡 Result:
- V8 isolate thread-safety: A
v8::Isolateis not generally safe for concurrent access; in V8’s own headers,Isolate::Enter()/Exit()are explicitly “Not thread-safe” and multiple threads must not enter/exit the same isolate at the same time without external locking (e.g.,v8::Locker). [1] - libuv event loop thread affinity: The libuv event loop (
uv_loop_t) is designed to be tied to a single thread; libuv APIs involving the loop/handles are not thread-safe unless explicitly stated otherwise. Network I/O is done via non-blocking sockets polled by epoll/kqueue/IOCP/etc. [2] - Node.js “instance” mapping: In Node’s embedder model, a Node “instance” (
node::Environment) is associated with exactly onev8::Isolateand exactly oneuv_loop_t(i.e., one JS engine instance + one event loop). [3] - libuv thread pool vs event loop: libuv’s thread pool (default size 4, configurable via
UV_THREADPOOL_SIZE) is used for work that can’t be handled purely by non-blocking OS polling—specifically “all file system operations” andgetaddrinfo/getnameinfo(DNS-related) work; the pool is global/shared and additional work queues when all threads are busy. [4] - Why you can’t “just call JS” from libuv worker threads: libuv’s own threading guide uses Node/V8 as the example of thread affinity: V8 contexts/objects are bound to the thread where V8 runs, so background work (e.g., from
uv_queue_work) must signal back to the loop thread (e.g.,uv_async_send) to safely invoke JS. [5]
Practical implication: In typical Node code, one thread runs the V8 isolate + libuv event loop; background libuv pool threads may execute blocking/native work, but must marshal results back to the loop thread to interact with JS. For parallel JS execution, Node uses worker threads, which create additional independent JS execution threads (and, at the embedder level, additional isolate/loop pairs). [3]
References:
[1] https://chromium.googlesource.com/v8/v8/+/7a33bd5da48f6b7abdeeb013df360f1af84c06b7/src/isolate.h
[2] https://docs.libuv.org/en/latest/design.html
[3] https://r2.nodejs.org/docs/v21.0.0/api/embedding.html
[4] https://man.archlinux.org/man/libuv.1.en
[5] https://docs.libuv.org/en/v1.x/guide/threads.html
🌐 Web query:
Node.js embedding multiple threads isolate locker
💡 Result:
When you embed Node.js/V8 in a multi-threaded host, the key rule is:
- A given
v8::Isolatemay be entered/used by at most one OS thread at a time, and the embedder must enforce this withv8::Locker/v8::Unlocker. V8’s own docs spell this out: “An isolate can be entered by at most one thread … The Locker/Unlocker API must be used to synchronize.” [1]
What this means in practice
-
If multiple threads might touch the same
Isolate, wrap every period of V8/Node interaction on that thread inv8::Locker:v8::Locker locker(isolate);v8::Isolate::Scope isolate_scope(isolate);- then your
HandleScope,Context::Scope, etc.
This is exactly how the Node embedder example structures an embedded run: it enters aLockerscope before interacting with the isolate/context and running the event loop. [2]
-
If you “hand off” an isolate between threads (create on thread A, run later on thread B):
- You still need
v8::Lockeron thread B when you enter/use it, because V8 needs to be told about thread switches. Thev8::Unlockerdocs describe this requirement explicitly. [3]
- You still need
-
Don’t treat
v8::Lockeras a magic “make Node thread-safe” switch.- It only ensures mutual exclusion for that isolate inside V8. You must still ensure you are not sharing V8 handles,
Locals, persistent handles, or embedder state across threads incorrectly (and Node’s own embedder state is generally designed around “oneEnvironment↔ oneIsolate↔ oneuv_loop_t”). Node’s embedding docs define this 1:1 association and show the typical structure. [4]
- It only ensures mutual exclusion for that isolate inside V8. You must still ensure you are not sharing V8 handles,
Preferred designs (to avoid isolate locking complexity)
- One isolate per thread (true parallelism, no
Lockercontention), or - Run all JS/Node work for an isolate on a single dedicated thread, and communicate via thread-safe queues to that thread.
If you share a single isolate across multiple threads, you’re effectively serializing all JS execution anyway (because only one thread can hold the isolate lock at a time). [1]
Sources:
[1] V8 v8::Isolate docs (thread entry + Locker/Unlocker requirement) https://v8.github.io/api/head/classv8_1_1Isolate.html
[2] Node embedding example (uses Locker locker(isolate); around isolate/context + event loop) https://nodejs.cn/api/v20/embedding.html
[3] V8 v8::Unlocker docs (Locker/Unlocker used to signal thread switches) https://v8.github.io/api/head/classv8_1_1Unlocker.html
[4] Node.js embedding docs: Environment is associated with exactly one Isolate and one uv_loop_t https://r2.nodejs.org/docs/v21.0.0/api/embedding.html
Defer EmitReserved to the scripting thread to respect V8/libuv thread affinity.
This RPC handler executes V8 APIs and event handlers directly from the network callback thread using v8::Locker. While v8::Locker correctly serializes V8 isolate access, it does not address libuv's thread affinity requirement: the libuv event loop is tightly bound to the thread that runs NodeEngine::Tick(), and libuv APIs/state are not thread-safe. Executing handlers from a different thread can trigger libuv operations (timers, I/O, promises, etc.), creating a race condition with the event loop running on the main thread.
Enqueue the event emission onto the scripting thread's event loop instead of executing it synchronously in the network callback. Store the event in a thread-safe queue and emit it during the next NodeEngine::Tick() call within the proper event loop thread and V8 context.
🤖 Prompt for AI Agents
In `@code/framework/src/integrations/client/instance.cpp` around lines 388 - 429,
This code calls V8 and libuv-bound handlers directly from the network thread
(using v8::Locker), which can race with the scripting/event loop; instead,
change the flow so the RPC handler does not call
resourceManager->GetEvents().EmitReserved(...) directly but pushes a small event
record (isolate-agnostic data: eventName string and payloadStr) into a
thread-safe queue owned by the scripting module/engine, and then return; update
NodeEngine::Tick() (or the scripting thread tick handler) to drain that queue on
the scripting/libuv thread, reconstruct the V8 values (parse JSON and create V8
handles) and call resourceManager->GetEvents().EmitReserved(isolate, context,
eventName, args) there within the V8 context; reference the EmitReserved call,
the network RPC handler block that currently parses payloadStr and calls
EmitReserved, and NodeEngine::Tick() as the place to move emission and
JSON-to-V8 parsing.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests