Skip to content

src: add AddCodeEventHook API for code events #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: node-v22.x-nsolid-v5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,7 @@
'src/nsolid.cc',
'src/nsolid/continuous_profiler.cc',
'src/nsolid/nsolid_api.cc',
'src/nsolid/nsolid_code_event_handler.cc',
'src/nsolid/nsolid_trace.cc',
'src/nsolid/nsolid_cpu_profiler.cc',
'src/nsolid/nsolid_heap_snapshot.cc',
Expand Down
47 changes: 47 additions & 0 deletions src/nsolid.cc
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,42 @@ int Snapshot::get_snapshot_(SharedEnvInst envinst,
GetHeapSnapshot(envinst, redacted, data, proxy, deleter);
}

class CodeEventHook::Impl {
public:
Impl() = default;
~Impl() {
// Remove the hook from the TSList in EnvList
EnvList::Inst()->RemoveCodeEventHook(hook_);
}
Comment on lines +603 to +606
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This cannot ever happen afaict


private:
friend class CodeEventHook;
void Setup(internal::code_event_hook_proxy_sig cb,
void(*deleter)(void*),
void* data) {
// Add hook to the TSList in EnvList
hook_ = EnvList::Inst()->AddCodeEventHook(data, cb, deleter);
}

TSList<EnvList::CodeEventHookStor>::iterator hook_;
};

CodeEventHook::CodeEventHook(): impl_(std::make_unique<Impl>()) {
}

CodeEventHook::~CodeEventHook() = default;

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;
}

void CodeEventHook::DoSetup(internal::code_event_hook_proxy_sig cb,
internal::deleter_sig deleter,
void* data) {
impl_->Setup(cb, deleter, data);
}

namespace internal {

Expand Down Expand Up @@ -637,6 +673,17 @@ void thread_removed_hook_(void* data,
EnvList::Inst()->EnvironmentDeletionHook(data, proxy, deleter);
}

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;
}
Comment on lines +676 to +685
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

DoSetup cannot fail



