From 3571383be4212cd3a64e8217ef27d92c546f84f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 20 Sep 2022 17:05:45 +0400 Subject: [PATCH 1/3] worker: inherits mutated NODE_OPTIONS --- lib/internal/worker.js | 2 +- test/parallel/test-worker-process-env-options.js | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-worker-process-env-options.js diff --git a/lib/internal/worker.js b/lib/internal/worker.js index d88170ab9cd9cf..f44f9a6fda0dd1 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -189,7 +189,7 @@ class Worker extends EventEmitter { // Set up the C++ handle for the worker, as well as some internal wiring. this[kHandle] = new WorkerImpl(url, - env === process.env ? null : env, + env, options.execArgv, parseResourceLimits(options.resourceLimits), !!(options.trackUnmanagedFds ?? true)); diff --git a/test/parallel/test-worker-process-env-options.js b/test/parallel/test-worker-process-env-options.js new file mode 100644 index 00000000000000..74287c26219c5e --- /dev/null +++ b/test/parallel/test-worker-process-env-options.js @@ -0,0 +1,14 @@ +'use strict'; +const { Worker } = require('node:worker_threads'); + +if (!require.main) { + globalThis.setup = true; +} else { + process.env.NODE_OPTIONS ??= ``; + process.env.NODE_OPTIONS += ` --require ${JSON.stringify(__filename)}`; + + new Worker(` + const assert = require('assert'); + assert.strictEqual(globalThis.setup, true); + `, { eval: true }); +} From e3695726ac3d3bef18f07b5c0a04fc8b40373d07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 20 Sep 2022 17:12:22 +0400 Subject: [PATCH 2/3] Lint --- test/parallel/test-worker-process-env-options.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-worker-process-env-options.js b/test/parallel/test-worker-process-env-options.js index 74287c26219c5e..a9ed7d02d281ce 100644 --- a/test/parallel/test-worker-process-env-options.js +++ b/test/parallel/test-worker-process-env-options.js @@ -1,10 +1,11 @@ 'use strict'; +require('../common'); const { Worker } = require('node:worker_threads'); if (!require.main) { globalThis.setup = true; } else { - process.env.NODE_OPTIONS ??= ``; + process.env.NODE_OPTIONS ??= ''; process.env.NODE_OPTIONS += ` --require ${JSON.stringify(__filename)}`; new Worker(` From 7749bf154e83ab56d50b65dcaecd4e1bd7a4eb4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 21 Sep 2022 20:07:43 +0400 Subject: [PATCH 3/3] Tries fixing the problem inside the C++ code --- lib/internal/worker.js | 2 +- src/node_worker.cc | 68 +++++++++---------- test/fixtures/assert-global.js | 3 + .../test-worker-process-env-options.js | 18 ++--- 4 files changed, 44 insertions(+), 47 deletions(-) create mode 100644 test/fixtures/assert-global.js diff --git a/lib/internal/worker.js b/lib/internal/worker.js index f44f9a6fda0dd1..d88170ab9cd9cf 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -189,7 +189,7 @@ class Worker extends EventEmitter { // Set up the C++ handle for the worker, as well as some internal wiring. this[kHandle] = new WorkerImpl(url, - env, + env === process.env ? null : env, options.execArgv, parseResourceLimits(options.resourceLimits), !!(options.trackUnmanagedFds ?? true)); diff --git a/src/node_worker.cc b/src/node_worker.cc index 9e2999ed8c3955..048f15aea1cbc1 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -485,46 +485,44 @@ void Worker::New(const FunctionCallbackInfo& args) { env_vars = env->env_vars(); } - if (args[1]->IsObject() || args[2]->IsArray()) { - per_isolate_opts.reset(new PerIsolateOptions()); + per_isolate_opts.reset(new PerIsolateOptions()); - HandleEnvOptions(per_isolate_opts->per_env, [&env_vars](const char* name) { - return env_vars->Get(name).FromMaybe(""); - }); + HandleEnvOptions(per_isolate_opts->per_env, [&env_vars](const char* name) { + return env_vars->Get(name).FromMaybe(""); + }); #ifndef NODE_WITHOUT_NODE_OPTIONS - MaybeLocal maybe_node_opts = - env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS")); - Local node_opts; - if (maybe_node_opts.ToLocal(&node_opts)) { - std::string node_options(*String::Utf8Value(isolate, node_opts)); - std::vector errors{}; - std::vector env_argv = - ParseNodeOptionsEnvVar(node_options, &errors); - // [0] is expected to be the program name, add dummy string. - env_argv.insert(env_argv.begin(), ""); - std::vector invalid_args{}; - options_parser::Parse(&env_argv, - nullptr, - &invalid_args, - per_isolate_opts.get(), - kAllowedInEnvironment, - &errors); - if (!errors.empty() && args[1]->IsObject()) { - // Only fail for explicitly provided env, this protects from failures - // when NODE_OPTIONS from parent's env is used (which is the default). - Local error; - if (!ToV8Value(env->context(), errors).ToLocal(&error)) return; - Local key = - FIXED_ONE_BYTE_STRING(env->isolate(), "invalidNodeOptions"); - // Ignore the return value of Set() because exceptions bubble up to JS - // when we return anyway. - USE(args.This()->Set(env->context(), key, error)); - return; - } + MaybeLocal maybe_node_opts = + env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS")); + Local node_opts; + if (maybe_node_opts.ToLocal(&node_opts)) { + std::string node_options(*String::Utf8Value(isolate, node_opts)); + std::vector errors{}; + std::vector env_argv = + ParseNodeOptionsEnvVar(node_options, &errors); + // [0] is expected to be the program name, add dummy string. + env_argv.insert(env_argv.begin(), ""); + std::vector invalid_args{}; + options_parser::Parse(&env_argv, + nullptr, + &invalid_args, + per_isolate_opts.get(), + kAllowedInEnvironment, + &errors); + if (!errors.empty() && args[1]->IsObject()) { + // Only fail for explicitly provided env, this protects from failures + // when NODE_OPTIONS from parent's env is used (which is the default). + Local error; + if (!ToV8Value(env->context(), errors).ToLocal(&error)) return; + Local key = + FIXED_ONE_BYTE_STRING(env->isolate(), "invalidNodeOptions"); + // Ignore the return value of Set() because exceptions bubble up to JS + // when we return anyway. + USE(args.This()->Set(env->context(), key, error)); + return; } -#endif // NODE_WITHOUT_NODE_OPTIONS } +#endif // NODE_WITHOUT_NODE_OPTIONS if (args[2]->IsArray()) { Local array = args[2].As(); diff --git a/test/fixtures/assert-global.js b/test/fixtures/assert-global.js new file mode 100644 index 00000000000000..40b3ab566fcec0 --- /dev/null +++ b/test/fixtures/assert-global.js @@ -0,0 +1,3 @@ +'use strict'; +const assert = require('assert'); +assert.strictEqual(global.a, 'test'); diff --git a/test/parallel/test-worker-process-env-options.js b/test/parallel/test-worker-process-env-options.js index a9ed7d02d281ce..0dba089d573977 100644 --- a/test/parallel/test-worker-process-env-options.js +++ b/test/parallel/test-worker-process-env-options.js @@ -1,15 +1,11 @@ 'use strict'; + require('../common'); + +const fixtures = require('../common/fixtures'); const { Worker } = require('node:worker_threads'); -if (!require.main) { - globalThis.setup = true; -} else { - process.env.NODE_OPTIONS ??= ''; - process.env.NODE_OPTIONS += ` --require ${JSON.stringify(__filename)}`; - - new Worker(` - const assert = require('assert'); - assert.strictEqual(globalThis.setup, true); - `, { eval: true }); -} +process.env.NODE_OPTIONS ??= ''; +process.env.NODE_OPTIONS += ` --require ${JSON.stringify(fixtures.path('define-global.js'))}`; + +new Worker(fixtures.path('assert-global.js'));