tracing: refactor tracing state ownership

doc/api/tracing.md

@@ -82,6 +82,8 @@ as the one used by `process.hrtime()`
 however the trace-event timestamps are expressed in microseconds,
 unlike `process.hrtime()` which returns nanoseconds.
 
+The features from this module are not available in [`Worker`][] threads.
+
 ## The `trace_events` module
 <!-- YAML
 added: v10.0.0
@@ -205,3 +207,4 @@ console.log(trace_events.getEnabledCategories());
 [Performance API]: perf_hooks.html
 [V8]: v8.html
 [`async_hooks`]: async_hooks.html
+[`Worker`]: worker_threads.html#worker_threads_class_worker

doc/api/worker_threads.md

@@ -253,6 +253,7 @@ Notable differences inside a Worker environment are:
 - Execution may stop at any point as a result of [`worker.terminate()`][]
   being invoked.
 - IPC channels from parent processes are not accessible.
+- The [`trace_events`][] module is not supported.
 
 Currently, the following differences also exist until they are addressed:
 
@@ -489,6 +490,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
 [Signals events]: process.html#process_signal_events
 [`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
+[`trace_events`]: tracing.html
 [browser `MessagePort`]: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
 [child processes]: child_process.html
 [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

lib/trace_events.js

@@ -13,7 +13,8 @@ const {
   ERR_INVALID_ARG_TYPE
 } = require('internal/errors').codes;
 
-if (!hasTracing)
+const { isMainThread } = require('internal/worker');
+if (!hasTracing || !isMainThread)
   throw new ERR_TRACE_EVENTS_UNAVAILABLE();
 
 const { CategorySet, getEnabledCategories } = internalBinding('trace_events');

src/env-inl.h

@@ -334,10 +334,6 @@ inline v8::Isolate* Environment::isolate() const {
   return isolate_;
 }
 
-inline tracing::AgentWriterHandle* Environment::tracing_agent_writer() const {
-  return tracing_agent_writer_;
-}
-
 inline Environment* Environment::from_timer_handle(uv_timer_t* handle) {
   return ContainerOf(&Environment::timer_handle_, handle);
 }

src/env.cc

@@ -128,11 +128,22 @@ void InitThreadLocalOnce() {
 }
 
 void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
+  if (!env_->is_main_thread()) {
+    // Ideally, we’d have a consistent story that treats all threads/Environment
+    // instances equally here. However, tracing is essentially global, and this
+    // callback is called from whichever thread calls `StartTracing()` or
+    // `StopTracing()`. The only way to do this in a threadsafe fashion
+    // seems to be only tracking this from the main thread, and only allowing
+    // these state modifications from the main thread.
+    return;
+  }
+
   env_->trace_category_state()[0] =
       *TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
           TRACING_CATEGORY_NODE1(async_hooks));
 
   Isolate* isolate = env_->isolate();
+  HandleScope handle_scope(isolate);
   Local<Function> cb = env_->trace_category_state_function();
   if (cb.IsEmpty())
     return;
@@ -143,11 +154,9 @@ void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
 }
 
 Environment::Environment(IsolateData* isolate_data,
-                         Local<Context> context,
-                         tracing::AgentWriterHandle* tracing_agent_writer)
+                         Local<Context> context)
     : isolate_(context->GetIsolate()),
       isolate_data_(isolate_data),
-      tracing_agent_writer_(tracing_agent_writer),
       immediate_info_(context->GetIsolate()),
       tick_info_(context->GetIsolate()),
       timer_base_(uv_now(isolate_data->event_loop())),
@@ -182,10 +191,9 @@ Environment::Environment(IsolateData* isolate_data,
 
   AssignToContext(context, ContextInfo(""));
 
-  if (tracing_agent_writer_ != nullptr) {
-    trace_state_observer_.reset(new TrackingTraceStateObserver(this));
-    v8::TracingController* tracing_controller =
-        tracing_agent_writer_->GetTracingController();
+  if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
+    trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);
+    v8::TracingController* tracing_controller = writer->GetTracingController();
     if (tracing_controller != nullptr)
       tracing_controller->AddTraceStateObserver(trace_state_observer_.get());
   }
@@ -234,9 +242,10 @@ Environment::~Environment() {
   context()->SetAlignedPointerInEmbedderData(
       ContextEmbedderIndex::kEnvironment, nullptr);
 
-  if (tracing_agent_writer_ != nullptr) {
-    v8::TracingController* tracing_controller =
-        tracing_agent_writer_->GetTracingController();
+  if (trace_state_observer_) {
+    tracing::AgentWriterHandle* writer = GetTracingAgentWriter();
+    CHECK_NOT_NULL(writer);
+    v8::TracingController* tracing_controller = writer->GetTracingController();
     if (tracing_controller != nullptr)
       tracing_controller->RemoveTraceStateObserver(trace_state_observer_.get());
   }

src/env.h

@@ -593,8 +593,7 @@ class Environment {
   static inline Environment* GetThreadLocalEnv();
 
   Environment(IsolateData* isolate_data,
-              v8::Local<v8::Context> context,
-              tracing::AgentWriterHandle* tracing_agent_writer);
+              v8::Local<v8::Context> context);
   ~Environment();
 
   void Start(const std::vector<std::string>& args,
@@ -630,7 +629,6 @@ class Environment {
   inline bool profiler_idle_notifier_started() const;
 
   inline v8::Isolate* isolate() const;
-  inline tracing::AgentWriterHandle* tracing_agent_writer() const;
   inline uv_loop_t* event_loop() const;
   inline uint32_t watched_providers() const;
 
@@ -920,7 +918,6 @@ class Environment {
 
   v8::Isolate* const isolate_;
   IsolateData* const isolate_data_;
-  tracing::AgentWriterHandle* const tracing_agent_writer_;
   uv_timer_t timer_handle_;
   uv_check_t immediate_check_handle_;
   uv_idle_t immediate_idle_handle_;

src/inspector/tracing_agent.cc

@@ -1,4 +1,5 @@
 #include "tracing_agent.h"
+#include "node_internals.h"
 
 #include "env-inl.h"
 #include "v8.h"
@@ -64,6 +65,10 @@ DispatchResponse TracingAgent::start(
     return DispatchResponse::Error(
         "Call NodeTracing::end to stop tracing before updating the config");
   }
+  if (!env_->is_main_thread()) {
+    return DispatchResponse::Error(
+        "Tracing properties can only be changed through main thread sessions");
+  }
 
   std::set<std::string> categories_set;
   protocol::Array<std::string>* categories =
@@ -74,9 +79,9 @@ DispatchResponse TracingAgent::start(
   if (categories_set.empty())
     return DispatchResponse::Error("At least one category should be enabled");
 
-  auto* writer = env_->tracing_agent_writer();
+  tracing::AgentWriterHandle* writer = GetTracingAgentWriter();
   if (writer != nullptr) {
-    trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient(
+    trace_writer_ = writer->agent()->AddClient(
         categories_set,
         std::unique_ptr<InspectorTraceWriter>(
             new InspectorTraceWriter(frontend_.get())),

src/node.cc

@@ -379,6 +379,10 @@ static struct {
 #endif  //  !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR
 } v8_platform;
 
+tracing::AgentWriterHandle* GetTracingAgentWriter() {
+  return v8_platform.GetTracingAgentWriter();
+}
+
 #ifdef __POSIX__
 static const unsigned kMaxSignal = 32;
 #endif
@@ -2747,8 +2751,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
   // options than the global parse call.
   std::vector<std::string> args(argv, argv + argc);
   std::vector<std::string> exec_args(exec_argv, exec_argv + exec_argc);
-  Environment* env = new Environment(isolate_data, context,
-                                     v8_platform.GetTracingAgentWriter());
+  Environment* env = new Environment(isolate_data, context);
   env->Start(args, exec_args, v8_is_profiling);
   return env;
 }
@@ -2818,7 +2821,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   HandleScope handle_scope(isolate);
   Local<Context> context = NewContext(isolate);
   Context::Scope context_scope(context);
-  Environment env(isolate_data, context, v8_platform.GetTracingAgentWriter());
+  Environment env(isolate_data, context);
   env.Start(args, exec_args, v8_is_profiling);
 
   const char* path = args.size() > 1 ? args[1].c_str() : nullptr;

src/node_internals.h

@@ -538,6 +538,8 @@ int ThreadPoolWork::CancelWork() {
   return uv_cancel(reinterpret_cast<uv_req_t*>(&work_req_));
 }
 
+tracing::AgentWriterHandle* GetTracingAgentWriter();
+
 static inline const char* errno_string(int errorno) {
 #define ERRNO_CASE(e)  case e: return #e;
   switch (errorno) {

src/node_trace_events.cc

@@ -57,7 +57,7 @@ void NodeCategorySet::New(const FunctionCallbackInfo<Value>& args) {
     if (!*val) return;
     categories.emplace(*val);
   }
-  CHECK_NOT_NULL(env->tracing_agent_writer());
+  CHECK_NOT_NULL(GetTracingAgentWriter());
   new NodeCategorySet(env, args.This(), std::move(categories));
 }
 
@@ -68,7 +68,7 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (!category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent_writer()->Enable(categories);
+    GetTracingAgentWriter()->Enable(categories);
     category_set->enabled_ = true;
   }
 }
@@ -80,15 +80,15 @@ void NodeCategorySet::Disable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent_writer()->Disable(categories);
+    GetTracingAgentWriter()->Disable(categories);
     category_set->enabled_ = false;
   }
 }
 
 void GetEnabledCategories(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   std::string categories =
-      env->tracing_agent_writer()->agent()->GetEnabledCategories();
+      GetTracingAgentWriter()->agent()->GetEnabledCategories();
   if (!categories.empty()) {
     args.GetReturnValue().Set(
       String::NewFromUtf8(env->isolate(),

src/node_worker.cc

@@ -116,9 +116,7 @@ Worker::Worker(Environment* env, Local<Object> wrap, const std::string& url)
     Context::Scope context_scope(context);
 
     // TODO(addaleax): Use CreateEnvironment(), or generally another public API.
-    env_.reset(new Environment(isolate_data_.get(),
-                               context,
-                               nullptr));
+    env_.reset(new Environment(isolate_data_.get(), context));
     CHECK_NE(env_, nullptr);
     env_->set_abort_on_uncaught_exception(false);
     env_->set_worker_context(this);

test/parallel/test-trace-events-api-worker-disabled.js

@@ -0,0 +1,11 @@
+// Flags: --experimental-worker
+'use strict';
+
+const common = require('../common');
+const { Worker } = require('worker_threads');
+
+new Worker("require('trace_events')", { eval: true })
+  .on('error', common.expectsError({
+    code: 'ERR_TRACE_EVENTS_UNAVAILABLE',
+    type: Error
+  }));

test/parallel/test-trace-events-dynamic-enable-workers-disabled.js

@@ -0,0 +1,28 @@
+// Flags: --experimental-worker
+'use strict';
+
+const common = require('../common');
+const { Worker } = require('worker_threads');
+
+common.skipIfInspectorDisabled();
+
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+  new Worker(__filename);
+  return;
+}
+
+const assert = require('assert');
+const { Session } = require('inspector');
+
+const session = new Session();
+session.connect();
+session.post('NodeTracing.start', {
+  traceConfig: { includedCategories: ['node.perf'] }
+}, common.mustCall((err) => {
+  assert.deepStrictEqual(err, {
+    code: -32000,
+    message:
+      'Tracing properties can only be changed through main thread sessions'
+  });
+}));