From 01cd21973b26a2cbacbe143c5983cb4adf8e7681 Mon Sep 17 00:00:00 2001
From: Yael Hermon <yaelhe@wix.com>
Date: Fri, 4 Jan 2019 20:02:25 +0200
Subject: [PATCH] worker: enable passing command line flags

This PR adds the ability to provide Workers with their own
execArgv flags in replacement of the main thread's execArgv. Only
per-Isolate/per-Environment options are allowed. Per-Process options
and V8 flags are not allowed. Passing an empty execArgv array will
reset per-Isolate and per-Environment options of the Worker to their
defaults. If execArgv option is not passed, the Worker will get
the same flags as the main thread.

Usage example:
```
const worker = new Worker(__filename, {
    execArgv: ['--trace-warnings'],
});
```

PR-URL: https://github.com/nodejs/node/pull/25467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
---
 doc/api/errors.md                             |  6 ++
 doc/api/worker_threads.md                     |  9 ++-
 lib/internal/errors.js                        |  3 +
 lib/internal/worker.js                        | 13 +++-
 src/env-inl.h                                 |  5 ++
 src/env.h                                     |  1 +
 src/node_worker.cc                            | 67 ++++++++++++++++++-
 src/node_worker.h                             |  5 +-
 test/parallel/test-internal-errors.js         | 10 +++
 test/parallel/test-worker-execargv-invalid.js | 35 ++++++++++
 test/parallel/test-worker-execargv.js         | 22 ++++++
 11 files changed, 167 insertions(+), 9 deletions(-)
 create mode 100644 test/parallel/test-worker-execargv-invalid.js
 create mode 100644 test/parallel/test-worker-execargv.js

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 840ba9b85c12c2..96ba39dd486a29 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1909,6 +1909,12 @@ The fulfilled value of a linking promise is not a `vm.SourceTextModule` object.
 The current module's status does not allow for this operation. The specific
 meaning of the error depends on the specific function.
 
