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

src: refactor --trace-env to reuse option selection and handling #56293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+88 −79
Open

src: refactor --trace-env to reuse option selection and handling #56293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 1 addition & 9 deletions src/node_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,7 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) {
*text = value.value();
}

auto options =
(env != nullptr ? env->options()
: per_process::cli_options->per_isolate->per_env);

if (options->trace_env) {
fprintf(stderr, "[--trace-env] get environment variable \"%s\"\n", key);

PrintTraceEnvStack(options);
}
TraceEnv(env, "get", key);

return has_env;
}
Expand Down
106 changes: 61 additions & 45 deletions src/node_env_var.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,19 +337,69 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
return JustVoid();
}

void PrintTraceEnvStack(Environment* env) {
PrintTraceEnvStack(env->options());
}
struct TraceEnvOptions {
bool print_message : 1 = 0;
bool print_js_stack : 1 = 0;
bool print_native_stack : 1 = 0;
};

void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options) {
if (options->trace_env_native_stack) {
template <typename... Args>
inline void TraceEnvImpl(Environment* env,
TraceEnvOptions options,
const char* format,
Args&&... args) {
if (options.print_message) {
fprintf(stderr, format, std::forward<Args>(args)...);
}
if (options.print_native_stack) {
DumpNativeBacktrace(stderr);
}
if (options->trace_env_js_stack) {
if (options.print_js_stack) {
DumpJavaScriptBacktrace(stderr);
}
}

TraceEnvOptions GetTraceEnvOptions(Environment* env) {
TraceEnvOptions options;
auto cli_options = env != nullptr
? env->options()
: per_process::cli_options->per_isolate->per_env;
if (cli_options->trace_env) {
options.print_message = 1;
};
if (cli_options->trace_env_js_stack) {
options.print_js_stack = 1;
};
if (cli_options->trace_env_native_stack) {
options.print_native_stack = 1;
};
return options;
}

void TraceEnv(Environment* env, const char* message) {
TraceEnvImpl(env, GetTraceEnvOptions(env), "[--trace-env] %s\n", message);
}

void TraceEnv(Environment* env, const char* message, const char* key) {
TraceEnvImpl(
env, GetTraceEnvOptions(env), "[--trace-env] %s \"%s\"\n", message, key);
}

void TraceEnv(Environment* env,
const char* message,
v8::Local<v8::String> key) {
TraceEnvOptions options = GetTraceEnvOptions(env);
if (options.print_message) {
Utf8Value key_utf8(env->isolate(), key);
TraceEnvImpl(env,
options,
"[--trace-env] %s \"%.*s\"\n",
message,
static_cast<int>(key_utf8.length()),
key_utf8.out());
}
}

static Intercepted EnvGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
Expand All @@ -363,14 +413,7 @@ static Intercepted EnvGetter(Local<Name> property,
env->env_vars()->Get(env->isolate(), property.As<String>());

bool has_env = !value_string.IsEmpty();
if (env->options()->trace_env) {
Utf8Value key(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] get environment variable \"%.*s\"\n",
static_cast<int>(key.length()),
key.out());
PrintTraceEnvStack(env);
}
TraceEnv(env, "get", property.As<String>());

if (has_env) {
info.GetReturnValue().Set(value_string.ToLocalChecked());
Expand Down Expand Up @@ -410,14 +453,7 @@ static Intercepted EnvSetter(Local<Name> property,
}

env->env_vars()->Set(env->isolate(), key, value_string);
if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), key);
fprintf(stderr,
"[--trace-env] set environment variable \"%.*s\"\n",
static_cast<int>(key_utf8.length()),
key_utf8.out());
PrintTraceEnvStack(env);
}
TraceEnv(env, "set", key);

return Intercepted::kYes;
}
Expand All @@ -429,16 +465,7 @@ static Intercepted EnvQuery(Local<Name> property,
if (property->IsString()) {
int32_t rc = env->env_vars()->Query(env->isolate(), property.As<String>());
bool has_env = (rc != -1);

if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] query environment variable \"%.*s\": %s\n",
static_cast<int>(key_utf8.length()),
key_utf8.out(),
has_env ? "is set" : "is not set");
PrintTraceEnvStack(env);
}
TraceEnv(env, "query", property.As<String>());
if (has_env) {
// Return attributes for the property.
info.GetReturnValue().Set(v8::None);
Expand All @@ -455,14 +482,7 @@ static Intercepted EnvDeleter(Local<Name> property,
if (property->IsString()) {
env->env_vars()->Delete(env->isolate(), property.As<String>());

if (env->options()->trace_env) {
Utf8Value key_utf8(env->isolate(), property.As<String>());
fprintf(stderr,
"[--trace-env] delete environment variable \"%.*s\"\n",
static_cast<int>(key_utf8.length()),
key_utf8.out());
PrintTraceEnvStack(env);
}
TraceEnv(env, "delete", property.As<String>());
}

// process.env never has non-configurable properties, so always
Expand All @@ -475,11 +495,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());

if (env->options()->trace_env) {
fprintf(stderr, "[--trace-env] enumerate environment variables\n");

PrintTraceEnvStack(env);
}
TraceEnv(env, "enumerate environment variables");

info.GetReturnValue().Set(
env->env_vars()->Enumerate(env->isolate()));
Expand Down
5 changes: 3 additions & 2 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,9 @@ namespace credentials {
bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr);
} // namespace credentials

