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

worker_threads: proper env and NODE_OPTIONS handling #31711

Closed
wants to merge 2 commits into from
Closed

worker_threads: proper env and NODE_OPTIONS handling #31711

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
0c87f8c
src: allow to reuse env options handling
lundibundi Feb 9, 2020
503003e
worker_threads: properly handle env and NODE_OPTIONS in workers
lundibundi Feb 9, 2020
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
4 changes: 2 additions & 2 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,8 +1358,8 @@ E('ERR_VM_MODULE_NOT_MODULE',
'Provided module is not an instance of Module', Error);
E('ERR_VM_MODULE_STATUS', 'Module status %s', Error);
E('ERR_WASI_ALREADY_STARTED', 'WASI instance has already started', Error);
E('ERR_WORKER_INVALID_EXEC_ARGV', (errors) =>
`Initiated Worker with invalid execArgv flags: ${errors.join(', ')}`,
E('ERR_WORKER_INVALID_EXEC_ARGV', (errors, msg = 'invalid execArgv flags') =>
`Initiated Worker with ${msg}: ${errors.join(', ')}`,
Error);
Copy link
Member

Choose a reason for hiding this comment

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

Tbh, I think this is a situation here we should just use ERR_WORKER_INVALID_EXEC_ARGV, as it is the same type of error that’s being reported

Copy link
Member Author

Choose a reason for hiding this comment

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

Gave it another thought, imo ERR_WORKER_INVALID_EXEC_ARGV is way too specific and may mislead people into thinking that the error is elsewhere. I think it would be better to add ERR_WORKER_INVALID_OPTIONS and group both ERR_WORKER_INVALID_EXEC_ARGV and ERR_WORKER_INVALID_NODE_OPTIONS under it, though that may be breaking. Or just add and use ERR_WORKER_INVALID_OPTIONS here and then open a semver-major to change that in for ERR_WORKER_INVALID_EXEC_ARGV. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

That’s a general problem with our error codes – they tend to be way too specific.

I’d be good with changing the code, but yes, that would be breaking and I’d like to avoid it in this PR.

I’d still be good with using ERR_WORKER_INVALID_EXEC_ARGV and then pointing out in the error message where the error came from (and, ideally, also in a property on the error object). We can always switch that to a different error code later.

(Fwiw, ERR_WORKER_INVALID_OPTIONS sounds like it refers to the options object passed to the Worker constructor…)

Copy link
Member Author

Choose a reason for hiding this comment

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

(Fwiw, ERR_WORKER_INVALID_OPTIONS sounds like it refers to the options object passed to the Worker constructor…)

That was the idea - part of the options passed is incorrect. tbh we can probably use ERR_INVALID_OPT_VALUE from #31251 😄.

Then I'll use the ERR_WORKER_INVALID_EXEC_ARGV in this PR and then open another for the error change.

E('ERR_WORKER_NOT_RUNNING', 'Worker instance not running', Error);
E('ERR_WORKER_OUT_OF_MEMORY', 'Worker terminated due to reaching memory limit',
Expand Down
13 changes: 6 additions & 7 deletions lib/internal/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,16 @@ class Worker extends EventEmitter {

const url = options.eval ? null : pathToFileURL(filename);
// Set up the C++ handle for the worker, as well as some internal wiring.
this[kHandle] = new WorkerImpl(url, options.execArgv,
this[kHandle] = new WorkerImpl(url,
env === process.env ? null : env,
options.execArgv,
parseResourceLimits(options.resourceLimits));
if (this[kHandle].invalidExecArgv) {
throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
}
if (env === process.env) {
// This may be faster than manually cloning the object in C++, especially
// when recursively spawning Workers.
this[kHandle].cloneParentEnvVars();
} else if (env !== undefined) {
this[kHandle].setEnvVars(env);
if (this[kHandle].invalidNodeOptions) {
throw new ERR_WORKER_INVALID_EXEC_ARGV(
this[kHandle].invalidNodeOptions, 'invalid NODE_OPTIONS env variable');
}
this[kHandle].onexit = (code, customErr) => this[kOnExit](code, customErr);
this[kPort] = this[kHandle].messagePort;
Expand Down
73 changes: 6 additions & 67 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,80 +790,19 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
#endif

std::shared_ptr<EnvironmentOptions> default_env_options =
per_process::cli_options->per_isolate->per_env;
{
std::string text;
default_env_options->pending_deprecation =
credentials::SafeGetenv("NODE_PENDING_DEPRECATION", &text) &&
text[0] == '1';
}

// Allow for environment set preserving symlinks.
{
std::string text;
default_env_options->preserve_symlinks =
credentials::SafeGetenv("NODE_PRESERVE_SYMLINKS", &text) &&
text[0] == '1';
}

{
std::string text;
default_env_options->preserve_symlinks_main =
credentials::SafeGetenv("NODE_PRESERVE_SYMLINKS_MAIN", &text) &&
text[0] == '1';
}

if (default_env_options->redirect_warnings.empty()) {
credentials::SafeGetenv("NODE_REDIRECT_WARNINGS",
&default_env_options->redirect_warnings);
}
HandleEnvOptions(per_process::cli_options->per_isolate->per_env);

#if !defined(NODE_WITHOUT_NODE_OPTIONS)
std::string node_options;

if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) {
std::vector<std::string> env_argv;
// [0] is expected to be the program name, fill it in from the real argv.
env_argv.push_back(argv->at(0));

bool is_in_string = false;
bool will_start_new_arg = true;
for (std::string::size_type index = 0;
index < node_options.size();
++index) {
char c = node_options.at(index);

// Backslashes escape the following character
if (c == '\\' && is_in_string) {
if (index + 1 == node_options.size()) {
errors->push_back("invalid value for NODE_OPTIONS "
"(invalid escape)\n");
return 9;
} else {
c = node_options.at(++index);
}
} else if (c == ' ' && !is_in_string) {
will_start_new_arg = true;
continue;
} else if (c == '"') {
is_in_string = !is_in_string;
continue;
}
std::vector<std::string> env_argv =
ParseNodeOptionsEnvVar(node_options, errors);

if (will_start_new_arg) {
env_argv.emplace_back(std::string(1, c));
will_start_new_arg = false;
} else {
env_argv.back() += c;
}
}
if (!errors->empty()) return 9;

if (is_in_string) {
errors->push_back("invalid value for NODE_OPTIONS "
"(unterminated string)\n");
return 9;
}
// [0] is expected to be the program name, fill it in from the real argv.
env_argv.insert(env_argv.begin(), argv->at(0));

const int exit_code = ProcessGlobalArgs(&env_argv,
nullptr,
Expand Down
2 changes: 1 addition & 1 deletion src/node_options-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <cstdlib>
#include "node_options.h"
#include "util.h"
#include <cstdlib>

namespace node {

Expand Down
63 changes: 63 additions & 0 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "env-inl.h"
#include "node_binding.h"
#include "node_internals.h"

#include <errno.h>
#include <sstream>
Expand Down Expand Up @@ -1009,6 +1010,68 @@ void Initialize(Local<Object> target,
}

} // namespace options_parser

void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options) {
HandleEnvOptions(env_options, [](const char* name) {
std::string text;
return credentials::SafeGetenv(name, &text) ? text : "";
});
}

void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options,
std::function<std::string(const char*)> opt_getter) {
lundibundi marked this conversation as resolved.
Show resolved Hide resolved
env_options->pending_deprecation =
opt_getter("NODE_PENDING_DEPRECATION") == "1";

env_options->preserve_symlinks = opt_getter("NODE_PRESERVE_SYMLINKS") == "1";

env_options->preserve_symlinks_main =
opt_getter("NODE_PRESERVE_SYMLINKS_MAIN") == "1";

if (env_options->redirect_warnings.empty())
env_options->redirect_warnings = opt_getter("NODE_REDIRECT_WARNINGS");
}

std::vector<std::string> ParseNodeOptionsEnvVar(
const std::string& node_options, std::vector<std::string>* errors) {
std::vector<std::string> env_argv;

bool is_in_string = false;
bool will_start_new_arg = true;
for (std::string::size_type index = 0; index < node_options.size(); ++index) {
char c = node_options.at(index);

// Backslashes escape the following character
if (c == '\\' && is_in_string) {
if (index + 1 == node_options.size()) {
errors->push_back("invalid value for NODE_OPTIONS "
"(invalid escape)\n");
return env_argv;
} else {
c = node_options.at(++index);
}
} else if (c == ' ' && !is_in_string) {
will_start_new_arg = true;
continue;
} else if (c == '"') {
is_in_string = !is_in_string;
continue;
}

if (will_start_new_arg) {
env_argv.emplace_back(std::string(1, c));
will_start_new_arg = false;
} else {
env_argv.back() += c;
}
}

if (is_in_string) {
errors->push_back("invalid value for NODE_OPTIONS "
"(unterminated string)\n");
}
return env_argv;
}
} // namespace node

NODE_MODULE_CONTEXT_AWARE_INTERNAL(options, node::options_parser::Initialize)
7 changes: 7 additions & 0 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,13 @@ extern Mutex cli_options_mutex;
extern std::shared_ptr<PerProcessOptions> cli_options;

} // namespace per_process

void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options);
void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options,
std::function<std::string(const char*)> opt_getter);

std::vector<std::string> ParseNodeOptionsEnvVar(
const std::string& node_options, std::vector<std::string>* errors);
} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
Loading