From e7b99e027b2fe2fc0834bf2df9daf6d94b27f02b Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Mon, 27 Apr 2020 03:41:56 +0200
Subject: [PATCH] worker: add stack size resource limit option

Add `stackSizeMb` to the `resourceLimit` option group.

Refs: https://github.com/nodejs/node/pull/31593#issuecomment-619633820

PR-URL: https://github.com/nodejs/node/pull/33085
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 doc/api/worker_threads.md                     |  4 ++
 lib/internal/worker.js                        |  6 ++-
 src/node_worker.cc                            | 20 ++++++--
 src/node_worker.h                             |  3 +-
 test/parallel/test-worker-resource-limits.js  |  1 +
 .../test-worker-stack-overflow-stack-size.js  | 47 +++++++++++++------
 6 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 4b6927bf63286d..b3f4aca313bfac 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -174,6 +174,7 @@ added:
   * `maxYoungGenerationSizeMb` {number}
   * `maxOldGenerationSizeMb` {number}
   * `codeRangeSizeMb` {number}
+  * `stackSizeMb` {number}
 
 Provides the set of JS engine resource constraints inside this Worker thread.
 If the `resourceLimits` option was passed to the [`Worker`][] constructor,
@@ -584,6 +585,8 @@ changes:
       recently created objects.
     * `codeRangeSizeMb` {number} The size of a pre-allocated memory range
       used for generated code.
+    * `stackSizeMb` {number} The default maximum stack size for the thread.
+      Small values may lead to unusable Worker instances. **Default:** `4`.
 
 ### Event: `'error'`
 <!-- YAML
@@ -679,6 +682,7 @@ added:
   * `maxYoungGenerationSizeMb` {number}
   * `maxOldGenerationSizeMb` {number}
   * `codeRangeSizeMb` {number}
+  * `stackSizeMb` {number}
 
 Provides the set of JS engine resource constraints for this Worker thread.
 If the `resourceLimits` option was passed to the [`Worker`][] constructor,
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index 5f92a7898e93a9..48886aee3fef5d 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -54,6 +54,7 @@ const {
   kMaxYoungGenerationSizeMb,
   kMaxOldGenerationSizeMb,
   kCodeRangeSizeMb,
+  kStackSizeMb,
   kTotalResourceLimitCount
 } = internalBinding('worker');
 
@@ -379,6 +380,8 @@ function parseResourceLimits(obj) {
     ret[kMaxYoungGenerationSizeMb] = obj.maxYoungGenerationSizeMb;
   if (typeof obj.codeRangeSizeMb === 'number')
     ret[kCodeRangeSizeMb] = obj.codeRangeSizeMb;
+  if (typeof obj.stackSizeMb === 'number')
+    ret[kStackSizeMb] = obj.stackSizeMb;
   return ret;
 }
 
@@ -386,7 +389,8 @@ function makeResourceLimits(float64arr) {
   return {
     maxYoungGenerationSizeMb: float64arr[kMaxYoungGenerationSizeMb],
     maxOldGenerationSizeMb: float64arr[kMaxOldGenerationSizeMb],
-    codeRangeSizeMb: float64arr[kCodeRangeSizeMb]
+    codeRangeSizeMb: float64arr[kCodeRangeSizeMb],
+    stackSizeMb: float64arr[kStackSizeMb]
   };
 }
 
