src: add AddCodeEventHook API for code events#327
src: add AddCodeEventHook API for code events#327santigimeno wants to merge 2 commits intonode-v22.x-nsolid-v5.xfrom
Conversation
WalkthroughThis change introduces a new infrastructure for registering and handling code event hooks in the NSolid runtime. It adds new source and header files for a code event handler, updates build configuration, extends core classes to support asynchronous code event dispatching, and provides a public API for registering hooks. Comprehensive tests and a Node.js addon are included to validate and expose the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript
participant Addon as Native Addon
participant NSolid as NSolid Runtime
participant EnvList as EnvList
participant EnvInst as EnvInst
participant Handler as NSolidCodeEventHandler
JS->>Addon: registerCodeEventHook(callback)
Addon->>NSolid: AddCodeEventHook(cb, data)
NSolid->>EnvList: AddCodeEventHook(cb, data)
EnvList->>EnvInst: setup_code_event_handler()
EnvInst->>Handler: (creates NSolidCodeEventHandler)
Handler-->>Handler: (V8 triggers code event)
Handler->>EnvList: on_code_event_q_.enqueue(CodeEventInfo)
EnvList->>Addon: got_code_event(CodeEventInfo)
Addon->>JS: callback(functionName, threadId)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the linked issue. Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new API for registering and handling code event hooks in nsolid, along with several infrastructure updates to support asynchronous code event callbacks. Key changes include:
- New TSList APIs to iterate with size parameters and return updated list sizes on erasure.
- Integration of code event hook registration/handling in both C++ (nsolid APIs, code event handler, and event dispatching) and JavaScript (addon tests).
- Updates to binding, build files, and API header modifications to support code event hooks.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/cctest/test_nsolid_thread_safe.cc | Added tests for new TSList for_each and erase APIs. |
| test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js | Added tests for the new code event hook API in a multi-threaded context. |
| test/addons/nsolid-code-event-hook/binding.gyp | Added binding definition for the new addon. |
| test/addons/nsolid-code-event-hook/binding.cc | Implemented code event callback and hook registration logic in C++. |
| src/nsolid/thread_safe.h | Added for_each overload and modified erase to return updated list size. |
| src/nsolid/nsolid_code_event_handler.h & .cc | Introduced the NSolidCodeEventHandler for handling code events. |
| src/nsolid/nsolid_api.h & .cc | Updated API to include code event handler setup and hook management. |
| src/nsolid/nsolid.cc | Added CodeEventHook implementation and adjustments to hook lifecycle. |
| node.gyp | Updated build configuration to include the new code event handler module. |
Comments suppressed due to low confidence (2)
src/nsolid/nsolid.cc:625
- Calling 'delete this;' in CodeEventHook::Dispose can be error-prone if not handled carefully. Consider clarifying the ownership semantics in documentation or refactoring to use a smart pointer based approach to avoid potential misuse.
delete this;
src/nsolid/nsolid_api.cc:1984
- The conditional use of 'std::move(info)' when size equals 1 versus passing by value when multiple hooks are present could lead to inconsistent data ownership. Consider using a consistent strategy when dispatching CodeEventInfo to hooks to avoid potential issues with moved-from objects.
code_event_hook_list_.for_each([&info, &envinst_sp](auto& stor, size_t size) {
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
| ~Impl() { | ||
| // Remove the hook from the TSList in EnvList | ||
| EnvList::Inst()->RemoveCodeEventHook(hook_); | ||
| } |
There was a problem hiding this comment.
The destructor of CodeEventHook::Impl unconditionally calls EnvList::Inst()->RemoveCodeEventHook(hook_), but there's no check if hook_ is a valid iterator. If Setup() fails or is never called, this could lead to undefined behavior when removing an invalid iterator.
There was a problem hiding this comment.
This cannot ever happen afaict
| CodeEventHook* add_code_event_hook_(void* data, | ||
| code_event_hook_proxy_sig proxy, | ||
| deleter_sig deleter) { | ||
| CodeEventHook* hook = new (std::nothrow) CodeEventHook(); | ||
| if (hook == nullptr) { | ||
| return nullptr; | ||
| } | ||
| hook->DoSetup(proxy, deleter, data); | ||
| return hook; | ||
| } |
There was a problem hiding this comment.
The add_code_event_hook_ function allocates a CodeEventHook with new but doesn't check if DoSetup() might fail. If DoSetup() fails, the function would still return a partially initialized hook object that could cause issues when used.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
1-1: Remove redundant 'use strict' directive.The static analysis tool correctly identifies that the 'use strict' directive is redundant since JavaScript modules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
src/nsolid.cc (2)
625-627: Document the ownership model forDispose().The
Dispose()method deletes the object itself (delete this), which requires careful documentation about ownership transfer and usage patterns to prevent use-after-free errors.Consider adding a comment to clarify the ownership model:
void CodeEventHook::Dispose() { + // This method transfers ownership to the callee and deletes the object. + // The caller must not use this object after calling Dispose(). delete this; }
674-683: Consider using exceptions or error codes for allocation failures.The function returns
nullptron allocation failure, but the caller might not check for this condition.Consider either:
- Using exceptions (if allowed in the codebase)
- Adding an error code parameter
- At minimum, documenting the nullptr return value in the header file
Current implementation is acceptable if callers are expected to check for nullptr.
src/nsolid/nsolid_api.cc (3)
891-898: Async queue lambda capturesinfoby forwarding-reference – use const ref
CodeEventInfocan potentially be large. The lambda already moves it when
delivering, so capturing by forwarding ref (&&) brings no extra value and
may lead to a double move/invalid use if extended.- +[](CodeEventInfo&& info, EnvList* envlist) { - envlist->got_code_event(std::move(info)); + +[](CodeEventInfo info, EnvList* envlist) { + envlist->got_code_event(std::move(info)); },A plain pass-by-value here is cheaper and clearer.
1033-1056: Minor: potential redundant map copy during last-hook removalWhen the last hook is removed we copy
env_map_even if it is already empty.
A quick guard can avoid unnecessary work in the common “no envs” case:if (size == 0) { decltype(env_map_) env_map; { ns_mutex::scoped_lock lock(map_lock_); if (env_map_.empty()) return; env_map = env_map_; } ... }
1498-1508:DCHECK_EQmay fire if invoked via EventLoop command
setup_code_event_handler()is scheduled withCommandType::InterruptOnly
which is executed inside V8’s interrupt callback. At that point
Isolate::TryGetCurrent()can legitimately return the same isolate but on a
different native thread (depending on how interrupts are delivered). Consider
removing or downgrading theDCHECK_EQto avoid false positives in RELEASE +
NODE_DEBUGbuilds.src/nsolid.h (2)
660-672: Consider making the struct fields const where appropriate.Since
CodeEventInfois likely used to pass immutable event data to callbacks, consider making appropriate fieldsconstto prevent accidental modification:struct CodeEventInfo { - uint64_t thread_id; - uint64_t timestamp; - v8::CodeEventType type; - uintptr_t code_start; - uintptr_t new_code_start; - size_t code_len; - std::string fn_name; - std::string script_name; - int script_line; - int script_column; - std::string comment; + const uint64_t thread_id; + const uint64_t timestamp; + const v8::CodeEventType type; + const uintptr_t code_start; + const uintptr_t new_code_start; + const size_t code_len; + const std::string fn_name; + const std::string script_name; + const int script_line; + const int script_column; + const std::string comment; };
679-686: Add documentation for theDispose()method lifecycle.While the brief describes that
Dispose()removes the hook and releases resources, it would be helpful to document whether:
- The method is idempotent (can be called multiple times safely)
- The method blocks until any in-progress callbacks complete
- The object becomes invalid immediately after calling
Dispose()src/nsolid/nsolid_api.h (2)
248-250: Consider adding documentation for the new methods.These methods are part of the public interface of
EnvInst. Consider adding documentation to explain:
- When these methods should be called
- Thread safety guarantees
- Any preconditions or postconditions
653-653: Consider documenting the threading model for code event dispatch.The method
got_code_eventtakes an rvalue reference, suggesting it's designed for efficient event forwarding. Document:
- Which thread(s) call this method
- How it interacts with the async queue
- Performance considerations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
node.gyp(1 hunks)src/nsolid.cc(2 hunks)src/nsolid.h(7 hunks)src/nsolid/nsolid_api.cc(8 hunks)src/nsolid/nsolid_api.h(11 hunks)src/nsolid/nsolid_code_event_handler.cc(1 hunks)src/nsolid/nsolid_code_event_handler.h(1 hunks)src/nsolid/thread_safe.h(2 hunks)test/addons/nsolid-code-event-hook/binding.cc(1 hunks)test/addons/nsolid-code-event-hook/binding.gyp(1 hunks)test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js(1 hunks)test/cctest/test_nsolid_thread_safe.cc(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
test/common/index.js (1)
buildType(155-157)
src/nsolid/nsolid_code_event_handler.cc (3)
src/nsolid/nsolid_code_event_handler.h (3)
NSolidCodeEventHandler(14-14)NSolidCodeEventHandler(15-15)code_event(17-17)src/util.h (1)
Utf8Value(548-548)src/nsolid/nsolid_api.cc (2)
Inst(914-917)Inst(914-914)
test/addons/nsolid-code-event-hook/binding.cc (3)
src/nsolid.cc (10)
CodeEventHook(620-621)CodeEventHook(623-623)cb(610-615)cb(610-612)GetThreadId(56-64)GetThreadId(56-56)GetLocalEnvInst(43-45)GetLocalEnvInst(43-43)GetLocalEnvInst(47-49)GetLocalEnvInst(47-47)src/nsolid.h (14)
CodeEventHook(679-702)CodeEventHook(690-690)CodeEventHook(693-693)RunCommand(523-526)RunCommand(1602-1626)RunCommand(1602-1605)cb(658-658)thread_id(852-852)thread_id(852-852)GetThreadId(404-404)GetLocalEnvInst(385-385)GetLocalEnvInst(391-391)AddCodeEventHook(1842-1860)AddCodeEventHook(1842-1842)src/nsolid/nsolid_api.cc (8)
RunCommand(157-202)RunCommand(157-160)GetCurrent(433-436)GetCurrent(433-433)GetCurrent(439-442)GetCurrent(439-439)AddCodeEventHook(1007-1031)AddCodeEventHook(1007-1010)
src/nsolid/nsolid_api.cc (3)
src/nsolid/nsolid_api.h (23)
data(80-80)data(81-81)data(515-518)data(520-523)data(525-528)data(529-532)data(539-542)data(544-546)envinst(340-341)envinst(342-343)envinst(344-345)envinst(346-347)it(547-547)it(620-620)envinst_sp(195-198)envinst_sp(334-335)envinst_sp(662-662)envinst_sp(670-670)envinst_sp(671-671)envinst_sp(672-672)envinst_sp(674-674)envinst_sp(678-678)envinst_sp(679-679)src/nsolid.h (5)
AddCodeEventHook(1842-1860)AddCodeEventHook(1842-1842)RunCommand(523-526)RunCommand(1602-1626)RunCommand(1602-1605)src/nsolid.cc (2)
deleter(463-463)deleter(611-611)
🪛 Biome (1.9.4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-macOS
- GitHub Check: coverage-windows
- GitHub Check: build-docs
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-linux
- GitHub Check: lint-js-and-md
- GitHub Check: lint-cpp
- GitHub Check: build-tarball
🔇 Additional comments (25)
src/nsolid/thread_safe.h (4)
149-155: LGTM! Well-designed enhancement for thread-safe iteration with size awareness.The new
for_eachoverload that provides the current list size to the callback is a thoughtful addition. Capturing the size once before iteration ensures consistency throughout the callback execution, which is crucial for the code event hook infrastructure.
156-160: Excellent improvement to return the new size after element removal.Returning the updated list size from the
erasemethod provides valuable feedback for managing hook lifecycles and is more informative than the previousvoidreturn type.
189-195: Consistent implementation in the pointer specialization.The changes are properly mirrored in the pointer template specialization, maintaining API consistency across both variants of TSList.
196-200: Good consistency in the pointer specialization's erase method.The pointer specialization correctly returns the new size after erasure, maintaining consistency with the main template implementation.
test/addons/nsolid-code-event-hook/binding.gyp (1)
1-9: Clean and correct build configuration for the test addon.The binding.gyp file follows standard node-gyp conventions with appropriate debug symbols enabled for testing. The configuration correctly references the binding.cc source file and establishes the "binding" target.
node.gyp (1)
517-517: Proper integration of the new code event handler into the build system.The source file is correctly added to the
nsolid_sourceslist, following the established pattern for NSolid components and maintaining consistency with the directory structure.src/nsolid/nsolid_code_event_handler.h (1)
1-28: Excellent design for the NSolid code event handler.This header file demonstrates several best practices:
- Proper header guards and conditional compilation for internal use only
- Clean inheritance from
v8::CodeEventHandlerwith appropriate method override- Virtual destructor following best practices for polymorphic base classes
- Well-designed constructor taking necessary context (isolate and thread ID)
- Clear separation between public interface and private implementation details
The design properly encapsulates the functionality needed for capturing and processing V8 code events within the NSolid framework.
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
3-91: Comprehensive and well-structured test for code event hooks.This test provides excellent coverage of the code event hook functionality:
- Thorough testing scope: Tests registration, callback invocation, selective unregistration, and complete cleanup
- Thread-aware testing: Uses worker threads to verify thread-specific hook behavior and thread ID correctness
- State management: Proper state tracking and reset mechanisms for reliable test execution
- Asynchronous handling: Appropriate use of setTimeout for asynchronous event testing
- Robust assertions: Verifies both positive cases (hooks firing) and negative cases (hooks not firing after unregistration)
The test design effectively validates the entire lifecycle of code event hooks and ensures proper isolation between different hook instances.
src/nsolid/nsolid_code_event_handler.cc (2)
1-39: LGTM! Clean implementation of the code event handler.The implementation correctly:
- Inherits from
v8::CodeEventHandlerand overrides theHandlemethod- Extracts all relevant information from the
CodeEventobject- Creates a well-structured
CodeEventInfowith proper move semantics- Enqueues the event for asynchronous processing
34-34:Details
✅ Verification successful
Verify that
GetComment()never returns null.The code directly passes the result of
code_event->GetComment()tostd::stringconstructor. IfGetComment()can return a null pointer, this would cause undefined behavior.
🏁 Script executed:
#!/bin/bash # Check the V8 header files or documentation for GetComment() behavior rg -A 5 "GetComment" --glob "*.h" --glob "*.cc" deps/v8Length of output: 5140
Confirmed
GetComment()always returns a non-null C-stringV8’s own code and tests assume it never yields a null pointer:
In deps/v8/src/api/api.cc:
const char* CodeEvent::GetComment() { return reinterpret_cast<i::CodeEvent*>(this)->comment; }The internal
commentfield is always initialized to point at a valid string (empty or otherwise).In deps/v8/src/codegen/code-comments.cc:
const char* CodeCommentsIterator::GetComment() const { const char* comment_string = …; CHECK_EQ(GetCommentSize(), strlen(comment_string) + 1); return comment_string; }The
CHECK_EQguaranteescomment_stringis non-null and null-terminated.V8 unit tests (e.g. logging/log-unittest.cc) call
std::string(code_event->GetComment())without guarding against null, and would crash ifGetComment()could ever return nullptr.No defensive null check is needed before constructing
std::stringfromGetComment().test/cctest/test_nsolid_thread_safe.cc (2)
53-72: Well-structured test for the enhancedfor_eachmethod.The test properly verifies that:
- The callback receives both the value and the current list size
- The size remains consistent (3) during iteration
- Values are visited in the expected order
74-83: Good test coverage for theerasemethod returning size.The test correctly verifies that
erasereturns the updated list size after each removal.src/nsolid.cc (1)
600-618: Clean implementation of the CodeEventHook internal management.The
Implclass properly encapsulates the hook lifecycle:
- Adds the hook to
EnvListon setup- Removes it on destruction
- Follows RAII principles
src/nsolid/nsolid_api.cc (2)
1120-1123: Dependent on previous fix – keep eye on enable logicThis check is correct only when the race in
AddCodeEventHook()is fixed.
Otherwisesize()could still be zero here and the handler would never be
enabled for this environment.
844-852: 🛠️ Refactor suggestion
EnvInst::setup_code_event_handler()leaks the old handler on re-enableIf a handler was created previously and
Disable()was called,code_event_handler_still holds the (disabled) instance.
Callingsetup_code_event_handler()again will therefore not create a fresh handler nor re-enable the old one.void EnvInst::setup_code_event_handler() { - if (!code_event_handler_) { - code_event_handler_ = - std::make_unique<NSolidCodeEventHandler>(isolate_, thread_id_); - } - - code_event_handler_->Enable(); + if (!code_event_handler_) + code_event_handler_ = + std::make_unique<NSolidCodeEventHandler>(isolate_, thread_id_); + + code_event_handler_->Enable(); // idempotent – safe to call twice }Consider resetting the unique-ptr in
disable_code_event_handler()(see next comment) so that a brand-new handler is constructed next time, avoiding surprises caused by a lingering disabled instance.Likely an incorrect or invalid review comment.
src/nsolid.h (5)
290-290: LGTM!The function pointer signature is correctly defined to handle code events with the appropriate parameters.
320-320: LGTM!The proxy function declaration follows the established pattern for other hook proxies in the codebase.
356-358: LGTM!The internal function declaration correctly returns a
CodeEventHook*and takes the necessary parameters for hook registration.
691-692: LGTM!The friend declaration is necessary to allow the internal function to construct
CodeEventHookobjects while keeping the constructor private.
1930-1935: LGTM!The proxy implementation correctly moves the
CodeEventInfoto avoid unnecessary copying of the potentially large struct.src/nsolid/nsolid_api.h (5)
21-21: LGTM!The include is necessary for the new async queue functionality used for code event dispatching.
427-427: LGTM!The unique pointer correctly manages the lifetime of the code event handler.
504-507: LGTM!The struct follows the established pattern for hook storage with proper callback and user data management.
757-758: LGTM!The async queue and hook list are appropriately placed for managing code event distribution to registered hooks.
544-547:Details
✅ Verification successful
Verify thread safety of the hook list operations.
The methods operate on
TSListwhich should be thread-safe based on the naming convention. However, ensure that the iterator returned byAddCodeEventHookremains valid across concurrent operations.
🏁 Script executed:
#!/bin/bash # Check TSList implementation for thread safety guarantees rg -A 10 "class TSList" src/nsolid/thread_safe.hLength of output: 50
We need to locate the
TSListdefinition to verify its thread-safety guarantees. Let’s search the entire repository:
🏁 Script executed:
#!/bin/bash # Search for TSList class definition and usages across the repo rg -n "class TSList" -n . rg -n "TSList<" -n .Length of output: 2633
Continuing verification: locate the full
TSListimplementation to confirm iterator safety.Adjust search if needed based on the results.
🏁 Script executed:
#!/bin/bash # Show TSList implementation around its declaration rg -n -A20 -B5 "struct TSList<DataType\*>" src/nsolid/thread_safe.hLength of output: 918
Continuing verification: extract the full
TSList<DataType*>specialization (including anyerase/removemethods) to confirm that all list operations are mutex-protected.
🏁 Script executed:
#!/bin/bash # Print lines 170–350 of thread_safe.h to capture the entire TSList<DataType*> block sed -n '170,350p' src/nsolid/thread_safe.hLength of output: 1235
Thread safety and iterator validity confirmed
The
TSList<DataType*>specialization insrc/nsolid/thread_safe.huses a singlensuv::ns_mutexto guard allpush_back,erase,size, andfor_eachoperations.
push_backacquires the lock, appends the element, and returns astd::listiterator. According to C++ guarantees forstd::list, this iterator remains valid across subsequent insertions.- Only calling
erase(iterator)(i.e. viaRemoveCodeEventHook) will invalidate that specific iterator—this matches the intended removal semantics.No further changes are required.
| envlist->on_code_event_q_.reset(); | ||
| if (envlist->continuous_profiler_) { | ||
| envlist->continuous_profiler_.reset(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Queue reset before drain – possible event drop
on_code_event_q_.reset() is called unconditionally during shutdown. If
there are still pending code events they will be destroyed silently, skipping
user hooks and making debugging harder.
Consider:
- Draining the queue synchronously (
dequeueuntil empty) before reset. - Or calling
reset()after confirmingthread_loop_has stopped
processing callbacks.
🤖 Prompt for AI Agents
In src/nsolid/nsolid_api.cc around lines 1712 to 1716, the on_code_event_q_
queue is reset unconditionally during shutdown, which can cause pending code
events to be dropped silently. To fix this, modify the shutdown sequence to
first drain the queue synchronously by dequeuing all events until it is empty
before calling reset(), or ensure that thread_loop_ has fully stopped processing
callbacks before resetting the queue. This prevents loss of events and ensures
user hooks are executed properly.
There was a problem hiding this comment.
I don't think this is relevant once EnvList is gone
There was a problem hiding this comment.
@santigimeno, understood! If EnvList is planned to be removed in future refactoring, then improving the queue draining behavior in the current implementation wouldn't provide much value. Thanks for the clarification on the architectural direction.
santigimeno
force-pushed
the
santi/jit_code_event_handler
branch
2 times, most recently
from
ba37b75 to
16f39c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/addons/nsolid-code-event-hook/binding.cc (1)
53-58:⚠️ Potential issueAdd error handling for V8 operations.
The code uses
ToLocalChecked()without error handling, which will crash the process if the operations fail.- Local<Value> argv[2] = { - String::NewFromUtf8(isolate, info.fn_name.c_str()).ToLocalChecked(), - Number::New(isolate, info.thread_id), - }; - // Call the callback in the current context - cb->Call(isolate->GetCurrentContext(), - Undefined(isolate), 2, argv).ToLocalChecked(); + v8::TryCatch try_catch(isolate); + Local<String> fn_name; + if (!String::NewFromUtf8(isolate, info.fn_name.c_str()).ToLocal(&fn_name)) { + // Handle string creation failure + return; + } + Local<Value> argv[2] = { + fn_name, + Number::New(isolate, info.thread_id), + }; + // Call the callback in the current context + Local<Value> ret; + if (!cb->Call(isolate->GetCurrentContext(), + Undefined(isolate), 2, argv).ToLocal(&ret)) { + // Handle exception if needed + if (try_catch.HasCaught()) { + try_catch.ReThrow(); + } + }
🧹 Nitpick comments (3)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (3)
1-1: Remove redundant 'use strict' directive.JavaScript modules are automatically in strict mode.
-'use strict'; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
9-10: Improve thread check readability.The condition is checking if the process is a worker thread but not launched with the correct PID argument.
-if (!isMainThread && +process.argv[2] !== process.pid) +// Skip if this is a worker thread that wasn't launched by the main test +if (!isMainThread && +process.argv[2] !== process.pid)
46-73: Consider making the test more deterministic.The test uses multiple fixed 500ms timeouts which could lead to flaky tests on heavily loaded systems. Consider using a more deterministic approach.
Would you like me to suggest an implementation using promises or event-based synchronization instead of fixed timeouts to make the test more reliable?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
node.gyp(1 hunks)src/nsolid.cc(2 hunks)src/nsolid.h(7 hunks)src/nsolid/nsolid_api.cc(8 hunks)src/nsolid/nsolid_api.h(11 hunks)src/nsolid/nsolid_code_event_handler.cc(1 hunks)src/nsolid/nsolid_code_event_handler.h(1 hunks)src/nsolid/thread_safe.h(2 hunks)test/addons/nsolid-code-event-hook/binding.cc(1 hunks)test/addons/nsolid-code-event-hook/binding.gyp(1 hunks)test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js(1 hunks)test/cctest/test_nsolid_thread_safe.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- test/addons/nsolid-code-event-hook/binding.gyp
- node.gyp
- src/nsolid/nsolid_code_event_handler.cc
- src/nsolid/nsolid_code_event_handler.h
- src/nsolid/thread_safe.h
- test/cctest/test_nsolid_thread_safe.cc
- src/nsolid.cc
- src/nsolid/nsolid_api.cc
- src/nsolid.h
- src/nsolid/nsolid_api.h
🧰 Additional context used
🧠 Learnings (1)
test/addons/nsolid-code-event-hook/binding.cc (1)
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
🧬 Code Graph Analysis (2)
test/addons/nsolid-code-event-hook/binding.cc (4)
src/nsolid.cc (12)
CodeEventHook(620-621)CodeEventHook(623-623)cb(610-615)cb(610-612)GetThreadId(56-64)GetThreadId(56-56)GetLocalEnvInst(43-45)GetLocalEnvInst(43-43)GetLocalEnvInst(47-49)GetLocalEnvInst(47-47)IsMainThread(91-96)IsMainThread(91-91)src/nsolid.h (15)
CodeEventHook(679-702)CodeEventHook(690-690)CodeEventHook(693-693)RunCommand(523-526)RunCommand(1602-1626)RunCommand(1602-1605)cb(658-658)thread_id(852-852)thread_id(852-852)GetThreadId(404-404)GetLocalEnvInst(385-385)GetLocalEnvInst(391-391)AddCodeEventHook(1842-1860)AddCodeEventHook(1842-1842)IsMainThread(426-426)src/nsolid/nsolid_api.cc (8)
RunCommand(157-202)RunCommand(157-160)GetCurrent(433-436)GetCurrent(433-433)GetCurrent(439-442)GetCurrent(439-439)AddCodeEventHook(1002-1028)AddCodeEventHook(1002-1005)test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
assert(4-4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
test/common/index.js (1)
buildType(155-157)
🪛 Biome (1.9.4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-tarball-linux
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
🔇 Additional comments (1)
test/addons/nsolid-code-event-hook/binding.cc (1)
38-61:⚠️ Potential issueAdd thread safety for js_callback_per_thread access.
The
CodeEventCallbackfunction accessesholder->js_callback_per_threadwithout holding the mutex, whileRegisterJSCodeEventCallbackmodifies it under the mutex lock. This creates a potential data race.Since the map access happens in a lambda that runs on the event loop, you need to ensure thread-safe access:
void CodeEventCallback(std::shared_ptr<node::nsolid::EnvInst> envinst, const node::nsolid::CodeEventInfo& info, HookHolder* holder) { assert(0 == node::nsolid::RunCommand( envinst, node::nsolid::CommandType::EventLoop, [holder](node::nsolid::SharedEnvInst envinst, const node::nsolid::CodeEventInfo& info) { + nsuv::ns_mutex::scoped_lock lock(hooks_mutex_); // Call JS callback with function name if (!holder->js_callback_per_thread[info.thread_id].IsEmpty()) { Isolate* isolate = Isolate::GetCurrent(); v8::HandleScope scope(isolate); Local<Function> cb = holder->js_callback_per_thread[info.thread_id].Get(isolate); + lock.unlock(); // Release lock before calling JS Local<Value> argv[2] = { String::NewFromUtf8(isolate, info.fn_name.c_str()).ToLocalChecked(), Number::New(isolate, info.thread_id), }; // Call the callback in the current context cb->Call(isolate->GetCurrentContext(), Undefined(isolate), 2, argv).ToLocalChecked(); } }, info)); }Likely an incorrect or invalid review comment.
santigimeno
force-pushed
the
santi/jit_code_event_handler
branch
from
16f39c0 to
c9ae3c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/nsolid/nsolid_api.cc (1)
1709-1709: Potential event loss during shutdownThe queue reset happens unconditionally during shutdown, which could silently drop pending code events as mentioned in previous reviews. While this may be acceptable for shutdown scenarios, consider if there are critical events that should be processed before reset.
🧹 Nitpick comments (4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (2)
1-1: Remove redundant 'use strict' directive.The static analysis tool correctly identifies that the 'use strict' directive is redundant in ES modules, as they are automatically in strict mode.
-'use strict';
9-10: Clarify the skip condition logic.The skip condition logic is confusing and may not work as intended. The condition
+process.argv[2] !== process.pidcompares the passed main thread PID with the current process PID, but worker threads share the same process ID as the main thread in Node.js.Consider simplifying this logic or adding a comment to explain the intended behavior:
-if (!isMainThread && +process.argv[2] !== process.pid) - skip('Test must first run as the main thread'); +if (!isMainThread && process.argv.length < 5) + skip('Test must first run as the main thread');Or add a comment explaining the current logic:
+// Skip if running in worker thread but without expected main thread PID if (!isMainThread && +process.argv[2] !== process.pid) skip('Test must first run as the main thread');src/nsolid/nsolid_api.cc (2)
1002-1028: Verify the race condition fix implementationThe reordering of operations (add hook first, then enable handlers) should address the race condition mentioned in previous reviews. However, there's no error handling when
RunCommandfails for individual environments.Consider adding error handling:
for (auto& entry : env_map) { SharedEnvInst envinst = entry.second; int er = RunCommand(envinst, CommandType::InterruptOnly, setup_code_event_handler); if (er) { - // Nothing to do here, really. + // Log warning about failed handler setup for debugging } }
1979-1992: Review move semantics optimizationThe optimization to move the event when there's only one hook (line 1987) vs. copying for multiple hooks (line 1989) is correct. However, ensure that hooks are prepared to handle both moved and copied
CodeEventInfoobjects consistently.The null check for
envinst_sp(line 1981-1983) properly handles late-arriving events from destroyed environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
node.gyp(1 hunks)src/nsolid.cc(2 hunks)src/nsolid.h(7 hunks)src/nsolid/nsolid_api.cc(8 hunks)src/nsolid/nsolid_api.h(11 hunks)src/nsolid/nsolid_code_event_handler.cc(1 hunks)src/nsolid/nsolid_code_event_handler.h(1 hunks)src/nsolid/thread_safe.h(2 hunks)test/addons/nsolid-code-event-hook/binding.cc(1 hunks)test/addons/nsolid-code-event-hook/binding.gyp(1 hunks)test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js(1 hunks)test/cctest/test_nsolid_thread_safe.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- test/addons/nsolid-code-event-hook/binding.gyp
- node.gyp
- src/nsolid/nsolid_code_event_handler.h
- src/nsolid/thread_safe.h
- test/cctest/test_nsolid_thread_safe.cc
- src/nsolid/nsolid_code_event_handler.cc
- src/nsolid/nsolid_api.h
- test/addons/nsolid-code-event-hook/binding.cc
- src/nsolid.cc
- src/nsolid.h
🧰 Additional context used
🧠 Learnings (2)
src/nsolid/nsolid_api.cc (3)
Learnt from: santigimeno
PR: nodesource/nsolid#319
File: src/nsolid/async_ts_queue.h:123-135
Timestamp: 2025-06-03T11:35:52.830Z
Learning: The unbounded timer loop issue in AsyncTSQueue has been resolved by implementing proper timer arming logic with the timer_armed_ atomic flag and adding a queue emptiness check (!queue->queue_.empty()) in the async_callback method before restarting the timer. This prevents the timer from firing continuously when the queue is empty.
Learnt from: santigimeno
PR: nodesource/nsolid#319
File: src/nsolid/async_ts_queue.h:123-135
Timestamp: 2025-06-03T11:35:52.830Z
Learning: The unbounded timer loop issue in AsyncTSQueue has been resolved by implementing proper timer arming logic with the timer_armed_ atomic flag and adding a queue emptiness check (!queue->queue_.empty()) in the async_callback method before restarting the timer. This prevents the timer from firing continuously when the queue is empty.
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
🧬 Code Graph Analysis (1)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
test/common/index.js (1)
buildType(155-157)
🪛 Biome (1.9.4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: coverage-linux
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
🔇 Additional comments (7)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
27-74: Excellent test coverage of hook lifecycle.The test comprehensively validates the hook lifecycle including registration, event triggering, selective unregistration, and cleanup. The logic correctly verifies that hooks receive expected parameters and that unregistered hooks are properly cleaned up.
src/nsolid/nsolid_api.cc (6)
2-2: LGTM - Clean include additionThe include for the code event handler header is properly added.
886-892: LGTM - Proper async queue initializationThe code event queue is correctly initialized with a lambda callback to
got_code_event(). The queue setup follows the established pattern used elsewhere in the codebase.
1030-1053: LGTM - Consistent hook removal patternThe hook removal logic correctly checks if the list becomes empty and disables handlers on all environments. The pattern matches the addition logic and properly handles the last-hook-removed case.
1117-1119: LGTM - Proper hook integration in environment creationThe code correctly sets up the code event handler when adding a new environment if hooks are already registered. This ensures new environments don't miss code events.
1495-1505: LGTM - Clean static wrapper methodsThe static helper methods properly ensure the handler setup/disable operations run on the correct isolate thread with appropriate assertions.
844-854: Confirm proper teardown of the code event handlerIn EnvInst::disable_code_event_handler() (src/nsolid/nsolid_api.cc:844–854) you currently only do:
code_event_handler_.reset();without first unregistering the V8 handler. NSolidCodeEventHandler does not override a Disable() method and its destructor is empty, so it’s unclear whether destroying the pointer automatically calls into the base v8::CodeEventHandler to disable itself.
Please verify one of the following:
- That destroying the NSolidCodeEventHandler (via its destructor and base‐class cleanup) unregisters the V8 code event handler, or
- If not, explicitly call
before resetting the unique_ptr to avoid leaving V8 in an inconsistent state.code_event_handler_->Disable();
| hookIndex1 = +process.argv[3]; | ||
| hookIndex2 = +process.argv[4]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for command line arguments.
The parsing of command line arguments lacks validation, which could lead to unexpected behavior if arguments are missing or invalid.
} else {
+ assert(process.argv.length >= 5, 'Missing required arguments');
hookIndex1 = +process.argv[3];
hookIndex2 = +process.argv[4];
+ assert(Number.isInteger(hookIndex1) && hookIndex1 >= 0, 'Invalid hookIndex1');
+ assert(Number.isInteger(hookIndex2) && hookIndex2 >= 0, 'Invalid hookIndex2');
}🤖 Prompt for AI Agents
In test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js around lines 87
to 88, the code directly converts process.argv[3] and process.argv[4] to numbers
without validating their presence or correctness. Add input validation to check
if these arguments exist and are valid numbers before using them. If invalid or
missing, handle the error gracefully, such as by logging an error message and
exiting the process or providing default values.
c9ae3c8 to
41949db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (2)
82-85: Add error handling for worker thread.The worker thread creation lacks error handling, which could lead to silent failures in the test.
const worker = new Worker(__filename, { argv: [process.pid, hookIndex1, hookIndex2] }); + worker.on('error', (err) => { + throw err; + }); worker.on('exit', (code) => { assert.strictEqual(code, 0); });
47-73: Replace hardcoded timeouts with more reliable synchronization.The test relies on hardcoded 500ms timeouts which could lead to flaky test behavior in CI environments or under load. Consider using more deterministic synchronization mechanisms like
setImmediate()orprocess.nextTick().- // Wait a tick to allow the hook to fire - setTimeout(() => { + // Use setImmediate for more reliable timing + setImmediate(() => { assert.ok(state1.called, 'Code event hook was not called'); assert.ok(state1.customFnNameReceived, 'Custom function name was not received'); assert.ok(state2.called, 'Code event hook was not called'); assert.ok(state2.customFnNameReceived, 'Custom function name was not received'); // Unregister the hooks binding.unregisterCodeEventHook(hookIndex1); resetState(state1); resetState(state2); // The hook should not be called after unregistering binding.triggerCodeEvent(); - setTimeout(() => { + setImmediate(() => { assert.ok(!state1.called, 'Code event hook was called after unregistering'); assert.ok(state2.called, 'Code event hook was not called after unregistering'); binding.unregisterCodeEventHook(hookIndex2); resetState(state1); resetState(state2); - setTimeout(() => { + setImmediate(() => { assert.ok(!state1.called, 'Code event hook was called after unregistering'); assert.ok(!state2.called, 'Code event hook was called after unregistering'); binding.unregisterAllHooks(); - }, 500); - }, 500); - }, 500); + }); + }); + });
🧹 Nitpick comments (3)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (1)
1-1: Remove redundant 'use strict' directive.The 'use strict' directive is redundant in JavaScript modules, as they are automatically in strict mode.
-'use strict';src/nsolid/nsolid_api.cc (2)
1002-1028: Inefficient handler enablement on existing environmentsThe current implementation enables code event handlers on all existing environments every time a hook is added, even if handlers are already enabled. This could be inefficient with multiple hooks.
Consider tracking whether this is the first hook (transitioning from 0→1) and only enabling handlers in that case:
TSList<EnvList::CodeEventHookStor>::iterator EnvList::AddCodeEventHook( void* data, internal::code_event_hook_proxy_sig proxy, internal::deleter_sig deleter) { + bool first_hook = (code_event_hook_list_.size() == 0); auto it = code_event_hook_list_.push_back( { proxy, nsolid::internal::user_data(data, deleter) }); + if (first_hook) { decltype(env_map_) env_map; { // Copy the envinst map so we don't need to keep it locked the entire time. ns_mutex::scoped_lock lock(map_lock_); env_map = env_map_; } for (auto& entry : env_map) { SharedEnvInst envinst = entry.second; int er = RunCommand(envinst, CommandType::InterruptOnly, setup_code_event_handler); if (er) { // Nothing to do here, really. } } + } return it; }
1709-1709: Queue reset during shutdown may drop pending eventsThe queue is reset unconditionally during shutdown, which could silently drop pending code events and prevent user hooks from being called.
While this might be acceptable during shutdown, consider documenting this behavior or draining the queue first if event delivery is important for cleanup:
void EnvList::removed_env_cb_(ns_async*, EnvList* envlist) { + // Note: Pending code events will be dropped during shutdown envlist->on_code_event_q_.reset();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
node.gyp(1 hunks)src/nsolid.cc(2 hunks)src/nsolid.h(7 hunks)src/nsolid/nsolid_api.cc(8 hunks)src/nsolid/nsolid_api.h(11 hunks)src/nsolid/nsolid_code_event_handler.cc(1 hunks)src/nsolid/nsolid_code_event_handler.h(1 hunks)test/addons/nsolid-code-event-hook/binding.cc(1 hunks)test/addons/nsolid-code-event-hook/binding.gyp(1 hunks)test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/nsolid/nsolid_code_event_handler.h
🚧 Files skipped from review as they are similar to previous changes (7)
- test/addons/nsolid-code-event-hook/binding.gyp
- node.gyp
- src/nsolid/nsolid_code_event_handler.cc
- src/nsolid.cc
- test/addons/nsolid-code-event-hook/binding.cc
- src/nsolid.h
- src/nsolid/nsolid_api.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
src/nsolid/nsolid_api.cc (6)
<retrieved_learning>
Learnt from: santigimeno
PR: #319
File: src/nsolid/async_ts_queue.h:123-135
Timestamp: 2025-06-03T11:35:52.830Z
Learning: The unbounded timer loop issue in AsyncTSQueue has been resolved by implementing proper timer arming logic with the timer_armed_ atomic flag and adding a queue emptiness check (!queue->queue_.empty()) in the async_callback method before restarting the timer. This prevents the timer from firing continuously when the queue is empty.
</retrieved_learning>
<retrieved_learning>
Learnt from: santigimeno
PR: #319
File: src/nsolid/async_ts_queue.h:123-135
Timestamp: 2025-06-03T11:35:52.830Z
Learning: The unbounded timer loop issue in AsyncTSQueue has been resolved by implementing proper timer arming logic with the timer_armed_ atomic flag and adding a queue emptiness check (!queue->queue_.empty()) in the async_callback method before restarting the timer. This prevents the timer from firing continuously when the queue is empty.
</retrieved_learning>
<retrieved_learning>
Learnt from: santigimeno
PR: #339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
</retrieved_learning>
<retrieved_learning>
Learnt from: santigimeno
PR: #339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., test/addons/*/binding.cc), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
</retrieved_learning>
<retrieved_learning>
Learnt from: santigimeno
PR: #327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
</retrieved_learning>
<retrieved_learning>
Learnt from: santigimeno
PR: #339
File: src/nsolid/nsolid_elf_utils.cc:18-18
Timestamp: 2025-07-08T16:05:45.341Z
Learning: In the NSolid project, the GetBuildId function in src/nsolid/nsolid_elf_utils.cc is designed to be called only from the NSolid thread, so the static cache doesn't require thread safety mechanisms like mutex protection.
</retrieved_learning>
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (4)
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
🪛 Biome (1.9.4)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
🔇 Additional comments (10)
test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js (3)
12-19: LGTM: Hook registration logic is well-structured.The
registerHookfunction properly validates callback parameters and maintains thread-specific state tracking. The use of bitwise OR forcustomFnNameReceivedis appropriate for accumulating multiple potential matches.
27-42: LGTM: Test state setup and hook registration.The test properly sets up two independent hook states and registers them correctly. The state structure is clear and supports the test's validation logic.
79-81: LGTM: Hook registration in main thread.The main thread properly registers two hooks before spawning the worker thread, ensuring the hook indices are available for the worker.
src/nsolid/nsolid_api.cc (7)
2-2: LGTM!Clean include of the code event handler header.
844-855: LGTM!The handler setup and disable methods are correctly implemented:
setup_code_event_handler()properly checks for existing handler before creatingdisable_code_event_handler()usesreset()to properly clean up resources- Both methods handle the handler lifecycle appropriately
886-892: LGTM!The async queue initialization is correctly implemented:
- Creates a typed queue for
CodeEventInfo- Uses proper lambda callback syntax with move semantics
- Integrates with the existing EnvList thread loop
1030-1053: LGTM!The hook removal logic is correctly implemented:
- Properly removes the hook from the list
- Checks if it was the last hook (size == 0) before disabling handlers
- Uses the same pattern as registration for consistency
1117-1119: LGTM!Correctly integrates code event handler setup into environment creation:
- Checks if hooks exist before enabling handler
- Follows the established pattern for conditional setup
1495-1505: LGTM!The static wrapper methods properly:
- Verify they're running on the correct isolate/thread
- Delegate to the appropriate EnvInst instance methods
- Follow the established command pattern
1979-1992: LGTM!The event dispatching logic is well-implemented:
- Properly handles the case where the environment no longer exists
- Optimizes for single hook case by moving instead of copying
- Uses the enhanced
for_eachmethod that provides size information
santigimeno
force-pushed
the
santi/jit_code_event_handler
branch
from
41949db to
9bda80b
Compare
Adds a for_each method to TSList that passes the current list size and item to the callback. Updates erase to return the new size after removal. These changes improve thread-safe iteration and management of hooks.
Introduces a new API to register code event hooks in nsolid. This includes new handler classes, queueing, and infrastructure to allow extensible code event observation and callbacks.
santigimeno
force-pushed
the
santi/jit_code_event_handler
branch
from
9bda80b to
cae6e4e
Compare
|
closing ticket since this must be complete and has been in this pipeline for more than 10 sprints |
Introduces a new API to register code event hooks in nsolid. This
includes new handler classes, queueing, and infrastructure to allow
extensible code event observation and callbacks.
Also:
Fixes: https://github.com/nodesource/ebpf-roadmap/issues/7
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests