From 39eca841c30e1a54424b85f34e8dd56b2648f930 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 1 Feb 2019 23:47:38 +0100 Subject: [PATCH] src: split ownsProcessState off isMainThread 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: https://github.com/nodejs/node/pull/25881 Reviewed-By: Colin Ihrig Reviewed-By: Joyee Cheung Reviewed-By: James M Snell --- lib/internal/bootstrap/node.js | 6 +++--- lib/internal/worker.js | 8 +++++--- lib/trace_events.js | 4 ++-- src/api/environment.cc | 8 ++++++-- src/env-inl.h | 8 ++++++++ src/env.cc | 2 +- src/env.h | 6 +++++- src/inspector/tracing_agent.cc | 2 +- src/inspector_agent.cc | 2 +- src/node.cc | 11 +++++++++-- src/node_credentials.cc | 10 +++++----- src/node_process_methods.cc | 12 ++++++------ src/node_process_object.cc | 32 ++++++++++++++++++-------------- src/node_worker.cc | 6 ++++++ 14 files changed, 76 insertions(+), 41 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 915d07d83dd..358c4cb8c71 100644 --- 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 index f28f4cd6635..1d1a5e7d228 100644 --- 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'); @@ -243,7 +244,8 @@ function pipeWithoutWarning(source, dest) { } module.exports = { + ownsProcessState, + isMainThread, threadId, Worker, - isMainThread }; diff --git a/lib/trace_events.js b/lib/trace_events.js index d2977679e16..41c4dbe741c 100644 --- 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 index e54252824bd..bac3a1e42b7 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -143,8 +143,12 @@ Environment* CreateEnvironment(IsolateData* isolate_data, std::vector args(argv, argv + argc); std::vector 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::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 index 0a47b7cfcae..748577a2545 100644 --- 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 index 3d4f39026b0..d099a9e62ad 100644 --- 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’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 index f76fb14990c..40bd0797a2f 100644 --- a/src/env.h +++ b/src/env.h @@ -599,7 +599,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); @@ -768,6 +770,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 index fb8467a1b9d..09d213d8ae5 100644 --- 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 index fb85a54408e..f6255489fcb 100644 --- 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(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 index e1350ef4f1e..20206b8199b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -314,16 +314,18 @@ MaybeLocal RunBootstrapping(Environment* env) { loader_exports_obj->Get(context, env->require_string()).ToLocalChecked(); env->set_native_module_require(require.As()); - // process, loaderExports, isMainThread + // process, loaderExports, isMainThread, ownsProcessState, primordials std::vector> 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> node_args = { process, loader_exports_obj, Boolean::New(isolate, env->is_main_thread()), + Boolean::New(isolate, env->owns_process_state()), env->primordials()}; MaybeLocal result = ExecuteBootstrapper( @@ -752,7 +754,12 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, HandleScope handle_scope(isolate); Local context = NewContext(isolate); Context::Scope context_scope(context); - Environment env(isolate_data, context, Environment::kIsMainThread); + Environment env( + isolate_data, + context, + static_cast(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 index 1fea2659f79..8d38c38a0c5 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -175,7 +175,7 @@ static void GetEGid(const FunctionCallbackInfo& args) { static void SetGid(const FunctionCallbackInfo& 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& args) { static void SetEGid(const FunctionCallbackInfo& 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& args) { static void SetUid(const FunctionCallbackInfo& 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& args) { static void SetEUid(const FunctionCallbackInfo& 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 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 index 51758f05f6b..a6d2c252e77 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -72,7 +72,7 @@ static void Abort(const FunctionCallbackInfo& args) { static void Chdir(const FunctionCallbackInfo& 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 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 index f83f553c997..159d9fdc407 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -86,15 +86,17 @@ MaybeLocal 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, @@ -290,11 +292,13 @@ MaybeLocal 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 index d457ab0c3e1..2d960f6e4de 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -569,6 +569,12 @@ void InitWorker(Local 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