apply pr suggestions

Author: RafaelGSS

lib/diagnostics_channel.js

@@ -446,10 +446,7 @@ function tracingChannel(nameOrChannels) {
   return new TracingChannel(nameOrChannels);
 }
 
-dc_binding.setPublishCallback((name, message) => {
-  const ch = channels.get(name);
-  if (ch) ch.publish(message);
-});
+dc_binding.linkNativeChannel((name) => channel(name));
 
 module.exports = {
   channel,

lib/internal/process/pre_execution.js

@@ -622,17 +622,11 @@ function initializeClusterIPC() {
 }
 
 function setupDiagnosticsChannel() {
-  // The C++ Channel::Publish() API needs a JS callback to dispatch
-  // messages. This callback is stored in BindingData but cleared during
-  // snapshot serialization. Since lib/diagnostics_channel.js module body
-  // may not re-run after snapshot deserialization, we must explicitly
-  // re-register it here.
+  // Re-link native channels after snapshot deserialization since
+  // JS references are cleared during serialization.
   const dc = require('diagnostics_channel');
   const dc_binding = internalBinding('diagnostics_channel');
-  dc_binding.setPublishCallback((name, message) => {
-    const ch = dc.channel(name);
-    if (ch.hasSubscribers) ch.publish(message);
-  });
+  dc_binding.linkNativeChannel((name) => dc.channel(name));
 }
 
 function initializePermission() {

src/node_diagnostics_channel.cc

@@ -11,6 +11,7 @@ namespace node {
 namespace diagnostics_channel {
 
 using v8::Context;
+using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::HandleScope;
 using v8::Isolate;
@@ -66,22 +67,49 @@ void BindingData::GetOrCreateChannelIndex(
   args.GetReturnValue().Set(index);
 }
 
-void BindingData::SetPublishCallback(const FunctionCallbackInfo<Value>& args) {
+void BindingData::LinkNativeChannel(const FunctionCallbackInfo<Value>& args) {
   Realm* realm = Realm::GetCurrent(args);
   BindingData* binding = realm->GetBindingData<BindingData>();
   CHECK_NOT_NULL(binding);
 
   CHECK(args[0]->IsFunction());
-  binding->publish_callback_.Reset(realm->isolate(),
-                                   args[0].As<v8::Function>());
+  Isolate* isolate = realm->isolate();
+  Local<Context> context = realm->context();
+  binding->link_callback_.Reset(isolate, args[0].As<Function>());
+
+  // Resolve channels created before the link callback was available.
+  for (uint32_t index : binding->pending_channels_) {
+    for (const auto& pair : binding->channel_indices_) {
+      if (pair.second == index) {
+        Local<String> name =
+            String::NewFromUtf8(isolate, pair.first.c_str()).ToLocalChecked();
+        Local<Value> argv[] = {name};
+        Local<Value> result;
+        if (binding->link_callback_.Get(isolate)
+                ->Call(context, v8::Undefined(isolate), 1, argv)
+                .ToLocal(&result) &&
+            result->IsObject()) {
+          if (index >= binding->js_channels_.size()) {
+            binding->js_channels_.resize(index + 1);
+          }
+          binding->js_channels_[index].Reset(isolate, result.As<Object>());
+        }
+        break;
+      }
+    }
+  }
+  binding->pending_channels_.clear();
 }
 
 bool BindingData::PrepareForSerialization(Local<Context> context,
                                           SnapshotCreator* creator) {
   DCHECK_NULL(internal_field_info_);
   internal_field_info_ = InternalFieldInfoBase::New<InternalFieldInfo>(type());
   internal_field_info_->subscribers = subscribers_.Serialize(context, creator);
-  publish_callback_.Reset();
+  link_callback_.Reset();
+  for (auto& global : js_channels_) {
+    global.Reset();
+  }
   return true;
 }
 
@@ -109,7 +137,7 @@ void BindingData::CreatePerIsolateProperties(IsolateData* isolate_data,
   Isolate* isolate = isolate_data->isolate();
   SetMethod(
       isolate, target, "getOrCreateChannelIndex", GetOrCreateChannelIndex);
-  SetMethod(isolate, target, "setPublishCallback", SetPublishCallback);
+  SetMethod(isolate, target, "linkNativeChannel", LinkNativeChannel);
 }
 
 void BindingData::CreatePerContextProperties(Local<Object> target,
@@ -124,38 +152,72 @@ void BindingData::CreatePerContextProperties(Local<Object> target,
 void BindingData::RegisterExternalReferences(
     ExternalReferenceRegistry* registry) {
   registry->Register(GetOrCreateChannelIndex);
-  registry->Register(SetPublishCallback);
+  registry->Register(LinkNativeChannel);
 }
 
-Channel::Channel(BindingData* binding_data, uint32_t index, const char* name)
-    : binding_data_(binding_data), index_(index), name_(name) {}
+Channel::Channel(BindingData* binding_data, uint32_t index)
+    : binding_data_(binding_data), index_(index) {}
 
 Channel Channel::Get(Environment* env, const char* name) {
   Realm* realm = env->principal_realm();
   BindingData* binding = realm->GetBindingData<BindingData>();
   if (binding == nullptr) {
-    return Channel(nullptr, 0, name);
+    return Channel(nullptr, 0);
   }
   uint32_t index = binding->GetOrCreateChannelIndex(std::string(name));
-  return Channel(binding, index, name);
+
+  if (!binding->link_callback_.IsEmpty()) {
+    if (index >= binding->js_channels_.size()) {
+      binding->js_channels_.resize(index + 1);
+    }
+    if (binding->js_channels_[index].IsEmpty()) {
+      Isolate* isolate = env->isolate();
+      HandleScope handle_scope(isolate);
+      Local<Context> context = env->context();
+      Local<String> js_name =
+          String::NewFromUtf8(isolate, name).ToLocalChecked();
+      Local<Value> argv[] = {js_name};
+      Local<Value> result;
+      if (binding->link_callback_.Get(isolate)
+              ->Call(context, v8::Undefined(isolate), 1, argv)
+              .ToLocal(&result) &&
+          result->IsObject()) {
+        binding->js_channels_[index].Reset(isolate, result.As<Object>());
+      }
+    }
+  } else {
+    binding->pending_channels_.push_back(index);
+  }
+
+  return Channel(binding, index);
 }
 
 void Channel::Publish(Environment* env, Local<Value> message) const {
   if (!HasSubscribers()) return;
 
+  if (binding_data_ == nullptr) return;
+
   Isolate* isolate = env->isolate();
   HandleScope handle_scope(isolate);
   Local<Context> context = env->context();
   Context::Scope context_scope(context);
 
-  if (binding_data_->publish_callback_.IsEmpty()) return;
+  if (index_ >= binding_data_->js_channels_.size() ||
+      binding_data_->js_channels_[index_].IsEmpty()) {
+    return;
+  }
 
-  Local<v8::Function> callback = binding_data_->publish_callback_.Get(isolate);
-  Local<String> channel_name =
-      String::NewFromUtf8(isolate, name_).ToLocalChecked();
+  Local<Object> js_channel =
+      binding_data_->js_channels_[index_].Get(isolate);
+  Local<Value> publish_val;
+  if (!js_channel->Get(context, FIXED_ONE_BYTE_STRING(isolate, "publish"))
+           .ToLocal(&publish_val) ||
+      !publish_val->IsFunction()) {
+    return;
+  }
 
-  Local<Value> argv[] = {channel_name, message};
-  USE(callback->Call(context, v8::Undefined(isolate), 2, argv));
+  Local<Value> argv[] = {message};
+  USE(publish_val.As<Function>()->Call(context, js_channel, 1, argv));
 }
 
 }  // namespace diagnostics_channel

src/node_diagnostics_channel.h

@@ -6,6 +6,7 @@
 #include <cinttypes>
 #include <string>
 #include <unordered_map>
+#include <vector>
 #include "aliased_buffer.h"
 #include "node_snapshotable.h"
 
@@ -42,11 +43,13 @@ class BindingData : public SnapshotableObject {
 
   uint32_t GetOrCreateChannelIndex(const std::string& name);
 
-  v8::Global<v8::Function> publish_callback_;
+  v8::Global<v8::Function> link_callback_;
+  std::vector<v8::Global<v8::Object>> js_channels_;
+  std::vector<uint32_t> pending_channels_;
 
   static void GetOrCreateChannelIndex(
       const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void SetPublishCallback(
+  static void LinkNativeChannel(
       const v8::FunctionCallbackInfo<v8::Value>& args);
 
   static void CreatePerIsolateProperties(IsolateData* isolate_data,
@@ -72,11 +75,12 @@ class Channel {
   void Publish(Environment* env, v8::Local<v8::Value> message) const;
 
  private:
-  Channel(BindingData* binding_data, uint32_t index, const char* name);
+  friend class BindingData;
+
+  Channel(BindingData* binding_data, uint32_t index);
 
   BindingData* binding_data_;
   uint32_t index_;
-  const char* name_;
 };
 
 }  // namespace diagnostics_channel

src/permission/permission.cc

@@ -28,7 +28,7 @@ namespace permission {
 
 namespace {
 
-const char* GetDiagnosticsChannelName(PermissionScope scope) {
+constexpr std::string_view GetDiagnosticsChannelName(PermissionScope scope) {
   switch (scope) {
     case PermissionScope::kFileSystem:
     case PermissionScope::kFileSystemRead:
@@ -215,16 +215,16 @@ bool Permission::is_scope_granted(Environment* env,
   }
 
   if (!result && !publishing_) {
-    const char* channel_name = GetDiagnosticsChannelName(permission);
+    auto channel_name = GetDiagnosticsChannelName(permission);
     if (channel_name != nullptr) {
       diagnostics_channel::Channel ch =
-          diagnostics_channel::Channel::Get(env, channel_name);
+          GetOrCreateChannel(env, permission);
       if (ch.HasSubscribers()) {
         publishing_ = true;
         v8::Isolate* isolate = env->isolate();
         v8::HandleScope handle_scope(isolate);
         v8::Local<v8::Context> context = env->context();
-        v8::Local<v8::Object> msg = v8::Object::New(isolate);
+        v8::Local<v8::Object> msg = v8::Object::New(isolate, v8::Null(isolate), nullptr, nullptr, 0);
         const char* perm_str = PermissionToString(permission);
         msg->Set(context,
                  FIXED_ONE_BYTE_STRING(isolate, "permission"),
@@ -247,6 +247,19 @@ bool Permission::is_scope_granted(Environment* env,
   return result;
 }
 
+diagnostics_channel::Channel Permission::GetOrCreateChannel(
+    Environment* env, PermissionScope scope) const {
+  auto it = channels_.find(scope);
+  if (it != channels_.end()) {
+    return it->second;
+  }
+  const char* name = GetDiagnosticsChannelName(scope);
+  diagnostics_channel::Channel ch =
+      diagnostics_channel::Channel::Get(env, name);
+  channels_.emplace(scope, ch);
+  return ch;
+}
+
 void Permission::Apply(Environment* env,
                        const std::vector<std::string>& allow,
                        PermissionScope scope) {

src/permission/permission.h

@@ -13,6 +13,7 @@
 #include "permission/permission_base.h"
 #include "permission/wasi_permission.h"
 #include "permission/worker_permission.h"
+#include "node_diagnostics_channel.h"
 #include "v8.h"
 
 #include <string_view>
@@ -124,10 +125,15 @@ class Permission {
                                       const PermissionScope permission,
                                       const std::string_view& res = "") const;
 
+  diagnostics_channel::Channel GetOrCreateChannel(
+      Environment* env, PermissionScope scope) const;
+
   std::unordered_map<PermissionScope, std::shared_ptr<PermissionBase>> nodes_;
   bool enabled_;
   bool warning_only_;
   mutable bool publishing_ = false;
+  mutable std::unordered_map<PermissionScope, diagnostics_channel::Channel>
+      channels_;
 };
 
 v8::MaybeLocal<v8::Value> CreateAccessDeniedError(Environment* env,