fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader thr…

…eadsafe and non-global
nodejs · nodejs-github-bot · Jan 18, 2023 · Dec 22, 2022 · Jan 6, 2023 · Jan 6, 2023
commit fcdb475f4927a6cebedfae4da795b7af08c9f6ad
diff --git a/src/node_builtins.cc b/src/node_builtins.cc
@@ -59,8 +59,6 @@ bool BuiltinLoader::Exists(const char* id) {
 }
 
 bool BuiltinLoader::Add(const char* id, const UnionBytes& source) {
-  builtin_categories_
-      .reset();  // The category cache is reset by adding new sources
   auto result = source_.write()->emplace(id, source);
   return result.second;
 }
@@ -90,13 +88,8 @@ std::vector<std::string> BuiltinLoader::GetBuiltinIds() const {
   return ids;
 }
 
-const BuiltinLoader::BuiltinCategories&
-BuiltinLoader::InitializeBuiltinCategories() const {
-  if (LIKELY(builtin_categories_.has_value())) {
-    DCHECK(!builtin_categories_.value().can_be_required.empty());
-    return builtin_categories_.value();
-  }
-  auto& builtin_categories = builtin_categories_.emplace();
+BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const {
+  BuiltinCategories builtin_categories;
 
   std::vector<std::string> prefixes = {
 #if !HAVE_OPENSSL
@@ -160,22 +153,6 @@ BuiltinLoader::InitializeBuiltinCategories() const {
   return builtin_categories;
 }
 
-const std::set<std::string>& BuiltinLoader::GetCannotBeRequired() const {
-  return InitializeBuiltinCategories().cannot_be_required;
-}
-
-const std::set<std::string>& BuiltinLoader::GetCanBeRequired() const {
-  return InitializeBuiltinCategories().can_be_required;
-}
-
-bool BuiltinLoader::CanBeRequired(const char* id) const {
-  return GetCanBeRequired().count(id) == 1;
-}
-
-bool BuiltinLoader::CannotBeRequired(const char* id) const {
-  return GetCannotBeRequired().count(id) == 1;
-}
-
 const ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache(
     const char* id) const {
   RwLock::ScopedReadLock lock(code_cache_mutex_);
@@ -563,28 +540,29 @@ void BuiltinLoader::GetBuiltinCategories(
   Local<Context> context = env->context();
   Local<Object> result = Object::New(isolate);
 
-  std::set<std::string> cannot_be_required =
-      env->builtin_loader()->GetCannotBeRequired();
-  std::set<std::string> can_be_required =
-      env->builtin_loader()->GetCanBeRequired();
+  BuiltinCategories builtin_categories =
+      env->builtin_loader()->GetBuiltinCategories();
 
   if (!env->owns_process_state()) {
-    can_be_required.erase("trace_events");
-    cannot_be_required.insert("trace_events");
+    builtin_categories.can_be_required.erase("trace_events");
+    builtin_categories.cannot_be_required.insert("trace_events");
   }
 
   Local<Value> cannot_be_required_js;
   Local<Value> can_be_required_js;
 
-  if (!ToV8Value(context, cannot_be_required).ToLocal(&cannot_be_required_js))
+  if (!ToV8Value(context, builtin_categories.cannot_be_required)
+           .ToLocal(&cannot_be_required_js))
     return;
   if (result
           ->Set(context,
                 OneByteString(isolate, "cannotBeRequired"),
                 cannot_be_required_js)
           .IsNothing())
     return;
-  if (!ToV8Value(context, can_be_required).ToLocal(&can_be_required_js)) return;
+  if (!ToV8Value(context, builtin_categories.can_be_required)
+           .ToLocal(&can_be_required_js))
+    return;
   if (result
           ->Set(context,
                 OneByteString(isolate, "canBeRequired"),

diff --git a/src/node_builtins.h b/src/node_builtins.h
@@ -96,12 +96,10 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
     std::set<std::string> can_be_required;
     std::set<std::string> cannot_be_required;
   };
-  const BuiltinCategories& InitializeBuiltinCategories() const;
-  const std::set<std::string>& GetCannotBeRequired() const;
-  const std::set<std::string>& GetCanBeRequired() const;
-
-  bool CanBeRequired(const char* id) const;
-  bool CannotBeRequired(const char* id) const;
+  // This method builds `BuiltinCategories` from scratch every time,
+  // and is therefore somewhat expensive, but also currently only being
+  // used for testing, so that should not be an issue.
+  BuiltinCategories GetBuiltinCategories() const;
 
   const v8::ScriptCompiler::CachedData* GetCodeCache(const char* id) const;
   enum class Result { kWithCache, kWithoutCache };
@@ -137,10 +135,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
 
   void AddExternalizedBuiltin(const char* id, const char* filename);
 
-  // builtin_categories is marked mutable because it is only initialized
-  // as a cache, so that mutating it does not change any state.
-  mutable std::optional<BuiltinCategories> builtin_categories_;
-
   ThreadsafeCopyOnWrite<BuiltinSourceMap> source_;
 
   const UnionBytes config_;