int queue_callback_(void* data, queue_callback_proxy_sig proxy) {
return EnvList::Inst()->QueueCallback(proxy, data);
Expand Down
94 changes: 93 additions & 1 deletion src/nsolid.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ namespace nsolid {

class EnvInst;
struct LogWriteInfo;
class CodeEventHook;
struct CodeEventInfo;

#define kNSByte "byte"
#define kNSMhz "MHz"
Expand Down Expand Up @@ -285,6 +287,7 @@ using on_log_write_hook_proxy_sig = void(*)(SharedEnvInst, LogWriteInfo, void*);
using at_exit_hook_proxy_sig = void(*)(bool, bool, void*);
using thread_added_hook_proxy_sig = void(*)(SharedEnvInst, void*);
using thread_removed_hook_proxy_sig = thread_added_hook_proxy_sig;
using code_event_hook_proxy_sig = void(*)(SharedEnvInst, CodeEventInfo, void*);
using deleter_sig = void(*)(void*);
using user_data = std::unique_ptr<void, deleter_sig>;

Expand Down Expand Up @@ -314,6 +317,8 @@ void thread_added_hook_proxy_(SharedEnvInst, void* data);
template <typename G>
void thread_removed_hook_proxy_(SharedEnvInst, void* data);
template <typename G>
void code_event_hook_proxy_(SharedEnvInst, CodeEventInfo, void*);
template <typename G>
void delete_proxy_(void* g);


Expand Down Expand Up @@ -348,7 +353,9 @@ NODE_EXTERN void thread_added_hook_(void*,
NODE_EXTERN void thread_removed_hook_(void*,
thread_removed_hook_proxy_sig,
deleter_sig);

NODE_EXTERN CodeEventHook* add_code_event_hook_(void*,
code_event_hook_proxy_sig,
deleter_sig);
} // namespace internal

/** @endcond */
Expand Down Expand Up @@ -647,6 +654,52 @@ NODE_EXTERN int ThreadAddedHook(Cb&& cb, Data&&... data);
template <typename Cb, typename... Data>
NODE_EXTERN int ThreadRemovedHook(Cb&& cb, Data&&... data);

template <typename Cb, typename... Data>
NODE_EXTERN CodeEventHook* AddCodeEventHook(Cb&& cb, Data&&... data);

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;
};

/**
* @brief Opaque handle for a code event hook registered with AddCodeEventHook.
*
* Only Dispose() should be used to remove the hook and release resources.
*/
class NODE_EXTERN CodeEventHook {
public:
/**
* @brief Dispose and remove the registered code event hook.
*
* After calling Dispose(), this object must not be used again.
*/
void Dispose();

private:
CodeEventHook();
~CodeEventHook();
friend CodeEventHook* internal::add_code_event_hook_(
void*, internal::code_event_hook_proxy_sig, internal::deleter_sig);
CodeEventHook(const CodeEventHook&) = delete;
CodeEventHook& operator=(const CodeEventHook&) = delete;

void DoSetup(internal::code_event_hook_proxy_sig cb,
internal::deleter_sig deleter,
void* data);

class Impl;
std::unique_ptr<Impl> impl_;
};

/**
* @brief Defines the types of metrics supported
Expand Down Expand Up @@ -1776,6 +1829,38 @@ int ThreadRemovedHook(Cb&& cb, Data&&... data) {
}


/**
* @brief Register a hook (function) to be called on code events.
*
* @tparam Cb Callback type. The callback will be invoked with (...Data) arguments.
* @tparam Data Variable argument types to be propagated to the callback.
* @param cb Hook function with signature: cb(...Data)
* @param data Variable number of arguments to be propagated to the callback.
* @return Pointer to CodeEventHook if successful, nullptr otherwise.
*/
template <typename Cb, typename... Data>
inline CodeEventHook* AddCodeEventHook(Cb&& cb, Data&&... data) {
// NOLINTNEXTLINE(build/namespaces)
using namespace std::placeholders;
using UserData = decltype(std::bind(
std::forward<Cb>(cb), _1, _2, std::forward<Data>(data)...));

// _1 - SharedEnvInst
// _2 - CodeEventInfo
UserData* user_data = new (std::nothrow) UserData(std::bind(
std::forward<Cb>(cb), _1, _2, std::forward<Data>(data)...));
if (user_data == nullptr) {
return nullptr;
}

return internal::add_code_event_hook_(
user_data,
internal::code_event_hook_proxy_<UserData>,
internal::delete_proxy_<UserData>);
}



namespace internal {

template <typename G>
Expand Down Expand Up @@ -1842,6 +1927,13 @@ void thread_removed_hook_proxy_(SharedEnvInst envinst, void* data) {
(*static_cast<G*>(data))(envinst);
}

template <typename G>
void code_event_hook_proxy_(SharedEnvInst envinst,
CodeEventInfo info,
void* data) {
(*static_cast<G*>(data))(envinst, std::move(info));
}

template <typename G>
void delete_proxy_(void* g) {
delete static_cast<G*>(g);
Expand Down
105 changes: 105 additions & 0 deletions src/nsolid/nsolid_api.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "nsolid_api.h"
#include "nsolid/nsolid_code_event_handler.h"
#include "nsolid/nsolid_heap_snapshot.h"
#include "nsolid_bindings.h"
#include "node_buffer.h"
Expand Down Expand Up @@ -840,6 +841,19 @@ void EnvInst::StoreSourceCode(int script_id,
}


void EnvInst::setup_code_event_handler() {
if (!code_event_handler_) {
code_event_handler_ =
std::make_unique<NSolidCodeEventHandler>(isolate_, thread_id_);
code_event_handler_->Enable();
}
}

void EnvInst::disable_code_event_handler() {
code_event_handler_.reset();
}


EnvList::EnvList(): info_(nlohmann::json()) {
int er;
// Create event loop and new thread to run EnvList commands.
Expand Down Expand Up @@ -869,6 +883,13 @@ EnvList::EnvList(): info_(nlohmann::json()) {
er = thread_.create(env_list_routine_, this);
CHECK_EQ(er, 0);
continuous_profiler_ = std::make_shared<ContinuousProfiler>(&thread_loop_);
on_code_event_q_ =
AsyncTSQueue<CodeEventInfo>::create(
&thread_loop_,
+[](CodeEventInfo&& info, EnvList* envlist) {
envlist->got_code_event(std::move(info));
},
this);
}


Expand Down Expand Up @@ -978,6 +999,58 @@ void EnvList::OnUnblockedLoopHook(
{ 0, proxy, nsolid::internal::user_data(data, deleter) });
}

TSList<EnvList::CodeEventHookStor>::iterator EnvList::AddCodeEventHook(
void* data,
internal::code_event_hook_proxy_sig proxy,
internal::deleter_sig deleter) {

auto it = code_event_hook_list_.push_back(
{ proxy, nsolid::internal::user_data(data, deleter) });

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;
}

void EnvList::RemoveCodeEventHook(
TSList<EnvList::CodeEventHookStor>::iterator it) {
size_t size = code_event_hook_list_.erase(it);
if (size == 0) {
// No hooks left, disable the code event handler.
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,
disable_code_event_handler);
if (er) {
// Nothing to do here, really.
}
}
}
}

int EnvList::QueueCallback(q_cb_sig cb, uint64_t timeout, void* data) {
QCbTimeoutStor* stor = new (std::nothrow) QCbTimeoutStor({
Expand Down Expand Up @@ -1041,6 +1114,10 @@ NODE_PERFORMANCE_MILESTONES(V)
env->isolate()->AddGCEpilogueCallback(EnvInst::v8_gc_epilogue_cb_,
envinst_sp.get());

if (code_event_hook_list_.size() > 0) {
envinst_sp->setup_code_event_handler();
}

// Run EnvironmentCreationHook callbacks.
env_creation_list_.for_each([envinst_sp](auto& stor) {
stor.cb(envinst_sp, stor.data.get());
Expand Down Expand Up @@ -1415,6 +1492,18 @@ void EnvList::datapoint_cb_(std::queue<MetricsStream::Datapoint>&& q) {
});
}

void EnvList::setup_code_event_handler(SharedEnvInst envinst_sp) {
DCHECK_EQ(envinst_sp->isolate(), v8::Isolate::TryGetCurrent());

envinst_sp->setup_code_event_handler();
}

void EnvList::disable_code_event_handler(SharedEnvInst envinst_sp) {
DCHECK_EQ(envinst_sp->isolate(), v8::Isolate::TryGetCurrent());

envinst_sp->disable_code_event_handler();
}


void EnvInst::send_datapoint(MetricsStream::Type type,
double value) {
Expand Down Expand Up @@ -1617,9 +1706,11 @@ void EnvList::log_written_cb_(ns_async*, EnvList* envlist) {
// registered users will need to receive their last set of metrics and a
// notification that it'll be their last.
void EnvList::removed_env_cb_(ns_async*, EnvList* envlist) {
envlist->on_code_event_q_.reset();
if (envlist->continuous_profiler_) {
envlist->continuous_profiler_.reset();
}

Comment on lines +1709 to +1713
Copy link

@coderabbitai coderabbitai bot Jun 10, 2025

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Draining the queue synchronously (dequeue until empty) before reset.
  2. Or calling reset() after confirming thread_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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is relevant once EnvList is gone

Copy link

Choose a reason for hiding this comment

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

@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.

envlist->removed_env_msg_.close();
envlist->process_callbacks_msg_.close();
envlist->log_written_msg_.close();
Expand Down Expand Up @@ -1885,6 +1976,20 @@ void EnvList::fill_trace_id_q() {
}
}

void EnvList::got_code_event(CodeEventInfo&& info) {
auto envinst_sp = EnvInst::GetInst(info.thread_id);
if (envinst_sp == nullptr) {
return;
}

code_event_hook_list_.for_each([&info, &envinst_sp](auto& stor, size_t size) {
if (size == 1) {
stor.cb(envinst_sp, std::move(info), stor.data.get());
} else {
stor.cb(envinst_sp, info, stor.data.get());
}
});
}

void EnvInst::custom_command_(SharedEnvInst envinst_sp,
const std::string req_id) {
Expand Down
Loading
Loading