src: split ownsProcessState off isMainThread

addaleax · targos · commit 01bb7b755927 · 2019-02-10T19:56:10.000+01:00
Embedders may want to control whether a Node.js instance controls the current process, similar to what we currently have with `Worker`s. Previously, the `isMainThread` flag had a bit of a double usage, both for indicating whether we are (not) running a Worker and whether we can modify per-process state. PR-URL: #25881 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -14,7 +14,7 @@
 
 // This file is compiled as if it's wrapped in a function with arguments
 // passed by node::LoadEnvironment()
-/* global process, loaderExports, isMainThread */
+/* global process, loaderExports, isMainThread, ownsProcessState */
 
 const { internalBinding, NativeModule } = loaderExports;
 
@@ -51,7 +51,7 @@ const perThreadSetup = NativeModule.require('internal/process/per_thread');
 let mainThreadSetup;
 // Bootstrappers for the worker threads only
 let workerThreadSetup;
-if (isMainThread) {
+if (ownsProcessState) {
   mainThreadSetup = NativeModule.require(
     'internal/process/main_thread_only'
   );
@@ -140,7 +140,7 @@ if (credentials.implementsPosixCredentials) {
   process.getegid = credentials.getegid;
   process.getgroups = credentials.getgroups;
 
-  if (isMainThread) {
+  if (ownsProcessState) {
     const wrapped = mainThreadSetup.wrapPosixCredentialSetters(credentials);
     process.initgroups = wrapped.initgroups;
     process.setgroups = wrapped.setgroups;
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
@@ -30,9 +30,10 @@ const { deserializeError } = require('internal/error-serdes');
 const { pathToFileURL } = require('url');
 
 const {
-  Worker: WorkerImpl,
+  ownsProcessState,
+  isMainThread,
   threadId,
-  isMainThread
+  Worker: WorkerImpl,
 } = internalBinding('worker');
 
 const kHandle = Symbol('kHandle');
@@ -251,7 +252,8 @@ function pipeWithoutWarning(source, dest) {
 }
 
 module.exports = {
+  ownsProcessState,
+  isMainThread,
   threadId,
   Worker,
-  isMainThread
 };
diff --git a/lib/trace_events.js b/lib/trace_events.js
@@ -13,8 +13,8 @@ const {
   ERR_INVALID_ARG_TYPE
 } = require('internal/errors').codes;
 
-const { isMainThread } = require('internal/worker');
-if (!hasTracing || !isMainThread)
+const { ownsProcessState } = require('internal/worker');
+if (!hasTracing || !ownsProcessState)
   throw new ERR_TRACE_EVENTS_UNAVAILABLE();
 
 const { CategorySet, getEnabledCategories } = internalBinding('trace_events');
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -134,8 +134,12 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
   std::vector<std::string> args(argv, argv + argc);
   std::vector<std::string> exec_args(exec_argv, exec_argv + exec_argc);
   // TODO(addaleax): Provide more sensible flags, in an embedder-accessible way.
-  Environment* env =
-      new Environment(isolate_data, context, Environment::kIsMainThread);
+  Environment* env = new Environment(
+      isolate_data,
+      context,
+      static_cast<Environment::Flags>(Environment::kIsMainThread |
+                                      Environment::kOwnsProcessState |
+                                      Environment::kOwnsInspector));
   env->Start(per_process::v8_is_profiling);
   env->ProcessCliArgs(args, exec_args);
   return env;
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -650,6 +650,14 @@ inline bool Environment::is_main_thread() const {
   return flags_ & kIsMainThread;
 }
 
+inline bool Environment::owns_process_state() const {
+  return flags_ & kOwnsProcessState;
+}
+
+inline bool Environment::owns_inspector() const {
+  return flags_ & kOwnsInspector;
+}
+
 inline uint64_t Environment::thread_id() const {
   return thread_id_;
 }
diff --git a/src/env.cc b/src/env.cc
@@ -142,7 +142,7 @@ void InitThreadLocalOnce() {
 }
 
 void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
-  if (!env_->is_main_thread()) {
+  if (!env_->owns_process_state()) {
     // Ideally, we&rsquo;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
diff --git a/src/env.h b/src/env.h
@@ -597,7 +597,9 @@ class Environment {
 
   enum Flags {
     kNoFlags = 0,
-    kIsMainThread = 1
+    kIsMainThread = 1 << 0,
+    kOwnsProcessState = 1 << 1,
+    kOwnsInspector = 1 << 2,
   };
 
   static inline Environment* GetCurrent(v8::Isolate* isolate);
@@ -766,6 +768,8 @@ class Environment {
   inline void set_has_run_bootstrapping_code(bool has_run_bootstrapping_code);
 
   inline bool is_main_thread() const;
+  inline bool owns_process_state() const;
+  inline bool owns_inspector() const;
   inline uint64_t thread_id() const;
   inline worker::Worker* worker_context() const;
   inline void set_worker_context(worker::Worker* context);
diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc
@@ -141,7 +141,7 @@ DispatchResponse TracingAgent::start(
     return DispatchResponse::Error(
         "Call NodeTracing::end to stop tracing before updating the config");
   }
-  if (!env_->is_main_thread()) {
+  if (!env_->owns_process_state()) {
     return DispatchResponse::Error(
         "Tracing properties can only be changed through main thread sessions");
   }
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -686,7 +686,7 @@ bool Agent::Start(const std::string& path,
   host_port_ = host_port;
 
   client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
-  if (parent_env_->is_main_thread()) {
+  if (parent_env_->owns_inspector()) {
     CHECK_EQ(start_io_thread_async_initialized.exchange(true), false);
     CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
                               &start_io_thread_async,
diff --git a/src/node.cc b/src/node.cc
@@ -318,16 +318,18 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
       loader_exports_obj->Get(context, env->require_string()).ToLocalChecked();
   env->set_native_module_require(require.As<Function>());
 
-  // process, loaderExports, isMainThread
+  // process, loaderExports, isMainThread, ownsProcessState, primordials
   std::vector<Local<String>> node_params = {
       env->process_string(),
       FIXED_ONE_BYTE_STRING(isolate, "loaderExports"),
       FIXED_ONE_BYTE_STRING(isolate, "isMainThread"),
+      FIXED_ONE_BYTE_STRING(isolate, "ownsProcessState"),
       env->primordials_string()};
   std::vector<Local<Value>> node_args = {
       process,
       loader_exports_obj,
       Boolean::New(isolate, env->is_main_thread()),
+      Boolean::New(isolate, env->owns_process_state()),
       env->primordials()};
 
   MaybeLocal<Value> result = ExecuteBootstrapper(
@@ -756,7 +758,12 @@ 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, Environment::kIsMainThread);
+  Environment env(
+      isolate_data,
+      context,
+      static_cast<Environment::Flags>(Environment::kIsMainThread |
+                                      Environment::kOwnsProcessState |
+                                      Environment::kOwnsInspector));
   env.Start(per_process::v8_is_profiling);
   env.ProcessCliArgs(args, exec_args);
 
diff --git a/src/node_credentials.cc b/src/node_credentials.cc
@@ -175,7 +175,7 @@ static void GetEGid(const FunctionCallbackInfo<Value>& args) {
 
 static void SetGid(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK(env->is_main_thread());
+  CHECK(env->owns_process_state());
 
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsUint32() || args[0]->IsString());
@@ -194,7 +194,7 @@ static void SetGid(const FunctionCallbackInfo<Value>& args) {
 
 static void SetEGid(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK(env->is_main_thread());
+  CHECK(env->owns_process_state());
 
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsUint32() || args[0]->IsString());
@@ -213,7 +213,7 @@ static void SetEGid(const FunctionCallbackInfo<Value>& args) {
 
 static void SetUid(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK(env->is_main_thread());
+  CHECK(env->owns_process_state());
 
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsUint32() || args[0]->IsString());
@@ -232,7 +232,7 @@ static void SetUid(const FunctionCallbackInfo<Value>& args) {
 
 static void SetEUid(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK(env->is_main_thread());
+  CHECK(env->owns_process_state());
 
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsUint32() || args[0]->IsString());
@@ -363,7 +363,7 @@ static void Initialize(Local<Object> target,
   env->SetMethodNoSideEffect(target, "getegid", GetEGid);
   env->SetMethodNoSideEffect(target, "getgroups", GetGroups);
 
-  if (env->is_main_thread()) {
+  if (env->owns_process_state()) {
     env->SetMethod(target, "initgroups", InitGroups);
     env->SetMethod(target, "setegid", SetEGid);
     env->SetMethod(target, "seteuid", SetEUid);
diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -72,7 +72,7 @@ static void Abort(const FunctionCallbackInfo<Value>& args) {
 
 static void Chdir(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK(env->is_main_thread());
+  CHECK(env->owns_process_state());
 
   CHECK_EQ(args.Length(), 1);
   CHECK(args[0]->IsString());
@@ -392,17 +392,17 @@ static void InitializeProcessMethods(Local<Object> target,
   Environment* env = Environment::GetCurrent(context);
 
   // define various internal methods
-  if (env->is_main_thread()) {
+  if (env->owns_process_state()) {
     env->SetMethod(target, "_debugProcess", DebugProcess);
     env->SetMethod(target, "_debugEnd", DebugEnd);
-    env->SetMethod(
-        target, "_startProfilerIdleNotifier", StartProfilerIdleNotifier);
-    env->SetMethod(
-        target, "_stopProfilerIdleNotifier", StopProfilerIdleNotifier);
     env->SetMethod(target, "abort", Abort);
     env->SetMethod(target, "chdir", Chdir);
   }
 
+  env->SetMethod(
+      target, "_startProfilerIdleNotifier", StartProfilerIdleNotifier);
+  env->SetMethod(target, "_stopProfilerIdleNotifier", StopProfilerIdleNotifier);
+
   env->SetMethod(target, "umask", Umask);
   env->SetMethod(target, "_rawDebug", RawDebug);
   env->SetMethod(target, "memoryUsage", MemoryUsage);
diff --git a/src/node_process_object.cc b/src/node_process_object.cc
@@ -86,15 +86,17 @@ MaybeLocal<Object> CreateProcessObject(
 
   // process.title
   auto title_string = FIXED_ONE_BYTE_STRING(env->isolate(), "title");
-  CHECK(process->SetAccessor(
-      env->context(),
-      title_string,
-      ProcessTitleGetter,
-      env->is_main_thread() ? ProcessTitleSetter : nullptr,
-      env->as_external(),
-      DEFAULT,
-      None,
-      SideEffectType::kHasNoSideEffect).FromJust());
+  CHECK(process
+            ->SetAccessor(
+                env->context(),
+                title_string,
+                ProcessTitleGetter,
+                env->owns_process_state() ? ProcessTitleSetter : nullptr,
+                env->as_external(),
+                DEFAULT,
+                None,
+                SideEffectType::kHasNoSideEffect)
+            .FromJust());
 
   // process.version
   READONLY_PROPERTY(process,
@@ -302,11 +304,13 @@ MaybeLocal<Object> CreateProcessObject(
 
   // process.debugPort
   auto debug_port_string = FIXED_ONE_BYTE_STRING(env->isolate(), "debugPort");
-  CHECK(process->SetAccessor(env->context(),
-                             debug_port_string,
-                             DebugPortGetter,
-                             env->is_main_thread() ? DebugPortSetter : nullptr,
-                             env->as_external()).FromJust());
+  CHECK(process
+            ->SetAccessor(env->context(),
+                          debug_port_string,
+                          DebugPortGetter,
+                          env->owns_process_state() ? DebugPortSetter : nullptr,
+                          env->as_external())
+            .FromJust());
 
   // process._rawDebug: may be overwritten later in JS land, but should be
   // availbale from the begining for debugging purposes
diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -569,6 +569,12 @@ void InitWorker(Local<Object> target,
             FIXED_ONE_BYTE_STRING(env->isolate(), "isMainThread"),
             Boolean::New(env->isolate(), env->is_main_thread()))
       .FromJust();
+
+  target
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(env->isolate(), "ownsProcessState"),
+            Boolean::New(env->isolate(), env->owns_process_state()))
+      .FromJust();
 }
 
 }  // anonymous namespace

Original file line number	Diff line number	Diff line change
`@@ -650,6 +650,14 @@ inline bool Environment::is_main_thread() const {`
`650`	`650`	`return flags_ & kIsMainThread;`
`651`	`651`	`}`
`652`	`652`
	`653`	`+inline bool Environment::owns_process_state() const {`
	`654`	`+ return flags_ & kOwnsProcessState;`
	`655`	`+}`
	`656`	`+`
	`657`	`+inline bool Environment::owns_inspector() const {`
	`658`	`+ return flags_ & kOwnsInspector;`
	`659`	`+}`
	`660`	`+`
`653`	`661`	`inline uint64_t Environment::thread_id() const {`
`654`	`662`	`return thread_id_;`
`655`	`663`	`}`
Original file line number	Diff line number	Diff line change
`@@ -142,7 +142,7 @@ void InitThreadLocalOnce() {`
`142`	`142`	`}`
`143`	`143`
`144`	`144`	`void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {`
`145`		`- if (!env_->is_main_thread()) {`
	`145`	`+ if (!env_->owns_process_state()) {`
`146`	`146`	`// Ideally, we’d have a consistent story that treats all threads/Environment`
`147`	`147`	`// instances equally here. However, tracing is essentially global, and this`
`148`	`148`	// callback is called from whichever thread calls `StartTracing()` or
Original file line number	Diff line number	Diff line change
`@@ -141,7 +141,7 @@ DispatchResponse TracingAgent::start(`
`141`	`141`	`return DispatchResponse::Error(`
`142`	`142`	`"Call NodeTracing::end to stop tracing before updating the config");`
`143`	`143`	`}`
`144`		`- if (!env_->is_main_thread()) {`
	`144`	`+ if (!env_->owns_process_state()) {`
`145`	`145`	`return DispatchResponse::Error(`
`146`	`146`	`"Tracing properties can only be changed through main thread sessions");`
`147`	`147`	`}`