diff --git a/src/node_worker.cc b/src/node_worker.cc
index 2043d84490b6aa..1e1d9434cddb4c 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -39,6 +39,8 @@ using v8::Value;
 namespace node {
 namespace worker {
 
+constexpr double kMB = 1024 * 1024;
+
 Worker::Worker(Environment* env,
                Local<Object> wrap,
                const std::string& url,
@@ -93,8 +95,6 @@ bool Worker::is_stopped() const {
 void Worker::UpdateResourceConstraints(ResourceConstraints* constraints) {
   constraints->set_stack_limit(reinterpret_cast<uint32_t*>(stack_base_));
 
-  constexpr double kMB = 1024 * 1024;
-
   if (resource_limits_[kMaxYoungGenerationSizeMb] > 0) {
     constraints->set_max_young_generation_size_in_bytes(
         resource_limits_[kMaxYoungGenerationSizeMb] * kMB);
@@ -589,9 +589,20 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
 
   w->stopped_ = false;
 
+  if (w->resource_limits_[kStackSizeMb] > 0) {
+    if (w->resource_limits_[kStackSizeMb] * kMB < kStackBufferSize) {
+      w->resource_limits_[kStackSizeMb] = kStackBufferSize / kMB;
+      w->stack_size_ = kStackBufferSize;
+    } else {
+      w->stack_size_ = w->resource_limits_[kStackSizeMb] * kMB;
+    }
+  } else {
+    w->resource_limits_[kStackSizeMb] = w->stack_size_ / kMB;
+  }
+
   uv_thread_options_t thread_options;
   thread_options.flags = UV_THREAD_HAS_STACK_SIZE;
-  thread_options.stack_size = kStackSize;
+  thread_options.stack_size = w->stack_size_;
   int ret = uv_thread_create_ex(&w->tid_, &thread_options, [](void* arg) {
     // XXX: This could become a std::unique_ptr, but that makes at least
     // gcc 6.3 detect undefined behaviour when there shouldn't be any.
@@ -601,7 +612,7 @@ void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
 
     // Leave a few kilobytes just to make sure we're within limits and have
     // some space to do work in C++ land.
-    w->stack_base_ = stack_top - (kStackSize - kStackBufferSize);
+    w->stack_base_ = stack_top - (w->stack_size_ - kStackBufferSize);
 
     w->Run();
 
@@ -834,6 +845,7 @@ void InitWorker(Local<Object> target,
   NODE_DEFINE_CONSTANT(target, kMaxYoungGenerationSizeMb);
   NODE_DEFINE_CONSTANT(target, kMaxOldGenerationSizeMb);
   NODE_DEFINE_CONSTANT(target, kCodeRangeSizeMb);
+  NODE_DEFINE_CONSTANT(target, kStackSizeMb);
   NODE_DEFINE_CONSTANT(target, kTotalResourceLimitCount);
 }
 
diff --git a/src/node_worker.h b/src/node_worker.h
index f9deaa59b4a8cc..1b9ba322bed04f 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -16,6 +16,7 @@ enum ResourceLimits {
   kMaxYoungGenerationSizeMb,
   kMaxOldGenerationSizeMb,
   kCodeRangeSizeMb,
+  kStackSizeMb,
   kTotalResourceLimitCount
 };
 
@@ -95,7 +96,7 @@ class Worker : public AsyncWrap {
   void UpdateResourceConstraints(v8::ResourceConstraints* constraints);
 
   // Full size of the thread's stack.
-  static constexpr size_t kStackSize = 4 * 1024 * 1024;
+  size_t stack_size_ = 4 * 1024 * 1024;
   // Stack buffer size that is not available to the JS engine.
   static constexpr size_t kStackBufferSize = 192 * 1024;
 
diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js
index 9332a132694e78..0c168bbea1a488 100644
--- a/test/parallel/test-worker-resource-limits.js
+++ b/test/parallel/test-worker-resource-limits.js
@@ -12,6 +12,7 @@ const testResourceLimits = {
   maxOldGenerationSizeMb: 16,
   maxYoungGenerationSizeMb: 4,
   codeRangeSizeMb: 16,
+  stackSizeMb: 1,
 };
 
 // Do not use isMainThread so that this test itself can be run inside a Worker.
diff --git a/test/parallel/test-worker-stack-overflow-stack-size.js b/test/parallel/test-worker-stack-overflow-stack-size.js
index f218554297d1a3..a079624847bcb7 100644
--- a/test/parallel/test-worker-stack-overflow-stack-size.js
+++ b/test/parallel/test-worker-stack-overflow-stack-size.js
@@ -8,7 +8,7 @@ const { Worker } = require('worker_threads');
 // Verify that Workers don't care about --stack-size, as they have their own
 // fixed and known stack sizes.
 
-async function runWorker() {
+async function runWorker(options = {}) {
   const empiricalStackDepth = new Uint32Array(new SharedArrayBuffer(4));
   const worker = new Worker(`
   const { workerData: { empiricalStackDepth } } = require('worker_threads');
@@ -18,26 +18,45 @@ async function runWorker() {
   }
   f();`, {
     eval: true,
-    workerData: { empiricalStackDepth }
+    workerData: { empiricalStackDepth },
+    ...options
   });
 
   const [ error ] = await once(worker, 'error');
 
-  common.expectsError({
-    constructor: RangeError,
-    message: 'Maximum call stack size exceeded'
-  })(error);
+  if (!options.skipErrorCheck) {
+    common.expectsError({
+      constructor: RangeError,
+      message: 'Maximum call stack size exceeded'
+    })(error);
+  }
 
   return empiricalStackDepth[0];
 }
 
 (async function() {
-  v8.setFlagsFromString('--stack-size=500');
-  const w1stack = await runWorker();
-  v8.setFlagsFromString('--stack-size=1000');
-  const w2stack = await runWorker();
-  // Make sure the two stack sizes are within 10 % of each other, i.e. not
-  // affected by the different `--stack-size` settings.
-  assert(Math.max(w1stack, w2stack) / Math.min(w1stack, w2stack) < 1.1,
-         `w1stack = ${w1stack}, w2stack ${w2stack} are too far apart`);
+  {
+    v8.setFlagsFromString('--stack-size=500');
+    const w1stack = await runWorker();
+    v8.setFlagsFromString('--stack-size=1000');
+    const w2stack = await runWorker();
+    // Make sure the two stack sizes are within 10 % of each other, i.e. not
+    // affected by the different `--stack-size` settings.
+    assert(Math.max(w1stack, w2stack) / Math.min(w1stack, w2stack) < 1.1,
+           `w1stack = ${w1stack}, w2stack = ${w2stack} are too far apart`);
+  }
+
+  {
+    const w1stack = await runWorker({ resourceLimits: { stackSizeMb: 0.5 } });
+    const w2stack = await runWorker({ resourceLimits: { stackSizeMb: 1.0 } });
+    // Make sure the two stack sizes are at least 40 % apart from each other,
+    // i.e. affected by the different `stackSizeMb` settings.
+    assert(w2stack > w1stack * 1.4,
+           `w1stack = ${w1stack}, w2stack = ${w2stack} are too close`);
+  }
+
+  // Test that various low stack sizes result in an 'error' event.
+  for (const stackSizeMb of [ 0.001, 0.01, 0.1, 0.2, 0.3, 0.5 ]) {
+    await runWorker({ resourceLimits: { stackSizeMb }, skipErrorCheck: true });
+  }
 })().then(common.mustCall());