+<a id="ERR_WORKER_INVALID_EXEC_ARGV"></a>
+### ERR_WORKER_INVALID_EXEC_ARGV
+
+The `execArgv` option passed to the `Worker` constructor contains
+invalid flags.
+
 <a id="ERR_WORKER_PATH"></a>
 ### ERR_WORKER_PATH
 
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 78f35412c32a65..05cafa9cb178e7 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -316,13 +316,16 @@ if (isMainThread) {
     occur as described in the [HTML structured clone algorithm][], and an error
     will be thrown if the object cannot be cloned (e.g. because it contains
     `function`s).
-  * stdin {boolean} If this is set to `true`, then `worker.stdin` will
+  * `stdin` {boolean} If this is set to `true`, then `worker.stdin` will
     provide a writable stream whose contents will appear as `process.stdin`
     inside the Worker. By default, no data is provided.
-  * stdout {boolean} If this is set to `true`, then `worker.stdout` will
+  * `stdout` {boolean} If this is set to `true`, then `worker.stdout` will
     not automatically be piped through to `process.stdout` in the parent.
-  * stderr {boolean} If this is set to `true`, then `worker.stderr` will
+  * `stderr` {boolean} If this is set to `true`, then `worker.stderr` will
     not automatically be piped through to `process.stderr` in the parent.
+  * `execArgv` {string[]} List of node CLI options passed to the worker.
+    V8 options (such as `--max-old-space-size`) and options that affect the
+    process (such as `--title`) are not supported.
 
 ### Event: 'error'
 <!-- YAML
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 12b01ffb9b64f3..b33cd2710960b6 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -992,6 +992,9 @@ E('ERR_VM_MODULE_NOT_LINKED',
 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_WORKER_INVALID_EXEC_ARGV', (errors) =>
+  `Initiated Worker with invalid execArgv flags: ${errors.join(', ')}`,
+  Error);
 E('ERR_WORKER_PATH',
   'The worker script filename must be an absolute path or a relative ' +
   'path starting with \'./\' or \'../\'. Received "%s"',
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index 2e1568a73822b9..e20a37af8a7d6b 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -8,6 +8,8 @@ const {
   ERR_WORKER_PATH,
   ERR_WORKER_UNSERIALIZABLE_ERROR,
   ERR_WORKER_UNSUPPORTED_EXTENSION,
+  ERR_WORKER_INVALID_EXEC_ARGV,
+  ERR_INVALID_ARG_TYPE,
 } = require('internal/errors').codes;
 const { validateString } = require('internal/validators');
 
@@ -49,7 +51,11 @@ class Worker extends EventEmitter {
     super();
     debug(`[${threadId}] create new worker`, filename, options);
     validateString(filename, 'filename');
-
+    if (options.execArgv && !Array.isArray(options.execArgv)) {
+      throw new ERR_INVALID_ARG_TYPE('options.execArgv',
+                                     'array',
+                                     options.execArgv);
+    }
     if (!options.eval) {
       if (!path.isAbsolute(filename) &&
           !filename.startsWith('./') &&
@@ -68,7 +74,10 @@ 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);
+    this[kHandle] = new WorkerImpl(url, options.execArgv);
+    if (this[kHandle].invalidExecArgv) {
+      throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
+    }
     this[kHandle].onexit = (code) => this[kOnExit](code);
     this[kPort] = this[kHandle].messagePort;
     this[kPort].on('message', (data) => this[kOnMessage](data));
diff --git a/src/env-inl.h b/src/env-inl.h
index 7c643539ad94ac..d5bf6bc086ab77 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -595,6 +595,11 @@ inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
   return options_;
 }
 
+inline void IsolateData::set_options(
+    std::shared_ptr<PerIsolateOptions> options) {
+  options_ = options;
+}
+
 void Environment::CreateImmediate(native_immediate_callback cb,
                                   void* data,
                                   v8::Local<v8::Object> obj,
diff --git a/src/env.h b/src/env.h
index fdd5c2382cb7c1..711d07963a5f10 100644
--- a/src/env.h
+++ b/src/env.h
@@ -394,6 +394,7 @@ class IsolateData {
   inline uint32_t* zero_fill_field() const;
   inline MultiIsolatePlatform* platform() const;
   inline std::shared_ptr<PerIsolateOptions> options();
+  inline void set_options(std::shared_ptr<PerIsolateOptions> options);
 
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
 #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
diff --git a/src/node_worker.cc b/src/node_worker.cc
index dab7945aae3551..30004957a47442 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -10,7 +10,9 @@
 #include "async_wrap-inl.h"
 
 #include <string>
+#include <vector>
 
+using node::options_parser::kDisallowedInEnvironment;
 using v8::ArrayBuffer;
 using v8::Context;
 using v8::Function;
@@ -68,7 +70,10 @@ void WaitForWorkerInspectorToStop(Environment* child) {}
 
 }  // anonymous namespace
 
-Worker::Worker(Environment* env, Local<Object> wrap, const std::string& url)
+Worker::Worker(Environment* env,
+               Local<Object> wrap,
+               const std::string& url,
+               std::shared_ptr<PerIsolateOptions> per_isolate_opts)
     : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url) {
   // Generate a new thread id.
   {
@@ -113,6 +118,9 @@ Worker::Worker(Environment* env, Local<Object> wrap, const std::string& url)
                                           &loop_,
                                           env->isolate_data()->platform(),
                                           array_buffer_allocator_.get()));
+    if (per_isolate_opts != nullptr) {
+      isolate_data_->set_options(per_isolate_opts);
+    }
     CHECK(isolate_data_);
 
     Local<Context> context = NewContext(isolate_);
@@ -391,14 +399,67 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
   }
 
   std::string url;
+  std::shared_ptr<PerIsolateOptions> per_isolate_opts = nullptr;
+
   // Argument might be a string or URL
-  if (args.Length() == 1 && !args[0]->IsNullOrUndefined()) {
+  if (args.Length() > 0 && !args[0]->IsNullOrUndefined()) {
     Utf8Value value(
         args.GetIsolate(),
         args[0]->ToString(env->context()).FromMaybe(v8::Local<v8::String>()));
     url.append(value.out(), value.length());
+
+    if (args.Length() > 1 && args[1]->IsArray()) {
+      v8::Local<v8::Array> array = args[1].As<v8::Array>();
+      // The first argument is reserved for program name, but we don't need it
+      // in workers.
+      std::vector<std::string> exec_argv = {""};
+      uint32_t length = array->Length();
+      for (uint32_t i = 0; i < length; i++) {
+        v8::Local<v8::Value> arg;
+        if (!array->Get(env->context(), i).ToLocal(&arg)) {
+          return;
+        }
+        v8::MaybeLocal<v8::String> arg_v8_string =
+            arg->ToString(env->context());
+        if (arg_v8_string.IsEmpty()) {
+          return;
+        }
+        Utf8Value arg_utf8_value(
+            args.GetIsolate(),
+            arg_v8_string.FromMaybe(v8::Local<v8::String>()));
+        std::string arg_string(arg_utf8_value.out(), arg_utf8_value.length());
+        exec_argv.push_back(arg_string);
+      }
+
+      std::vector<std::string> invalid_args{};
+      std::vector<std::string> errors{};
+      per_isolate_opts.reset(new PerIsolateOptions());
+
+      // Using invalid_args as the v8_args argument as it stores unknown
+      // options for the per isolate parser.
+      options_parser::PerIsolateOptionsParser::instance.Parse(
+          &exec_argv,
+          nullptr,
+          &invalid_args,
+          per_isolate_opts.get(),
+          kDisallowedInEnvironment,
+          &errors);
+
+      // The first argument is program name.
+      invalid_args.erase(invalid_args.begin());
+      if (errors.size() > 0 || invalid_args.size() > 0) {
+        v8::Local<v8::Value> value =
+            ToV8Value(env->context(),
+                      errors.size() > 0 ? errors : invalid_args)
+                .ToLocalChecked();
+        Local<String> key =
+            FIXED_ONE_BYTE_STRING(env->isolate(), "invalidExecArgv");
+        args.This()->Set(env->context(), key, value).FromJust();
+        return;
+      }
+    }
   }
-  new Worker(env, args.This(), url);
+  new Worker(env, args.This(), url, per_isolate_opts);
 }
 
 void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_worker.h b/src/node_worker.h
index cbd4a861570b27..dd054ac38d5f76 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -12,7 +12,10 @@ namespace worker {
 // A worker thread, as represented in its parent thread.
 class Worker : public AsyncWrap {
  public:
-  Worker(Environment* env, v8::Local<v8::Object> wrap, const std::string& url);
+  Worker(Environment* env,
+         v8::Local<v8::Object> wrap,
+         const std::string& url,
+         std::shared_ptr<PerIsolateOptions> per_isolate_opts);
   ~Worker();
 
   // Run the worker. This is only called from the worker thread.
diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js
index bf4688d89bd0ca..e53a9a9fe42ca8 100644
--- a/test/parallel/test-internal-errors.js
+++ b/test/parallel/test-internal-errors.js
@@ -269,3 +269,13 @@ assert.strictEqual(
 
   restoreStdout();
 }
+
+{
+  const error = new errors.codes.ERR_WORKER_INVALID_EXEC_ARGV(
+    ['--foo, --bar']
+  );
+  assert.strictEqual(
+    error.message,
+    'Initiated Worker with invalid execArgv flags: --foo, --bar'
+  );
+}
diff --git a/test/parallel/test-worker-execargv-invalid.js b/test/parallel/test-worker-execargv-invalid.js
new file mode 100644
index 00000000000000..e2deaf5464113a
--- /dev/null
+++ b/test/parallel/test-worker-execargv-invalid.js
@@ -0,0 +1,35 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+{
+  const expectedErr = common.expectsError({
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError
+  }, 2);
+
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: 'hello' });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: 6 });
+  }, expectedErr);
+}
+
+{
+  const expectedErr = common.expectsError({
+    code: 'ERR_WORKER_INVALID_EXEC_ARGV',
+    type: Error
+  }, 3);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--foo'] });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--title=blah'] });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--redirect-warnings'] });
+  }, expectedErr);
+}
diff --git a/test/parallel/test-worker-execargv.js b/test/parallel/test-worker-execargv.js
new file mode 100644
index 00000000000000..e04f015e03e2ff
--- /dev/null
+++ b/test/parallel/test-worker-execargv.js
@@ -0,0 +1,22 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+// This test ensures that Workers have the ability to get
+// their own command line flags.
+
+const { Worker, isMainThread } = require('worker_threads');
+const { StringDecoder } = require('string_decoder');
+const decoder = new StringDecoder('utf8');
+
+if (isMainThread) {
+  const w = new Worker(__filename, { execArgv: ['--trace-warnings'] });
+  w.stderr.on('data', common.mustCall((chunk) => {
+    const error = decoder.write(chunk);
+    assert.ok(
+      /Warning: some warning[\s\S]*at Object\.<anonymous>/.test(error)
+    );
+  }));
+} else {
+  process.emitWarning('some warning');
+}