Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: move per-process global variables into node::per_process #25302

Closed
wants to merge 2 commits into from
+125 −91
Closed

src: move per-process global variables into node::per_process #25302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11ed69e
src: move per-process global variables into node::per_process
joyeecheung Jan 1, 2019
3a58828
fixup! src: move per-process global variables into node::per_process
joyeecheung Jan 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ IsolateData::IsolateData(Isolate* isolate,
if (platform_ != nullptr)
platform_->RegisterIsolate(isolate_, event_loop);

options_.reset(new PerIsolateOptions(*per_process_opts->per_isolate));
options_.reset(
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));

// Create string and private symbol properties as internalized one byte
// strings after the platform is properly initialized.
Expand Down
90 changes: 51 additions & 39 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,31 @@ using v8::Undefined;
using v8::V8;
using v8::Value;

namespace per_process {
// Tells whether --prof is passed.
// TODO(joyeecheung): move env->options()->prof_process to
// per_process::cli_options.prof_process and use that instead.
static bool v8_is_profiling = false;

// Bit flag used to track security reverts (see node_revert.h)
unsigned int reverted = 0;
// TODO(joyeecheung): these are no longer necessary. Remove them.
// See: https://github.com/nodejs/node/pull/25302#discussion_r244924196
// Isolate on the main thread
static Mutex main_isolate_mutex;
static Isolate* main_isolate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these actually consumed anywhere? It looks like we should remove this entirely…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are used to check that someone don't call Start multiple times and accidentally dispose the isolate being used in another thread..though I am having a hard time imaging how that would be possible with the current code...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node_isolate is kind of vestigial at this point, see commit 75adde0 from 2014. We couldn't remove it outright back then but we can now.

The mutex was introduced to fix a SIGUSR1 race, see commit 844f0a9. It too can be removed.


// node_revert.h
// Bit flag used to track security reverts.
unsigned int reverted_cve = 0;

// util.h
// Tells whether the per-process V8::Initialize() is called and
// if it is safe to call v8::Isolate::GetCurrent().
bool v8_initialized = false;

// node_internals.h
// process-relative uptime base, initialized at start-up
double prog_start_time;

Mutex per_process_opts_mutex;
std::shared_ptr<PerProcessOptions> per_process_opts {
new PerProcessOptions() };
static Mutex node_isolate_mutex;
static Isolate* node_isolate;
} // namespace per_process

// Ensures that __metadata trace events are only emitted
// when tracing is enabled.
Expand Down Expand Up @@ -265,14 +275,15 @@ static struct {
#endif // HAVE_INSPECTOR

void StartTracingAgent() {
if (per_process_opts->trace_event_categories.empty()) {
if (per_process::cli_options->trace_event_categories.empty()) {
tracing_file_writer_ = tracing_agent_->DefaultHandle();
} else {
tracing_file_writer_ = tracing_agent_->AddClient(
ParseCommaSeparatedSet(per_process_opts->trace_event_categories),
ParseCommaSeparatedSet(
per_process::cli_options->trace_event_categories),
std::unique_ptr<tracing::AsyncTraceWriter>(
new tracing::NodeTraceWriter(
per_process_opts->trace_event_file_pattern)),
per_process::cli_options->trace_event_file_pattern)),
tracing::Agent::kUseDefaultCategories);
}
}
Expand Down Expand Up @@ -496,7 +507,7 @@ const char* signo_string(int signo) {
}

