tools: lint thread_local in src

Several Environments can share a thread, so state in src/ that belongs
to one of them cannot be kept in a thread_local. Add a cpplint rule
that rejects thread_local in src/ unless the declaration is marked with
NOLINTNEXTLINE(runtime/thread_local), and mark the existing uses, which
are per-thread by design.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

Author: codebytere

src/api/environment.cc

@@ -1048,6 +1048,7 @@ Maybe<void> InitializePrimordials(Local<Context> context,
   // in the first place. However, creating BuiltinLoader instances is
   // relatively cheap and all the scripts that we may want to run at
   // startup are always present in it.
+  // NOLINTNEXTLINE(runtime/thread_local)
   thread_local builtins::BuiltinLoader builtin_loader;
   // Primordials can always be just eagerly compiled.
   builtin_loader.SetEagerCompile();

src/env.cc

@@ -1442,6 +1442,7 @@ void Environment::ClosePerEnvHandles() {
   close_and_finish(reinterpret_cast<uv_handle_t*>(&task_queues_async_));
 }
 
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local int handle_cleanup_depth = 0;
 
 void Environment::CleanupHandles() {

src/node_binding.cc

@@ -180,6 +180,7 @@ struct dl_wrap {
 static Mutex dlhandles_mutex;
 static std::unordered_set<dl_wrap*, dl_wrap::hash, dl_wrap::equal>
     dlhandles;
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local std::string dlerror_storage;
 
 char* wrapped_dlerror() {
@@ -286,6 +287,7 @@ using v8::Value;
 // Globals per process
 static node_module* modlist_internal;
 static node_module* modlist_linked;
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local node_module* thread_local_modpending;
 
 // This is set by node::Init() which is used by embedders

src/node_debug.cc

@@ -24,8 +24,10 @@ using v8::Number;
 using v8::Object;
 using v8::Value;
 
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local std::unordered_map<FastStringKey, int, FastStringKey::Hash>
     generic_usage_counters;
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local std::unordered_map<FastStringKey, int, FastStringKey::Hash>
     v8_fast_api_call_counts;
 

src/node_errors.cc

@@ -190,9 +190,11 @@ static std::string GetErrorSource(Isolate* isolate,
 }
 
 static std::atomic<bool> is_in_oom{false};
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local std::atomic<bool> is_retrieving_js_stacktrace{false};
 // This is thread-local because it only guards re-entrancy within the current
 // thread's uncaught-exception path; no cross-thread synchronization is needed.
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local bool is_in_uncaught_exception = false;
 MaybeLocal<StackTrace> GetCurrentStackTrace(Isolate* isolate, int frame_count) {
   if (isolate == nullptr) {

src/node_internals.h

@@ -286,6 +286,7 @@ class InternalCallbackScope {
 // Non-zero while an Environment on this thread is closing its handles with JS
 // disallowed isolate-wide; InternalCallbackScope re-allows it for the other
 // Environments whose callbacks run in those loop turns.
+// NOLINTNEXTLINE(runtime/thread_local)
 extern thread_local int handle_cleanup_depth;
 
 class DebugSealHandleScope {

src/quic/data.cc

@@ -33,6 +33,7 @@ using v8::Undefined;
 using v8::Value;
 
 namespace quic {
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local int DebugIndentScope::indent_ = 0;
 
 Path::Path(const SocketAddress& local, const SocketAddress& remote) {

src/quic/defs.h

@@ -395,6 +395,7 @@ class DebugIndentScope final {
   }
 
  private:
+  // NOLINTNEXTLINE(runtime/thread_local)
   static thread_local int indent_;
 };
 

tools/cpplint.py

@@ -350,6 +350,7 @@
     "runtime/printf_format",
     "runtime/references",
     "runtime/string",
+    "runtime/thread_local",
     "runtime/threadsafe_fn",
     "runtime/vlog",
     "runtime/v8_persistent",
@@ -7446,6 +7447,25 @@ def CheckStringValueUsage(filename, lines, error):
             'Use node::TwoByteValue instead.')
 
 
+def CheckThreadLocalUsage(filename, lines, error):
+  """Logs an error if thread_local is used in src/.
+  Args:
+    filename: The name of the current file.
+    lines: An array of strings, each representing a line of the file.
+    error: The function to call with any errors found.
+  """
+  if not (filename.startswith('src/') or filename.startswith('src\\')):
+    return
+
+  for linenum, line in enumerate(lines):
+    if re.search(r'\bthread_local\b', line.split('//', 1)[0]):
+      error(filename, linenum, 'runtime/thread_local', 5,
+            'Several Environments can share a thread, so keep state that '
+            'belongs to one on the Environment or its BindingData. Mark '
+            'intentionally per-thread state with '
+            'NOLINTNEXTLINE(runtime/thread_local).')
+
+
 def ProcessLine(
     filename,
     file_extension,
@@ -7609,6 +7629,8 @@ def ProcessFileData(filename, file_extension, lines, error, extra_check_function
 
     CheckStringValueUsage(filename, lines, error)
 
+    CheckThreadLocalUsage(filename, lines, error)
+
 
 def ProcessConfigOverrides(filename):
     """Loads the configuration files and processes the config overrides.