void PrintTraceEnvStack(Environment* env);
void PrintTraceEnvStack(std::shared_ptr<EnvironmentOptions> options);
void TraceEnv(Environment* env, const char* message);
void TraceEnv(Environment* env, const char* message, const char* key);
void TraceEnv(Environment* env, const char* message, v8::Local<v8::String> key);

void DefineZlibConstants(v8::Local<v8::Object> target);
v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,
Expand Down
46 changes: 23 additions & 23 deletions test/parallel/test-trace-env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,33 @@ const fixtures = require('../common/fixtures');
// Check reads from the Node.js internals.
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('empty.js')], {
stderr(output) {
// This is a non-exhaustive list of the environment variables checked
// This is a non-exhaustive list of thes checked
Copy link
Member

Choose a reason for hiding this comment

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

thes?

probably

Suggested change
// This is a non-exhaustive list of thes checked
// This is a non-exhaustive list of the environment variables checked

// during startup of an empty script at the time this test was written.
// If the internals remove any one of them, the checks here can be updated
// accordingly.
if (common.hasIntl) {
assert.match(output, /get environment variable "NODE_ICU_DATA"/);
assert.match(output, /get "NODE_ICU_DATA"/);
}
if (common.hasCrypto) {
assert.match(output, /get environment variable "NODE_EXTRA_CA_CERTS"/);
assert.match(output, /get "NODE_EXTRA_CA_CERTS"/);
}
if (common.hasOpenSSL3) {
assert.match(output, /get environment variable "OPENSSL_CONF"/);
assert.match(output, /get "OPENSSL_CONF"/);
}
assert.match(output, /get environment variable "NODE_DEBUG_NATIVE"/);
assert.match(output, /get environment variable "NODE_COMPILE_CACHE"/);
assert.match(output, /get environment variable "NODE_NO_WARNINGS"/);
assert.match(output, /get environment variable "NODE_V8_COVERAGE"/);
assert.match(output, /get environment variable "NODE_DEBUG"/);
assert.match(output, /get environment variable "NODE_CHANNEL_FD"/);
assert.match(output, /get environment variable "NODE_UNIQUE_ID"/);
assert.match(output, /get "NODE_DEBUG_NATIVE"/);
assert.match(output, /get "NODE_COMPILE_CACHE"/);
assert.match(output, /get "NODE_NO_WARNINGS"/);
assert.match(output, /get "NODE_V8_COVERAGE"/);
assert.match(output, /get "NODE_DEBUG"/);
assert.match(output, /get "NODE_CHANNEL_FD"/);
assert.match(output, /get "NODE_UNIQUE_ID"/);
if (common.isWindows) {
assert.match(output, /get environment variable "USERPROFILE"/);
assert.match(output, /get "USERPROFILE"/);
} else {
assert.match(output, /get environment variable "HOME"/);
assert.match(output, /get "HOME"/);
}
assert.match(output, /get environment variable "NODE_PATH"/);
assert.match(output, /get environment variable "WATCH_REPORT_DEPENDENCIES"/);
assert.match(output, /get "NODE_PATH"/);
assert.match(output, /get "WATCH_REPORT_DEPENDENCIES"/);
}
});

Expand All @@ -48,22 +48,22 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
}
}, {
stderr(output) {
assert.match(output, /get environment variable "FOO"/);
assert.match(output, /get environment variable "BAR"/);
assert.match(output, /get "FOO"/);
assert.match(output, /get "BAR"/);
}
});

// Check set from user land.
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'set.js') ], {
stderr(output) {
assert.match(output, /set environment variable "FOO"/);
assert.match(output, /set "FOO"/);
}
});

// Check define from user land.
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'define.js') ], {
stderr(output) {
assert.match(output, /set environment variable "FOO"/);
assert.match(output, /set "FOO"/);
}
});

Expand All @@ -77,16 +77,16 @@ spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env'
}
}, {
stderr(output) {
assert.match(output, /query environment variable "FOO": is set/);
assert.match(output, /query environment variable "BAR": is not set/);
assert.match(output, /query environment variable "BAZ": is not set/);
assert.match(output, /query "FOO"/);
assert.match(output, /query "BAR"/);
assert.match(output, /query "BAZ"/);
}
});

// Check delete from user land.
spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('process-env', 'delete.js') ], {
stderr(output) {
assert.match(output, /delete environment variable "FOO"/);
assert.match(output, /delete "FOO"/);
}
});

Expand Down
Loading