src: make creating per-binding data structures easier

Enable the state associated with the individual bindings, e.g. fs or http2, to be moved out of the Environment class, in order for these to be more modular and for Environment to be come less of a collection of random data fields. Do this by using a BaseObject as the data for callbacks, which can hold the per-binding state. By default, no per-binding state is available, although that can be configured when setting up the binding. PR-URL: #32538 Reviewed-By: James M Snell <[email protected]>
nodejs · Apr 7, 2020 · 1fc3de9 · 1fc3de9
1 parent dbd030a
commit 1fc3de9
Show file tree

Hide file tree

Showing 12 changed files with 104 additions and 31 deletions.
diff --git a/src/base_object-inl.h b/src/base_object-inl.h
@@ -100,15 +100,16 @@ Environment* BaseObject::env() const {
   return env_;
 }
 
-BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) {
-  CHECK_GT(obj->InternalFieldCount(), 0);
+BaseObject* BaseObject::FromJSObject(v8::Local<v8::Value> value) {
+  v8::Local<v8::Object> obj = value.As<v8::Object>();
+  DCHECK_GE(obj->InternalFieldCount(), BaseObject::kSlot);
   return static_cast<BaseObject*>(
       obj->GetAlignedPointerFromInternalField(BaseObject::kSlot));
 }
 
 
 template <typename T>
-T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
+T* BaseObject::FromJSObject(v8::Local<v8::Value> object) {
   return static_cast<T*>(FromJSObject(object));
 }
 

diff --git a/src/base_object.h b/src/base_object.h
@@ -61,9 +61,9 @@ class BaseObject : public MemoryRetainer {
   // was also passed to the `BaseObject()` constructor initially.
   // This may return `nullptr` if the C++ object has not been constructed yet,
   // e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
-  static inline BaseObject* FromJSObject(v8::Local<v8::Object> object);
+  static inline BaseObject* FromJSObject(v8::Local<v8::Value> object);
   template <typename T>
-  static inline T* FromJSObject(v8::Local<v8::Object> object);
+  static inline T* FromJSObject(v8::Local<v8::Value> object);
 
   // Make the `v8::Global` a weak reference and, `delete` this object once
   // the JS object has been garbage collected and there are no (strong)
@@ -152,7 +152,7 @@ class BaseObject : public MemoryRetainer {
 
 // Global alias for FromJSObject() to avoid churn.
 template <typename T>
-inline T* Unwrap(v8::Local<v8::Object> obj) {
+inline T* Unwrap(v8::Local<v8::Value> obj) {
   return BaseObject::FromJSObject<T>(obj);
 }
 

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -327,16 +327,48 @@ inline Environment* Environment::GetCurrent(
   return GetFromCallbackData(info.Data());
 }
 
-inline Environment* Environment::GetFromCallbackData(v8::Local<v8::Value> val) {
+Environment* Environment::GetFromCallbackData(v8::Local<v8::Value> val) {
   DCHECK(val->IsObject());
   v8::Local<v8::Object> obj = val.As<v8::Object>();
-  DCHECK_GE(obj->InternalFieldCount(), 1);
-  Environment* env =
-      static_cast<Environment*>(obj->GetAlignedPointerFromInternalField(0));
+  DCHECK_GE(obj->InternalFieldCount(),
+            BaseObject::kInternalFieldCount);
+  Environment* env = Unwrap<BaseObject>(obj)->env();
   DCHECK(env->as_callback_data_template()->HasInstance(obj));
   return env;
 }
 
+template <typename T>
+Environment::BindingScope<T>::BindingScope(Environment* env) : env(env) {
+  v8::Local<v8::Object> callback_data;
+  if (!env->MakeBindingCallbackData<T>().ToLocal(&callback_data))
+    return;
+  data = Unwrap<T>(callback_data);
+
+  // No nesting allowed currently.
+  CHECK_EQ(env->current_callback_data(), env->as_callback_data());
+  env->set_current_callback_data(callback_data);
+}
+
+template <typename T>
+Environment::BindingScope<T>::~BindingScope() {
+  env->set_current_callback_data(env->as_callback_data());
+}
+
+template <typename T>
+v8::MaybeLocal<v8::Object> Environment::MakeBindingCallbackData() {
+  v8::Local<v8::Function> ctor;
+  v8::Local<v8::Object> obj;
+  if (!as_callback_data_template()->GetFunction(context()).ToLocal(&ctor) ||
+      !ctor->NewInstance(context()).ToLocal(&obj)) {
+    return v8::MaybeLocal<v8::Object>();
+  }
+  T* data = new T(this, obj);
+  // This won't compile if T is not a BaseObject subclass.
+  CHECK_EQ(data, static_cast<BaseObject*>(data));
+  data->MakeWeak();
+  return obj;
+}
+
 inline Environment* Environment::GetThreadLocalEnv() {
   return static_cast<Environment*>(uv_key_get(&thread_local_env));
 }
@@ -1115,7 +1147,7 @@ inline v8::Local<v8::FunctionTemplate>
                                      v8::Local<v8::Signature> signature,
                                      v8::ConstructorBehavior behavior,
                                      v8::SideEffectType side_effect_type) {
-  v8::Local<v8::Object> external = as_callback_data();
+  v8::Local<v8::Object> external = current_callback_data();
   return v8::FunctionTemplate::New(isolate(), callback, external,
                                    signature, 0, behavior, side_effect_type);
 }
@@ -1253,7 +1285,7 @@ void Environment::modify_base_object_count(int64_t delta) {
 }
 
 int64_t Environment::base_object_count() const {
-  return base_object_count_;
+  return base_object_count_ - initial_base_object_count_;
 }
 
 void Environment::set_main_utf16(std::unique_ptr<v8::String::Value> str) {
@@ -1313,6 +1345,10 @@ void Environment::set_process_exit_handler(
   }
 }  // namespace node
 
+// These two files depend on each other. Including base_object-inl.h after this
+// file is the easiest way to avoid issues with that circular dependency.
+#include "base_object-inl.h"
+
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #endif  // SRC_ENV_INL_H_
diff --git a/src/env.cc b/src/env.cc
@@ -258,18 +258,30 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() {
   USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args));
 }
 
+class NoBindingData : public BaseObject {
+ public:
+  NoBindingData(Environment* env, Local<Object> obj) : BaseObject(env, obj) {}
+
+  SET_NO_MEMORY_INFO()
+  SET_MEMORY_INFO_NAME(NoBindingData)
+  SET_SELF_SIZE(NoBindingData)
+};
+
 void Environment::CreateProperties() {
   HandleScope handle_scope(isolate_);
   Local<Context> ctx = context();
-  Local<FunctionTemplate> templ = FunctionTemplate::New(isolate());
-  templ->InstanceTemplate()->SetInternalFieldCount(1);
-  Local<Object> obj = templ->GetFunction(ctx)
-                          .ToLocalChecked()
-                          ->NewInstance(ctx)
-                          .ToLocalChecked();
-  obj->SetAlignedPointerInInternalField(0, this);
-  set_as_callback_data(obj);
-  set_as_callback_data_template(templ);
+  {
+    Context::Scope context_scope(ctx);
+    Local<FunctionTemplate> templ = FunctionTemplate::New(isolate());
+    templ->InstanceTemplate()->SetInternalFieldCount(
+        BaseObject::kInternalFieldCount);
+    set_as_callback_data_template(templ);
+
+    Local<Object> obj = MakeBindingCallbackData<NoBindingData>()
+        .ToLocalChecked();
+    set_as_callback_data(obj);
+    set_current_callback_data(obj);
+  }
 
   // Store primordials setup by the per-context script in the environment.
   Local<Object> per_context_bindings =
@@ -417,6 +429,10 @@ Environment::Environment(IsolateData* isolate_data,
   // TODO(joyeecheung): deserialize when the snapshot covers the environment
   // properties.
   CreateProperties();
+
+  // This adjusts the return value of base_object_count() so that tests that
+  // check the count do not have to account for internally created BaseObjects.
+  initial_base_object_count_ = base_object_count();
 }
 
 Environment::~Environment() {
@@ -467,7 +483,7 @@ Environment::~Environment() {
     }
   }
 
-  CHECK_EQ(base_object_count(), 0);
+  CHECK_EQ(base_object_count_, 0);
 }
 
 void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {

diff --git a/src/env.h b/src/env.h
@@ -436,6 +436,7 @@ constexpr size_t kFsStatsBufferLength =
   V(async_hooks_promise_resolve_function, v8::Function)                        \
   V(buffer_prototype_object, v8::Object)                                       \
   V(crypto_key_object_constructor, v8::Function)                               \
+  V(current_callback_data, v8::Object)                                         \
   V(domain_callback, v8::Function)                                             \
   V(domexception_function, v8::Function)                                       \
   V(enhance_fatal_stack_after_inspector, v8::Function)                         \
@@ -871,6 +872,24 @@ class Environment : public MemoryRetainer {
 
   static inline Environment* GetFromCallbackData(v8::Local<v8::Value> val);
 
+  // Methods created using SetMethod(), SetPrototypeMethod(), etc. inside
+  // this scope can access the created T* object using
+  // Unwrap<T>(args.Data()) later.
+  template <typename T>
+  struct BindingScope {
+    explicit inline BindingScope(Environment* env);
+    inline ~BindingScope();
+
+    T* data = nullptr;
+    Environment* env;
+
+    inline operator bool() const { return data != nullptr; }
+    inline bool operator !() const { return data == nullptr; }
+  };
+
+  template <typename T>
+  inline v8::MaybeLocal<v8::Object> MakeBindingCallbackData();
+
   static uv_key_t thread_local_env;
   static inline Environment* GetThreadLocalEnv();
 
@@ -1458,6 +1477,7 @@ class Environment : public MemoryRetainer {
   bool started_cleanup_ = false;
 
   int64_t base_object_count_ = 0;
+  int64_t initial_base_object_count_ = 0;
   std::atomic_bool is_stopping_ { false };
 
   std::function<void(Environment*, int)> process_exit_handler_ {

diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc
@@ -108,7 +108,7 @@ void FSEventWrap::Initialize(Local<Object> target,
   Local<FunctionTemplate> get_initialized_templ =
       FunctionTemplate::New(env->isolate(),
                             GetInitialized,
-                            env->as_callback_data(),
+                            env->current_callback_data(),
                             Signature::New(env->isolate(), t));
 
   t->PrototypeTemplate()->SetAccessorProperty(

diff --git a/src/node.cc b/src/node.cc
@@ -331,7 +331,7 @@ MaybeLocal<Value> Environment::BootstrapNode() {
 
   Local<String> env_string = FIXED_ONE_BYTE_STRING(isolate_, "env");
   Local<Object> env_var_proxy;
-  if (!CreateEnvVarProxy(context(), isolate_, as_callback_data())
+  if (!CreateEnvVarProxy(context(), isolate_, current_callback_data())
            .ToLocal(&env_var_proxy) ||
       process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) {
     return MaybeLocal<Value>();

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -505,7 +505,7 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
   Local<FunctionTemplate> ctx_getter_templ =
       FunctionTemplate::New(env->isolate(),
                             CtxGetter,
-                            env->as_callback_data(),
+                            env->current_callback_data(),
                             Signature::New(env->isolate(), t));
 
 
@@ -5101,7 +5101,7 @@ void DiffieHellman::Initialize(Environment* env, Local<Object> target) {
     Local<FunctionTemplate> verify_error_getter_templ =
         FunctionTemplate::New(env->isolate(),
                               DiffieHellman::VerifyErrorGetter,
-                              env->as_callback_data(),
+                              env->current_callback_data(),
                               Signature::New(env->isolate(), t),
                               /* length */ 0,
                               ConstructorBehavior::kThrow,

diff --git a/src/node_process_object.cc b/src/node_process_object.cc
@@ -147,7 +147,7 @@ void PatchProcessObject(const FunctionCallbackInfo<Value>& args) {
                 FIXED_ONE_BYTE_STRING(isolate, "title"),
                 ProcessTitleGetter,
                 env->owns_process_state() ? ProcessTitleSetter : nullptr,
-                env->as_callback_data(),
+                env->current_callback_data(),
                 DEFAULT,
                 None,
                 SideEffectType::kHasNoSideEffect)
@@ -198,7 +198,7 @@ void PatchProcessObject(const FunctionCallbackInfo<Value>& args) {
                           FIXED_ONE_BYTE_STRING(isolate, "debugPort"),
                           DebugPortGetter,
                           env->owns_process_state() ? DebugPortSetter : nullptr,
-                          env->as_callback_data())
+                          env->current_callback_data())
             .FromJust());
 }
 

diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc
@@ -142,7 +142,7 @@ Local<FunctionTemplate> LibuvStreamWrap::GetConstructorTemplate(
     Local<FunctionTemplate> get_write_queue_size =
         FunctionTemplate::New(env->isolate(),
                               GetWriteQueueSize,
-                              env->as_callback_data(),
+                              env->current_callback_data(),
                               Signature::New(env->isolate(), tmpl));
     tmpl->PrototypeTemplate()->SetAccessorProperty(
         env->write_queue_size_string(),

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -1276,7 +1276,7 @@ void TLSWrap::Initialize(Local<Object> target,
   Local<FunctionTemplate> get_write_queue_size =
       FunctionTemplate::New(env->isolate(),
                             GetWriteQueueSize,
-                            env->as_callback_data(),
+                            env->current_callback_data(),
                             Signature::New(env->isolate(), t));
   t->PrototypeTemplate()->SetAccessorProperty(
       env->write_queue_size_string(),

diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc
@@ -145,7 +145,7 @@ void UDPWrap::Initialize(Local<Object> target,
   Local<FunctionTemplate> get_fd_templ =
       FunctionTemplate::New(env->isolate(),
                             UDPWrap::GetFD,
-                            env->as_callback_data(),
+                            env->current_callback_data(),
                             signature);
 
   t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(),