void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || per_process_opts->zero_fill_all_buffers)
if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers)
return UncheckedCalloc(size);
else
return UncheckedMalloc(size);
Expand Down Expand Up @@ -1271,12 +1282,12 @@ void ProcessArgv(std::vector<std::string>* args,
{
// TODO(addaleax): The mutex here should ideally be held during the
// entire function, but that doesn't play well with the exit() calls below.
Mutex::ScopedLock lock(per_process_opts_mutex);
Mutex::ScopedLock lock(per_process::cli_options_mutex);
options_parser::PerProcessOptionsParser::instance.Parse(
args,
exec_args,
&v8_args,
per_process_opts.get(),
per_process::cli_options.get(),
is_env ? kAllowedInEnvironment : kDisallowedInEnvironment,
&errors);
}
Expand All @@ -1288,20 +1299,20 @@ void ProcessArgv(std::vector<std::string>* args,
exit(9);
}

if (per_process_opts->print_version) {
if (per_process::cli_options->print_version) {
printf("%s\n", NODE_VERSION);
exit(0);
}

if (per_process_opts->print_v8_help) {
if (per_process::cli_options->print_v8_help) {
V8::SetFlagsFromString("--help", 6);
exit(0);
}

for (const std::string& cve : per_process_opts->security_reverts)
for (const std::string& cve : per_process::cli_options->security_reverts)
Revert(cve.c_str());

auto env_opts = per_process_opts->per_isolate->per_env;
auto env_opts = per_process::cli_options->per_isolate->per_env;
if (std::find(v8_args.begin(), v8_args.end(),
"--abort-on-uncaught-exception") != v8_args.end() ||
std::find(v8_args.begin(), v8_args.end(),
Expand All @@ -1314,14 +1325,14 @@ void ProcessArgv(std::vector<std::string>* args,
// behavior but it could also interfere with the user's intentions in ways
// we fail to anticipate. Dillema.
if (std::find(v8_args.begin(), v8_args.end(), "--prof") != v8_args.end()) {
v8_is_profiling = true;
per_process::v8_is_profiling = true;
}

#ifdef __POSIX__
// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
// performance penalty of frequent EINTR wakeups when the profiler is running.
// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
if (v8_is_profiling) {
if (per_process::v8_is_profiling) {
uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
}
#endif
Expand Down Expand Up @@ -1350,7 +1361,7 @@ void ProcessArgv(std::vector<std::string>* args,
void Init(std::vector<std::string>* argv,
std::vector<std::string>* exec_argv) {
// Initialize prog_start_time to get relative uptime.
prog_start_time = static_cast<double>(uv_now(uv_default_loop()));
per_process::prog_start_time = static_cast<double>(uv_now(uv_default_loop()));

// Register built-in modules
binding::RegisterBuiltinModules();
Expand All @@ -1366,7 +1377,7 @@ void Init(std::vector<std::string>* argv,
#endif

std::shared_ptr<EnvironmentOptions> default_env_options =
per_process_opts->per_isolate->per_env;
per_process::cli_options->per_isolate->per_env;
{
std::string text;
default_env_options->pending_deprecation =
Expand Down Expand Up @@ -1395,7 +1406,7 @@ void Init(std::vector<std::string>* argv,
}

#if HAVE_OPENSSL
std::string* openssl_config = &per_process_opts->openssl_config;
std::string* openssl_config = &per_process::cli_options->openssl_config;
if (openssl_config->empty()) {
credentials::SafeGetenv("OPENSSL_CONF", openssl_config);
}
Expand Down Expand Up @@ -1429,16 +1440,17 @@ void Init(std::vector<std::string>* argv,
ProcessArgv(argv, exec_argv, false);

// Set the process.title immediately after processing argv if --title is set.
if (!per_process_opts->title.empty())
uv_set_process_title(per_process_opts->title.c_str());
if (!per_process::cli_options->title.empty())
uv_set_process_title(per_process::cli_options->title.c_str());

#if defined(NODE_HAVE_I18N_SUPPORT)
// If the parameter isn't given, use the env variable.
if (per_process_opts->icu_data_dir.empty())
credentials::SafeGetenv("NODE_ICU_DATA", &per_process_opts->icu_data_dir);
if (per_process::cli_options->icu_data_dir.empty())
credentials::SafeGetenv("NODE_ICU_DATA",
&per_process::cli_options->icu_data_dir);
// Initialize ICU.
// If icu_data_dir is empty here, it will load the 'minimal' data.
if (!i18n::InitializeICUDirectory(per_process_opts->icu_data_dir)) {
if (!i18n::InitializeICUDirectory(per_process::cli_options->icu_data_dir)) {
fprintf(stderr,
"%s: could not initialize ICU "
"(check NODE_ICU_DATA or --icu-data-dir parameters)\n",
Expand Down Expand Up @@ -1599,7 +1611,7 @@ 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);
Environment* env = new Environment(isolate_data, context);
env->Start(args, exec_args, v8_is_profiling);
env->Start(args, exec_args, per_process::v8_is_profiling);
return env;
}

Expand Down Expand Up @@ -1673,7 +1685,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
Local<Context> context = NewContext(isolate);
Context::Scope context_scope(context);
Environment env(isolate_data, context);
env.Start(args, exec_args, v8_is_profiling);
env.Start(args, exec_args, per_process::v8_is_profiling);

const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
StartInspector(&env, path);
Expand Down Expand Up @@ -1780,9 +1792,9 @@ inline int Start(uv_loop_t* event_loop,
return 12; // Signal internal error.

{
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
CHECK_NULL(node_isolate);
node_isolate = isolate;
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
CHECK_NULL(per_process::main_isolate);
per_process::main_isolate = isolate;
}

int exit_code;
Expand All @@ -1807,9 +1819,9 @@ inline int Start(uv_loop_t* event_loop,
}

{
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
CHECK_EQ(node_isolate, isolate);
node_isolate = nullptr;
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
CHECK_EQ(per_process::main_isolate, isolate);
per_process::main_isolate = nullptr;
}

isolate->Dispose();
Expand Down Expand Up @@ -1857,14 +1869,14 @@ int Start(int argc, char** argv) {
V8::SetEntropySource(crypto::EntropySource);
#endif // HAVE_OPENSSL

InitializeV8Platform(per_process_opts->v8_thread_pool_size);
InitializeV8Platform(per_process::cli_options->v8_thread_pool_size);
V8::Initialize();
performance::performance_v8_start = PERFORMANCE_NOW();
v8_initialized = true;
per_process::v8_initialized = true;
const int exit_code =
Start(uv_default_loop(), args, exec_args);
v8_platform.StopTracingAgent();
v8_initialized = false;
per_process::v8_initialized = false;
V8::Dispose();

// uv_run cannot be called from the time before the beforeExit callback
Expand Down
6 changes: 3 additions & 3 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ namespace node {
namespace {

inline void* BufferMalloc(size_t length) {
return per_process_opts->zero_fill_all_buffers ?
node::UncheckedCalloc(length) :
node::UncheckedMalloc(length);
return per_process::cli_options->zero_fill_all_buffers
? node::UncheckedCalloc(length)
: node::UncheckedMalloc(length);
Copy link
Member

@bnoordhuis bnoordhuis Jan 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? / : on a new line is not a style we use anywhere else, I think. It always goes on the same line as the clause preceding it.

}

} // namespace
Expand Down
2 changes: 1 addition & 1 deletion src/node_config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static void Initialize(Local<Object> target,
#ifdef NODE_FIPS_MODE
READONLY_TRUE_PROPERTY(target, "fipsMode");
// TODO(addaleax): Use options parser variable instead.
if (per_process_opts->force_fips_crypto)
if (per_process::cli_options->force_fips_crypto)
READONLY_TRUE_PROPERTY(target, "fipsForced");
#endif

Expand Down
7 changes: 4 additions & 3 deletions src/node_constants.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1234,9 +1234,10 @@ void DefineCryptoConstants(Local<Object> target) {
NODE_DEFINE_STRING_CONSTANT(target,
"defaultCoreCipherList",
DEFAULT_CIPHER_LIST_CORE);
NODE_DEFINE_STRING_CONSTANT(target,
"defaultCipherList",
per_process_opts->tls_cipher_list.c_str());
NODE_DEFINE_STRING_CONSTANT(
target,
"defaultCipherList",
per_process::cli_options->tls_cipher_list.c_str());

NODE_DEFINE_CONSTANT(target, TLS1_VERSION);
NODE_DEFINE_CONSTANT(target, TLS1_1_VERSION);
Expand Down
2 changes: 1 addition & 1 deletion src/node_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ bool SafeGetenv(const char* key, std::string* text) {
#endif

{
Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
if (const char* value = getenv(key)) {
*text = value;
return true;
Expand Down
17 changes: 8 additions & 9 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,7 @@ static X509_STORE* NewRootCertStore() {
if (*system_cert_path != '\0') {
X509_STORE_load_locations(store, system_cert_path, nullptr);
}
if (per_process_opts->ssl_openssl_cert_store) {
if (per_process::cli_options->ssl_openssl_cert_store) {
X509_STORE_set_default_paths(store);
} else {
for (X509* cert : root_certs_vector) {
Expand Down Expand Up @@ -6207,16 +6207,15 @@ void InitCryptoOnce() {
OPENSSL_no_config();

// --openssl-config=...
if (!per_process_opts->openssl_config.empty()) {
if (!per_process::cli_options->openssl_config.empty()) {
OPENSSL_load_builtin_modules();
#ifndef OPENSSL_NO_ENGINE
ENGINE_load_builtin_engines();
#endif
ERR_clear_error();
CONF_modules_load_file(
per_process_opts->openssl_config.c_str(),
nullptr,
CONF_MFLAGS_DEFAULT_SECTION);
CONF_modules_load_file(per_process::cli_options->openssl_config.c_str(),
nullptr,
CONF_MFLAGS_DEFAULT_SECTION);
int err = ERR_get_error();
if (0 != err) {
fprintf(stderr,
Expand All @@ -6232,8 +6231,8 @@ void InitCryptoOnce() {
#ifdef NODE_FIPS_MODE
/* Override FIPS settings in cnf file, if needed. */
unsigned long err = 0; // NOLINT(runtime/int)
if (per_process_opts->enable_fips_crypto ||
per_process_opts->force_fips_crypto) {
if (per_process::cli_options->enable_fips_crypto ||
per_process::cli_options->force_fips_crypto) {
if (0 == FIPS_mode() && !FIPS_mode_set(1)) {
err = ERR_get_error();
}
Expand Down Expand Up @@ -6296,7 +6295,7 @@ void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
}

void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
CHECK(!per_process_opts->force_fips_crypto);
CHECK(!per_process::cli_options->force_fips_crypto);
Environment* env = Environment::GetCurrent(args);
const bool enabled = FIPS_mode();
bool enable = args[0]->BooleanValue(env->isolate());
Expand Down
14 changes: 9 additions & 5 deletions src/node_env_var.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,17 @@ using v8::PropertyCallbackInfo;
using v8::String;
using v8::Value;

namespace per_process {
Mutex env_var_mutex;
} // namespace per_process

static void EnvGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Isolate* isolate = info.GetIsolate();
if (property->IsSymbol()) {
return info.GetReturnValue().SetUndefined();
}
Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__
node::Utf8Value key(isolate, property);
const char* val = getenv(*key);
Expand Down Expand Up @@ -80,7 +84,7 @@ static void EnvSetter(Local<Name> property,
return;
}

Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__
node::Utf8Value key(info.GetIsolate(), property);
node::Utf8Value val(info.GetIsolate(), value);
Expand All @@ -100,7 +104,7 @@ static void EnvSetter(Local<Name> property,

static void EnvQuery(Local<Name> property,
const PropertyCallbackInfo<Integer>& info) {
Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
int32_t rc = -1; // Not found unless proven otherwise.
if (property->IsString()) {
#ifdef __POSIX__
Expand All @@ -127,7 +131,7 @@ static void EnvQuery(Local<Name> property,

static void EnvDeleter(Local<Name> property,
const PropertyCallbackInfo<Boolean>& info) {
Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
if (property->IsString()) {
#ifdef __POSIX__
node::Utf8Value key(info.GetIsolate(), property);
Expand All @@ -148,7 +152,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
Environment* env = Environment::GetCurrent(info);
Isolate* isolate = env->isolate();

Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
Local<Array> envarr;
int env_size = 0;
#ifdef __POSIX__
Expand Down
Loading