fs: fix build and snapshot issues with fs channels

Define the inline channel helpers in node_file-inl.h so that including
node_file.h alone builds under -Werror=undefined-inline. Hold the cached
channels weakly, as the permission code does, and skip the
instrumentation while building a snapshot: linking a native channel
creates JS channel objects that cannot be serialized.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

src/node_file-inl.h

@@ -204,6 +204,21 @@ FSReqPromise<AliasedBufferT>::~FSReqPromise() {
   CHECK_IMPLIES(!finished_, !env()->can_call_into_js());
 }
 
+inline bool FSOperationChannelHasSubscribers(FSOperationChannels& channels,
+                                             FSOperationChannel channel) {
+  diagnostics_channel::Channel* ch =
+      channels[static_cast<size_t>(channel)].get();
+  return ch != nullptr && ch->HasSubscribers();
+}
+
+inline bool AnyFSOperationChannelHasSubscribers(FSOperationChannels& channels) {
+  for (size_t i = 0; i < kNumFSOperationChannels; i++) {
+    diagnostics_channel::Channel* ch = channels[i].get();
+    if (ch != nullptr && ch->HasSubscribers()) return true;
+  }
+  return false;
+}
+
 template <typename AliasedBufferT>
 FSReqPromise<AliasedBufferT>::FSReqPromise(BindingData* binding_data,
                                            v8::Local<v8::Object> obj,
@@ -233,8 +248,8 @@ void FSReqPromise<AliasedBufferT>::Reject(v8::Local<v8::Value> reject) {
 template <typename AliasedBufferT>
 void FSReqPromise<AliasedBufferT>::Resolve(v8::Local<v8::Value> value) {
   finished_ = true;
-  PublishFSOpCompletionEvent(this, FSOperationChannel::kAsyncEnd, "result",
-                             value);
+  PublishFSOpCompletionEvent(
+      this, FSOperationChannel::kAsyncEnd, "result", value);
   v8::HandleScope scope(env()->isolate());
   InternalCallbackScope callback_scope(this);
   v8::Local<v8::Value> val;
@@ -338,12 +353,18 @@ FSReqBase* AsyncDestCall(Environment* env, FSReqBase* req_wrap,
   // See SyncCallAndThrowIf: instrumentation is unsafe with a pending
   // exception.
   if (binding != nullptr && !env->isolate()->HasPendingException()) {
-    channels = &GetFSOperationChannels(binding, env, syscall);
+    channels = GetFSOperationChannels(binding, env, syscall);
     req_wrap->set_op_channels(channels);
-    if (FSOperationChannelHasSubscribers(*channels,
-                                         FSOperationChannel::kStart)) {
-      PublishFSOperationEvent(env, *channels, FSOperationChannel::kStart,
-                              api, nullptr, req_wrap->data(), -1, nullptr,
+    if (channels != nullptr && FSOperationChannelHasSubscribers(
+                                   *channels, FSOperationChannel::kStart)) {
+      PublishFSOperationEvent(env,
+                              *channels,
+                              FSOperationChannel::kStart,
+                              api,
+                              nullptr,
+                              req_wrap->data(),
+                              -1,
+                              nullptr,
                               v8::Local<v8::Value>());
     }
   }
@@ -363,8 +384,14 @@ FSReqBase* AsyncDestCall(Environment* env, FSReqBase* req_wrap,
     // The path is captured for the completion events; it requires a copy
     // since the uv request is cleaned up before they fire.
     req_wrap->set_op_path(path == nullptr ? std::string() : path);
-    PublishFSOperationEvent(env, *channels, FSOperationChannel::kEnd, api,
-                            path, req_wrap->data(), fd, nullptr,
+    PublishFSOperationEvent(env,
+                            *channels,
+                            FSOperationChannel::kEnd,
+                            api,
+                            path,
+                            req_wrap->data(),
+                            fd,
+                            nullptr,
                             v8::Local<v8::Value>());
   }
   return req_wrap;
@@ -426,12 +453,18 @@ int SyncCallAndThrowIf(Predicate should_throw,
   // of which is safe with a pending exception (a multi-step operation keeps
   // going after a failed step to clean up, e.g. write + close).
   if (binding != nullptr && !env->isolate()->HasPendingException()) {
-    channels = &GetFSOperationChannels(binding, env, req_wrap->syscall_p);
-    if (FSOperationChannelHasSubscribers(*channels,
-                                         FSOperationChannel::kStart)) {
-      PublishFSOperationEvent(env, *channels, FSOperationChannel::kStart,
-                              "sync", req_wrap->path_p, req_wrap->dest_p, -1,
-                              nullptr, v8::Local<v8::Value>());
+    channels = GetFSOperationChannels(binding, env, req_wrap->syscall_p);
+    if (channels != nullptr && FSOperationChannelHasSubscribers(
+                                   *channels, FSOperationChannel::kStart)) {
+      PublishFSOperationEvent(env,
+                              *channels,
+                              FSOperationChannel::kStart,
+                              "sync",
+                              req_wrap->path_p,
+                              req_wrap->dest_p,
+                              -1,
+                              nullptr,
+                              v8::Local<v8::Value>());
     }
   }
   int result = fn(nullptr, &(req_wrap->req), args..., nullptr);
@@ -449,9 +482,15 @@ int SyncCallAndThrowIf(Predicate should_throw,
                                                  nullptr,
                                                  req_wrap->path_p,
                                                  req_wrap->dest_p);
-        PublishFSOperationEvent(env, *channels, FSOperationChannel::kError,
-                                "sync", req_wrap->path_p, req_wrap->dest_p,
-                                fd, "error", error);
+        PublishFSOperationEvent(env,
+                                *channels,
+                                FSOperationChannel::kError,
+                                "sync",
+                                req_wrap->path_p,
+                                req_wrap->dest_p,
+                                fd,
+                                "error",
+                                error);
       }
     } else if (FSOperationChannelHasSubscribers(*channels,
                                                 FSOperationChannel::kEnd)) {

src/node_file.cc

@@ -104,24 +104,35 @@ const char* const kFSOperationEventNames[kNumFSOperationChannels] = {
     "error",
 };
 
-FSOperationChannels& GetFSOperationChannels(BindingData* binding,
+FSOperationChannels* GetFSOperationChannels(BindingData* binding,
                                             Environment* env,
                                             const char* operation) {
+  if (env->isolate_data()->is_building_snapshot()) return nullptr;
   auto& names = binding->fs_op_channel_names_;
+  FSOperationChannels* set = nullptr;
   for (size_t i = 0; i < names.size(); i++) {
     if (names[i] == operation) {
-      return *binding->fs_op_channel_sets_[i];
+      set = binding->fs_op_channel_sets_[i].get();
+      break;
     }
   }
-  auto set = std::make_unique<FSOperationChannels>();
+  if (set == nullptr) {
+    names.push_back(operation);
+    binding->fs_op_channel_sets_.push_back(
+        std::make_unique<FSOperationChannels>());
+    set = binding->fs_op_channel_sets_.back().get();
+  }
+  // Entries are weak: fill in any that were never created or whose owner
+  // (the diagnostics_channel BindingData) has released them since.
   for (size_t i = 0; i < kNumFSOperationChannels; i++) {
+    if ((*set)[i]) continue;
     std::string name = std::string("tracing:fs.") + operation + ":" +
                        kFSOperationEventNames[i];
-    (*set)[i] = diagnostics_channel::Channel::Get(env, name);
+    if (auto ch = diagnostics_channel::Channel::Get(env, name)) {
+      (*set)[i] = BaseObjectWeakPtr<diagnostics_channel::Channel>(ch.get());
+    }
   }
-  names.push_back(operation);
-  binding->fs_op_channel_sets_.push_back(std::move(set));
-  return *binding->fs_op_channel_sets_.back();
+  return set;
 }
 
 void PublishFSOperationEvent(Environment* env,
@@ -867,8 +878,8 @@ void FSReqCallback::ResolveStatFs(const uv_statfs_t* stat) {
 }
 
 void FSReqCallback::Resolve(Local<Value> value) {
-  PublishFSOpCompletionEvent(this, FSOperationChannel::kAsyncEnd, "result",
-                             value);
+  PublishFSOpCompletionEvent(
+      this, FSOperationChannel::kAsyncEnd, "result", value);
   Local<Value> argv[2]{Null(env()->isolate()), value};
   MakeCallback(env()->oncomplete_string(),
                value->IsUndefined() ? 1 : arraysize(argv),
@@ -899,8 +910,8 @@ FSReqAfterScope::FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req)
   CHECK_EQ(wrap_->req(), req);
   // The async work for the operation has completed; the continuation window
   // begins here.
-  PublishFSOpCompletionEvent(wrap, FSOperationChannel::kAsyncStart, nullptr,
-                             Local<Value>());
+  PublishFSOpCompletionEvent(
+      wrap, FSOperationChannel::kAsyncStart, nullptr, Local<Value>());
 }
 
 FSReqAfterScope::~FSReqAfterScope() {

src/node_file.h

@@ -81,37 +81,33 @@ class FSReqBase;
 class BindingData;
 
 // The event channels of one operation's channel family, all created when the
-// operation is first seen.
+// operation is first seen. The diagnostics_channel BindingData is the sole
+// owner of Channel objects; weak references keep this cache from holding them
+// alive past Realm teardown or into a snapshot, where the owner releases them
+// before serialization. A released entry is fetched again on the next use.
 using FSOperationChannels =
-    std::array<BaseObjectPtr<diagnostics_channel::Channel>,
+    std::array<BaseObjectWeakPtr<diagnostics_channel::Channel>,
                kNumFSOperationChannels>;
 
 // Returns the channel set for `operation`, fetching it once per call site so
 // that publishing several events for one operation costs a single lookup.
 // `operation` must be a string literal: the cache is keyed on its identity.
-// The returned reference stays valid for the lifetime of the BindingData.
-FSOperationChannels& GetFSOperationChannels(BindingData* binding,
+// The returned pointer stays valid for the lifetime of the BindingData.
+// Returns nullptr while a snapshot is being built: creating a channel links a
+// JS channel object, and those cannot be serialized.
+FSOperationChannels* GetFSOperationChannels(BindingData* binding,
                                             Environment* env,
                                             const char* operation);
 
 // Returns true if the given event channel has subscribers. Check before
 // calling PublishFSOperationEvent so the no-subscriber fast path stays free
-// of out-of-line calls and payload preparation.
+// of out-of-line calls and payload preparation. Defined in node_file-inl.h.
 inline bool FSOperationChannelHasSubscribers(FSOperationChannels& channels,
-                                             FSOperationChannel channel) {
-  diagnostics_channel::Channel* ch =
-      channels[static_cast<size_t>(channel)].get();
-  return ch != nullptr && ch->HasSubscribers();
-}
+                                             FSOperationChannel channel);
 
 // Returns true if any of the operation's event channels has subscribers.
-inline bool AnyFSOperationChannelHasSubscribers(FSOperationChannels& channels) {
-  for (size_t i = 0; i < kNumFSOperationChannels; i++) {
-    diagnostics_channel::Channel* ch = channels[i].get();
-    if (ch != nullptr && ch->HasSubscribers()) return true;
-  }
-  return false;
-}
+// Defined in node_file-inl.h.
+inline bool AnyFSOperationChannelHasSubscribers(FSOperationChannels& channels);
 
 // Publishes an event on one of the operation's channels. Payload fields are
 // set only when applicable: `path`/`dest` (which may be null or empty) for