From c29e40f4af9fe5c6fd52ee6f1ce7a5bdfaafc775 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Fri, 9 Oct 2020 01:07:29 +0800
Subject: [PATCH 1/3] src: add --heapsnapshot-near-heap-limit option

This patch adds a --heapsnapshot-near-heap-limit CLI option
that takes heap snapshots when the V8 heap is approaching
the heap size limit. It will try to write the snapshots
to disk before the program crashes due to OOM.
---
 doc/api/cli.md                                |  48 ++++++
 doc/node.1                                    |   4 +
 src/debug_utils.h                             |   1 +
 src/env-inl.h                                 |  16 ++
 src/env.cc                                    | 146 ++++++++++++++++++
 src/env.h                                     |  10 ++
 src/heap_utils.cc                             |   6 +-
 src/inspector_profiler.cc                     |  20 +--
 src/node.cc                                   |   4 +
 src/node_internals.h                          |   4 +
 src/node_main_instance.cc                     |   2 +
 src/node_options.cc                           |  12 ++
 src/node_options.h                            |   1 +
 src/node_worker.cc                            |   5 +
 test/fixtures/workload/bounded.js             |  22 +++
 test/fixtures/workload/grow-worker.js         |  14 ++
 test/fixtures/workload/grow.js                |  12 ++
 ...st-heapsnapshot-near-heap-limit-bounded.js |  36 +++++
 .../test-heapsnapshot-near-heap-limit.js      | 106 +++++++++++++
 .../test-heapsnapshot-near-heap-limit-big.js  |  43 ++++++
 20 files changed, 491 insertions(+), 21 deletions(-)
 create mode 100644 test/fixtures/workload/bounded.js
 create mode 100644 test/fixtures/workload/grow-worker.js
 create mode 100644 test/fixtures/workload/grow.js
 create mode 100644 test/parallel/test-heapsnapshot-near-heap-limit-bounded.js
 create mode 100644 test/parallel/test-heapsnapshot-near-heap-limit.js
 create mode 100644 test/pummel/test-heapsnapshot-near-heap-limit-big.js

diff --git a/doc/api/cli.md b/doc/api/cli.md
index cb1d1ff2f43f34..27977f20ed0246 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -333,6 +333,53 @@ reference. Code may break under this flag.
 `--require` runs prior to freezing intrinsics in order to allow polyfills to
 be added.
 
+### `--heapsnapshot-near-heap-limit=max_count`
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+Writes a V8 heap snapshot to disk when the V8 heap usage is approaching the
+heap limit. `count` should be either -1 (in which case no snapshots would be
+written), or a positive integer (in which case Node.js will write no more than
+`max_count` snapshots to disk).
+
+When generating snapshots, garbage collection may be triggered and bring
+the heap usage down, therefore multiple snapshots may be written to disk
+before the Node.js instance finally runs out of memory. These heap snapshots
+can be compared to determine what objects are being allocated during the
+time consecutive snapshots are taken. It's not guaranteed that Node.js will
+write exactly `max_count` snapshots to disk, but it will try
+its best to generate at least one and up to `max_count` snapshots before the
+Node.js instance runs out of memory.
+
+Generating V8 snapshots takes time and memory (both memory managed by the
+V8 heap and native memory outside the V8 heap). The bigger the heap is,
+the more resources it needs. Node.js will adjust the V8 heap to accommondate
+the additional V8 heap memory overhead, and try its best to avoid using up
+all the memory avialable to the process. When the process uses
+more memory than the system deems appropriate, the process may be terminated
+abruptly by the system, depending on the system configuration.
+
+```console
+$ node --max-old-space-size=100 --heapsnapshot-near-heap-limit=3 index.js
+Wrote snapshot to Heap.20200430.100036.49580.0.001.heapsnapshot
+Wrote snapshot to Heap.20200430.100037.49580.0.002.heapsnapshot
+Wrote snapshot to Heap.20200430.100038.49580.0.003.heapsnapshot
+
+<--- Last few GCs --->
+
+[49580:0x110000000]     4826 ms: Mark-sweep 130.6 (147.8) -> 130.5 (147.8) MB, 27.4 / 0.0 ms  (average mu = 0.126, current mu = 0.034) allocation failure scavenge might not succeed
+[49580:0x110000000]     4845 ms: Mark-sweep 130.6 (147.8) -> 130.6 (147.8) MB, 18.8 / 0.0 ms  (average mu = 0.088, current mu = 0.031) allocation failure scavenge might not succeed
+
+
+<--- JS stacktrace --->
+
+FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
+....
+```
+
 ### `--heapsnapshot-signal=signal`
 <!-- YAML
 added: v12.0.0
@@ -1276,6 +1323,7 @@ Node.js options that are allowed are:
 * `--force-context-aware`
 * `--force-fips`
 * `--frozen-intrinsics`
+* `--heapsnapshot-near-heap-limit`
 * `--heapsnapshot-signal`
 * `--http-parser`
 * `--icu-data-dir`
diff --git a/doc/node.1 b/doc/node.1
index 63ee7391988e44..b2333c6548aed0 100644
--- a/doc/node.1
+++ b/doc/node.1
@@ -182,6 +182,10 @@ Same requirements as
 .It Fl -frozen-intrinsics
 Enable experimental frozen intrinsics support.
 .
+.It Fl -heapsnapshot-near-heap-limit Ns = Ns Ar max_count
+Generate heap snapshot when the V8 heap usage is approaching the heap limit.
+No more than the specified number of snapshots will be generated.
+.
 .It Fl -heapsnapshot-signal Ns = Ns Ar signal
 Generate heap snapshot on specified signal.
 .
diff --git a/src/debug_utils.h b/src/debug_utils.h
index 4915fbae325ecc..377493359e91e1 100644
--- a/src/debug_utils.h
+++ b/src/debug_utils.h
@@ -41,6 +41,7 @@ void FWrite(FILE* file, const std::string& str);
 // from a provider type to a debug category.
 #define DEBUG_CATEGORY_NAMES(V)                                                \
   NODE_ASYNC_PROVIDER_TYPES(V)                                                 \
+  V(DIAGNOSTICS)                                                               \
   V(HUGEPAGES)                                                                 \
   V(INSPECTOR_SERVER)                                                          \
   V(INSPECTOR_PROFILER)                                                        \
diff --git a/src/env-inl.h b/src/env-inl.h
index ec80faba4c49e2..7e7f4b3808a3f9 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -614,6 +614,22 @@ inline const std::string& Environment::exec_path() const {
   return exec_path_;
 }
 
+inline std::string Environment::GetCwd() {
+  char cwd[PATH_MAX_BYTES];
+  size_t size = PATH_MAX_BYTES;
+  const int err = uv_cwd(cwd, &size);
+
+  if (err == 0) {
+    CHECK_GT(size, 0);
+    return cwd;
+  }
+
+  // This can fail if the cwd is deleted. In that case, fall back to
+  // exec_path.
+  const std::string& exec_path = exec_path_;
+  return exec_path.substr(0, exec_path.find_last_of(kPathSeparator));
+}
+
 #if HAVE_INSPECTOR
 inline void Environment::set_coverage_directory(const char* dir) {
   coverage_directory_ = std::string(dir);
diff --git a/src/env.cc b/src/env.cc
index 4cdc4e3d1ba79e..ba9ac1e31ffe92 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -3,6 +3,7 @@
 #include "async_wrap.h"
 #include "base_object-inl.h"
 #include "debug_utils-inl.h"
+#include "diagnosticfilename-inl.h"
 #include "memory_tracker-inl.h"
 #include "node_buffer.h"
 #include "node_context_data.h"
@@ -24,6 +25,7 @@
 #include <cinttypes>
 #include <cstdio>
 #include <iostream>
+#include <limits>
 #include <memory>
 
 namespace node {
@@ -479,6 +481,11 @@ Environment::~Environment() {
   // FreeEnvironment() should have set this.
   CHECK(is_stopping());
 
+  if (options_->heap_snapshot_near_heap_limit > heap_limit_snapshot_taken_) {
+    isolate_->RemoveNearHeapLimitCallback(Environment::NearHeapLimitCallback,
+                                          0);
+  }
+
   isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback(
       BuildEmbedderGraph, this);
 
@@ -1402,6 +1409,25 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) {
   CHECK_EQ(ctx_from_snapshot, ctx);
 }
 
+uint64_t GuessMemoryAvailableToTheProcess() {
+  uint64_t free_in_system = uv_get_free_memory();
+  size_t allowed = uv_get_constrained_memory();
+  if (allowed == 0) {
+    return free_in_system;
+  }
+  size_t rss;
+  int err = uv_resident_set_memory(&rss);
+  if (err) {
+    return free_in_system;
+  }
+  if (allowed < rss) {
+    // Something is probably wrong. Fallback to the free memory.
+    return free_in_system;
+  }
+  // There may still be room for swap, but we will just leave it here.
+  return allowed - rss;
+}
+
 void Environment::BuildEmbedderGraph(Isolate* isolate,
                                      EmbedderGraph* graph,
                                      void* data) {
@@ -1414,6 +1440,126 @@ void Environment::BuildEmbedderGraph(Isolate* isolate,
   });
 }
 
+size_t Environment::NearHeapLimitCallback(void* data,
+                                          size_t current_heap_limit,
+                                          size_t initial_heap_limit) {
+  Environment* env = static_cast<Environment*>(data);
+
+  Debug(env,
+        DebugCategory::DIAGNOSTICS,
+        "Invoked NearHeapLimitCallback, processing=%d, "
+        "current_limit=%" PRIu64 ", "
+        "initial_limit=%" PRIu64 "\n",
+        env->is_processing_heap_limit_callback_,
+        static_cast<uint64_t>(current_heap_limit),
+        static_cast<uint64_t>(initial_heap_limit));
+
+  size_t max_young_gen_size = env->isolate_data()->max_young_gen_size;
+  size_t young_gen_size = 0;
+  size_t old_gen_size = 0;
+
+  v8::HeapSpaceStatistics stats;
+  size_t num_heap_spaces = env->isolate()->NumberOfHeapSpaces();
+  for (size_t i = 0; i < num_heap_spaces; ++i) {
+    env->isolate()->GetHeapSpaceStatistics(&stats, i);
+    if (strcmp(stats.space_name(), "new_space") == 0 ||
+        strcmp(stats.space_name(), "new_large_object_space") == 0) {
+      young_gen_size += stats.space_used_size();
+    } else {
+      old_gen_size += stats.space_used_size();
+    }
+  }
+
+  Debug(env,
+        DebugCategory::DIAGNOSTICS,
+        "max_young_gen_size=%" PRIu64 ", "
+        "young_gen_size=%" PRIu64 ", "
+        "old_gen_size=%" PRIu64 ", "
+        "total_size=%" PRIu64 "\n",
+        static_cast<uint64_t>(max_young_gen_size),
+        static_cast<uint64_t>(young_gen_size),
+        static_cast<uint64_t>(old_gen_size),
+        static_cast<uint64_t>(young_gen_size + old_gen_size));
+
+  uint64_t available = GuessMemoryAvailableToTheProcess();
+  // TODO(joyeecheung): get a better estimate about the native memory
+  // usage into the overhead, e.g. based on the count of objects.
+  uint64_t estimated_overhead = max_young_gen_size;
+  Debug(env,
+        DebugCategory::DIAGNOSTICS,
+        "Estimated available memory=%" PRIu64 ", "
+        "estimated overhead=%" PRIu64 "\n",
+        static_cast<uint64_t>(available),
+        static_cast<uint64_t>(estimated_overhead));
+
+  // This might be hit when the snapshot is being taken in another
+  // NearHeapLimitCallback invocation.
+  // When taking the snapshot, objects in the young generation may be
+  // promoted to the old generation, result in increased heap usage,
+  // but it should be no more than the young generation size.
+  // Ideally, this should be as small as possible - the heap limit
+  // can only be restored when the heap usage falls down below the
+  // new limit, so in a heap with unbounded growth the isolate
+  // may eventually crash with this new limit - effectively raising
+  // the heap limit to the new one.
+  if (env->is_processing_heap_limit_callback_) {
+    size_t new_limit = initial_heap_limit + max_young_gen_size;
+    Debug(env,
+          DebugCategory::DIAGNOSTICS,
+          "Not generating snapshots in nested callback. "
+          "new_limit=%" PRIu64 "\n",
+          static_cast<uint64_t>(new_limit));
+    return new_limit;
+  }
+
+  // Estimate whether the snapshot is going to use up all the memory
+  // available to the process. If so, just give up to prevent the system
+  // from killing the process for a system OOM.
+  if (estimated_overhead > available) {
+    Debug(env,
+          DebugCategory::DIAGNOSTICS,
+          "Not generating snapshots because it's too risky.\n");
+    env->isolate()->RemoveNearHeapLimitCallback(NearHeapLimitCallback,
+                                                initial_heap_limit);
+    return current_heap_limit;
+  }
+
+  // Take the snapshot synchronously.
+  env->is_processing_heap_limit_callback_ = true;
+
+  std::string dir = env->options()->diagnostic_dir;
+  if (dir.empty()) {
+    dir = env->GetCwd();
+  }
+  DiagnosticFilename name(env, "Heap", "heapsnapshot");
+  std::string filename = dir + kPathSeparator + (*name);
+
+  Debug(env, DebugCategory::DIAGNOSTICS, "Start generating %s...\n", *name);
+
+  // Remove the callback first in case it's triggered when generating
+  // the snapshot.
+  env->isolate()->RemoveNearHeapLimitCallback(NearHeapLimitCallback,
+                                              initial_heap_limit);
+
+  heap::WriteSnapshot(env->isolate(), filename.c_str());
+  env->heap_limit_snapshot_taken_ += 1;
+
+  // Don't take more snapshots than the number specified by
+  // --heapsnapshot-near-heap-limit.
+  if (env->heap_limit_snapshot_taken_ <
+      env->options_->heap_snapshot_near_heap_limit) {
+    env->isolate()->AddNearHeapLimitCallback(NearHeapLimitCallback, env);
+  }
+
+  FPrintF(stderr, "Wrote snapshot to %s\n", filename.c_str());
+  // Tell V8 to reset the heap limit once the heap usage falls down to
+  // 95% of the initial limit.
+  env->isolate()->AutomaticallyRestoreInitialHeapLimit(0.95);
+
+  env->is_processing_heap_limit_callback_ = false;
+  return initial_heap_limit;
+}
+
 inline size_t Environment::SelfSize() const {
   size_t size = sizeof(*this);
   // Remove non pointer fields that will be tracked in MemoryInfo()
diff --git a/src/env.h b/src/env.h
index db86bce8a1b6d8..5b3d51c572bb75 100644
--- a/src/env.h
+++ b/src/env.h
@@ -595,6 +595,7 @@ class IsolateData : public MemoryRetainer {
 #undef VP
   inline v8::Local<v8::String> async_wrap_provider(int index) const;
 
+  size_t max_young_gen_size = 1;
   std::unordered_map<const char*, v8::Eternal<v8::String>> static_str_map;
 
   inline v8::Isolate* isolate() const;
@@ -959,6 +960,9 @@ class Environment : public MemoryRetainer {
   void VerifyNoStrongBaseObjects();
   // Should be called before InitializeInspector()
   void InitializeDiagnostics();
+
+  std::string GetCwd();
+
 #if HAVE_INSPECTOR
   // If the environment is created for a worker, pass parent_handle and
   // the ownership if transferred into the Environment.
@@ -1317,6 +1321,9 @@ class Environment : public MemoryRetainer {
   inline void RemoveCleanupHook(void (*fn)(void*), void* arg);
   void RunCleanup();
 
+  static size_t NearHeapLimitCallback(void* data,
+                                      size_t current_heap_limit,
+                                      size_t initial_heap_limit);
   static void BuildEmbedderGraph(v8::Isolate* isolate,
                                  v8::EmbedderGraph* graph,
                                  void* data);
@@ -1435,6 +1442,9 @@ class Environment : public MemoryRetainer {
   std::vector<std::string> argv_;
   std::string exec_path_;
 
+  bool is_processing_heap_limit_callback_ = false;
+  int64_t heap_limit_snapshot_taken_ = 0;
+
   uint32_t module_id_counter_ = 0;
   uint32_t script_id_counter_ = 0;
   uint32_t function_id_counter_ = 0;
diff --git a/src/heap_utils.cc b/src/heap_utils.cc
index 449feb9e78d0e1..71bfd59ac3ea69 100644
--- a/src/heap_utils.cc
+++ b/src/heap_utils.cc
@@ -313,7 +313,9 @@ inline void TakeSnapshot(Isolate* isolate, v8::OutputStream* out) {
   snapshot->Serialize(out, HeapSnapshot::kJSON);
 }
 
-inline bool WriteSnapshot(Isolate* isolate, const char* filename) {
+}  // namespace
+
+bool WriteSnapshot(Isolate* isolate, const char* filename) {
   FILE* fp = fopen(filename, "w");
   if (fp == nullptr)
     return false;
@@ -323,8 +325,6 @@ inline bool WriteSnapshot(Isolate* isolate, const char* filename) {
   return true;
 }
 
-}  // namespace
-
 void DeleteHeapSnapshot(const HeapSnapshot* snapshot) {
   const_cast<HeapSnapshot*>(snapshot)->Delete();
 }
diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index 03cf2f6e5ca76b..9dd3e623ec861f 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -394,22 +394,6 @@ static void EndStartedProfilers(Environment* env) {
   }
 }
 
-std::string GetCwd(Environment* env) {
-  char cwd[PATH_MAX_BYTES];
-  size_t size = PATH_MAX_BYTES;
-  const int err = uv_cwd(cwd, &size);
-
-  if (err == 0) {
-    CHECK_GT(size, 0);
-    return cwd;
-  }
-
-  // This can fail if the cwd is deleted. In that case, fall back to
-  // exec_path.
-  const std::string& exec_path = env->exec_path();
-  return exec_path.substr(0, exec_path.find_last_of(kPathSeparator));
-}
-
 void StartProfilers(Environment* env) {
   AtExit(env, [](void* env) {
     EndStartedProfilers(static_cast<Environment*>(env));
@@ -427,7 +411,7 @@ void StartProfilers(Environment* env) {
   if (env->options()->cpu_prof) {
     const std::string& dir = env->options()->cpu_prof_dir;
     env->set_cpu_prof_interval(env->options()->cpu_prof_interval);
-    env->set_cpu_prof_dir(dir.empty() ? GetCwd(env) : dir);
+    env->set_cpu_prof_dir(dir.empty() ? env->GetCwd() : dir);
     if (env->options()->cpu_prof_name.empty()) {
       DiagnosticFilename filename(env, "CPU", "cpuprofile");
       env->set_cpu_prof_name(*filename);
@@ -442,7 +426,7 @@ void StartProfilers(Environment* env) {
   if (env->options()->heap_prof) {
     const std::string& dir = env->options()->heap_prof_dir;
     env->set_heap_prof_interval(env->options()->heap_prof_interval);
-    env->set_heap_prof_dir(dir.empty() ? GetCwd(env) : dir);
+    env->set_heap_prof_dir(dir.empty() ? env->GetCwd() : dir);
     if (env->options()->heap_prof_name.empty()) {
       DiagnosticFilename filename(env, "Heap", "heapprofile");
       env->set_heap_prof_name(*filename);
diff --git a/src/node.cc b/src/node.cc
index 521cae7757d477..efb4876002f774 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -275,6 +275,10 @@ static void AtomicsWaitCallback(Isolate::AtomicsWaitEvent event,
 void Environment::InitializeDiagnostics() {
   isolate_->GetHeapProfiler()->AddBuildEmbedderGraphCallback(
       Environment::BuildEmbedderGraph, this);
+  if (options_->heap_snapshot_near_heap_limit > 0) {
+    isolate_->AddNearHeapLimitCallback(Environment::NearHeapLimitCallback,
+                                       this);
+  }
   if (options_->trace_uncaught)
     isolate_->SetCaptureStackTraceForUncaughtExceptions(true);
   if (options_->trace_atomics_wait) {
diff --git a/src/node_internals.h b/src/node_internals.h
index aa7180e18544ca..f7a1e2d8d62c24 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -363,6 +363,10 @@ class DiagnosticFilename {
   std::string filename_;
 };
 
+namespace heap {
+bool WriteSnapshot(v8::Isolate* isolate, const char* filename);
+}
+
 class TraceEventScope {
  public:
   TraceEventScope(const char* category,
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index d406bf15444bf8..84d39831dcd1f7 100644
--- a/src/node_main_instance.cc
+++ b/src/node_main_instance.cc
@@ -108,6 +108,8 @@ NodeMainInstance::NodeMainInstance(
     // complete.
     SetIsolateErrorHandlers(isolate_, s);
   }
+  isolate_data_->max_young_gen_size =
+      params->constraints.max_young_generation_size_in_bytes();
 }
 
 void NodeMainInstance::Dispose() {
diff --git a/src/node_options.cc b/src/node_options.cc
index 4fb4f455c47b00..2d9d71e6ec6e91 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -118,6 +118,12 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
                       "used, not both");
   }
 
+  if (heap_snapshot_near_heap_limit < 1 &&
+      heap_snapshot_near_heap_limit != -1) {
+    errors->push_back(
+        "--heap-snapshot-near-heap-limit must be either postive or -1");
+  }
+
 #if HAVE_INSPECTOR
   if (!cpu_prof) {
     if (!cpu_prof_name.empty()) {
@@ -341,6 +347,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "Generate heap snapshot on specified signal",
             &EnvironmentOptions::heap_snapshot_signal,
             kAllowedInEnvironment);
+  AddOption("--heapsnapshot-near-heap-limit",
+            "Generate heap snapshots whenever V8 is approaching "
+            "the heap limit. No more than the specified number of "
+            "heap snapshots will be generated.",
+            &EnvironmentOptions::heap_snapshot_near_heap_limit,
+            kAllowedInEnvironment);
   AddOption("--http-parser", "", NoOp{}, kAllowedInEnvironment);
   AddOption("--insecure-http-parser",
             "use an insecure HTTP parser that accepts invalid HTTP headers",
diff --git a/src/node_options.h b/src/node_options.h
index e8051f64bc3f38..4b98b9dff1546b 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -115,6 +115,7 @@ class EnvironmentOptions : public Options {
   bool experimental_vm_modules = false;
   bool expose_internals = false;
   bool frozen_intrinsics = false;
+  int64_t heap_snapshot_near_heap_limit = -1;
   std::string heap_snapshot_signal;
   uint64_t max_http_header_size = 16 * 1024;
   bool no_deprecation = false;
diff --git a/src/node_worker.cc b/src/node_worker.cc
index 2006380cd411dc..e28aab7fb24c7f 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -159,6 +159,9 @@ class WorkerThreadData {
     Isolate::Initialize(isolate, params);
     SetIsolateUpForNode(isolate);
 
+    // Be sure it's called before Environment::InitializeDiagnostics()
+    // so that this callback stays when the callback of
+    // --heapsnapshot-near-heap-limit gets is popped.
     isolate->AddNearHeapLimitCallback(Worker::NearHeapLimit, w);
 
     {
@@ -177,6 +180,8 @@ class WorkerThreadData {
       if (w_->per_isolate_opts_)
         isolate_data_->set_options(std::move(w_->per_isolate_opts_));
       isolate_data_->set_worker_context(w_);
+      isolate_data_->max_young_gen_size =
+          params.constraints.max_young_generation_size_in_bytes();
     }
 
     Mutex::ScopedLock lock(w_->mutex_);
diff --git a/test/fixtures/workload/bounded.js b/test/fixtures/workload/bounded.js
new file mode 100644
index 00000000000000..ddf288d034bd1a
--- /dev/null
+++ b/test/fixtures/workload/bounded.js
@@ -0,0 +1,22 @@
+'use strict';
+
+const total = parseInt(process.env.TEST_ALLOCATION) || 5000;
+const chunk = parseInt(process.env.TEST_CHUNK) || 1000;
+const cleanInterval = parseInt(process.env.TEST_CLEAN_INTERVAL) || 100;
+let count = 0;
+let arr = [];
+function runAllocation() {
+  count++;
+  if (count < total) {
+    if (count % cleanInterval === 0) {
+      arr.splice(0, arr.length);
+      setImmediate(runAllocation);
+    } else {
+      const str = JSON.stringify(process.config).slice(0, chunk);
+      arr.push(str);
+      setImmediate(runAllocation);
+    }
+  }
+}
+
+setImmediate(runAllocation);
diff --git a/test/fixtures/workload/grow-worker.js b/test/fixtures/workload/grow-worker.js
new file mode 100644
index 00000000000000..092d8f27751fc2
--- /dev/null
+++ b/test/fixtures/workload/grow-worker.js
@@ -0,0 +1,14 @@
+'use strict';
+
+const { Worker } = require('worker_threads');
+const path = require('path');
+const max_snapshots = parseInt(process.env.TEST_SNAPSHOTS) || 1;
+new Worker(path.join(__dirname, 'grow.js'), {
+  execArgv: [
+    `--heapsnapshot-near-heap-limit=${max_snapshots}`,
+  ],
+  resourceLimits: {
+    maxOldGenerationSizeMb:
+      parseInt(process.env.TEST_OLD_SPACE_SIZE) || 20
+  }
+});
diff --git a/test/fixtures/workload/grow.js b/test/fixtures/workload/grow.js
new file mode 100644
index 00000000000000..9ac0139b332b52
--- /dev/null
+++ b/test/fixtures/workload/grow.js
@@ -0,0 +1,12 @@
+'use strict';
+
+const chunk = parseInt(process.env.TEST_CHUNK) || 1000;
+
+let arr = [];
+function runAllocation() {
+  const str = JSON.stringify(process.config).slice(0, chunk);
+  arr.push(str);
+  setImmediate(runAllocation);
+}
+
+setImmediate(runAllocation);
diff --git a/test/parallel/test-heapsnapshot-near-heap-limit-bounded.js b/test/parallel/test-heapsnapshot-near-heap-limit-bounded.js
new file mode 100644
index 00000000000000..16d1f915fee3c3
--- /dev/null
+++ b/test/parallel/test-heapsnapshot-near-heap-limit-bounded.js
@@ -0,0 +1,36 @@
+'use strict';
+
+require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+const env = {
+  ...process.env,
+  TEST_ALLOCATION: 50000,
+  TEST_CHUNK: 1000,
+  TEST_CLEAN_INTERVAL: 500,
+  NODE_DEBUG_NATIVE: 'diagnostics'
+};
+
+{
+  console.log('\nTesting limit = 1');
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--trace-gc',
+    '--heapsnapshot-near-heap-limit=1',
+    '--max-old-space-size=50',
+    fixtures.path('workload', 'bounded.js')
+  ], {
+    cwd: tmpdir.path,
+    env,
+  });
+  console.log(child.stdout.toString());
+  console.log(child.stderr.toString());
+  assert.strictEqual(child.signal, null);
+  assert.strictEqual(child.status, 0);
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  assert.strictEqual(list.length, 0);
+}
diff --git a/test/parallel/test-heapsnapshot-near-heap-limit.js b/test/parallel/test-heapsnapshot-near-heap-limit.js
new file mode 100644
index 00000000000000..254f686dc8df49
--- /dev/null
+++ b/test/parallel/test-heapsnapshot-near-heap-limit.js
@@ -0,0 +1,106 @@
+'use strict';
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+const env = {
+  ...process.env,
+  NODE_DEBUG_NATIVE: 'diagnostics'
+};
+
+{
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--heapsnapshot-near-heap-limit=-15',
+    '--max-old-space-size=50',
+    fixtures.path('workload', 'grow.js')
+  ], {
+    cwd: tmpdir.path,
+    env,
+  });
+  assert.strictEqual(child.status, 9);
+}
+
+{
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--heapsnapshot-near-heap-limit=0',
+    '--max-old-space-size=50',
+    fixtures.path('workload', 'grow.js')
+  ], {
+    cwd: tmpdir.path,
+    env,
+  });
+  assert.strictEqual(child.status, 9);
+}
+
+{
+  console.log('\nTesting limit = 1');
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--trace-gc',
+    '--heapsnapshot-near-heap-limit=1',
+    '--max-old-space-size=50',
+    fixtures.path('workload', 'grow.js')
+  ], {
+    cwd: tmpdir.path,
+    env,
+  });
+  console.log(child.stdout.toString());
+  console.log(child.stderr.toString());
+  assert(common.nodeProcessAborted(child.status, child.signal),
+         'process should have aborted, but did not');
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  assert.strictEqual(list.length, 1);
+}
+
+{
+  console.log('\nTesting limit = 3');
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--trace-gc',
+    '--heapsnapshot-near-heap-limit=3',
+    '--max-old-space-size=50',
+    fixtures.path('workload', 'grow.js')
+  ], {
+    cwd: tmpdir.path,
+    env,
+  });
+  console.log(child.stdout.toString());
+  console.log(child.stderr.toString());
+  assert(common.nodeProcessAborted(child.status, child.signal),
+         'process should have aborted, but did not');
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  assert(list.length > 0 && list.length <= 3);
+}
+
+
+{
+  console.log('\nTesting worker');
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    fixtures.path('workload', 'grow-worker.js')
+  ], {
+    cwd: tmpdir.path,
+    env: {
+      TEST_SNAPSHOTS: 1,
+      TEST_OLD_SPACE_SIZE: 50,
+      ...env
+    }
+  });
+  console.log(child.stdout.toString());
+  const stderr = child.stderr.toString();
+  console.log(stderr);
+  // There should be one snapshot taken and then after the
+  // snapshot heap limit callback is popped, the OOM callback
+  // becomes effective.
+  assert(stderr.includes('ERR_WORKER_OUT_OF_MEMORY'));
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  assert.strictEqual(list.length, 1);
+}
diff --git a/test/pummel/test-heapsnapshot-near-heap-limit-big.js b/test/pummel/test-heapsnapshot-near-heap-limit-big.js
new file mode 100644
index 00000000000000..7cc4e4fdf85ac2
--- /dev/null
+++ b/test/pummel/test-heapsnapshot-near-heap-limit-big.js
@@ -0,0 +1,43 @@
+'use strict';
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+const env = {
+  ...process.env,
+  NODE_DEBUG_NATIVE: 'diagnostics'
+};
+
+if (!common.enoughTestMem)
+  common.skip('Insufficient memory for snapshot test');
+
+{
+  console.log('\nTesting limit = 3');
+  tmpdir.refresh();
+  const child = spawnSync(process.execPath, [
+    '--heapsnapshot-near-heap-limit=3',
+    '--max-old-space-size=512',
+    fixtures.path('workload', 'grow.js')
+  ], {
+    cwd: tmpdir.path,
+    env: {
+      ...env,
+      TEST_CHUNK: 2000,
+    }
+  });
+  const stderr = child.stderr.toString();
+  console.log(stderr);
+  assert(common.nodeProcessAborted(child.status, child.signal),
+         'process should have aborted, but did not');
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  if (list.length === 0) {
+    assert(stderr.includes(
+      'Not generating snapshots because it\'s too risky'));
+  } else {
+    assert(list.length > 0 && list.length <= 5);
+  }
+}

From ca8101abbf2064dd142ef7faa1a91af1e594309d Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Thu, 15 Oct 2020 22:32:41 +0800
Subject: [PATCH 2/3] fixup! src: add --heapsnapshot-near-heap-limit option

---
 doc/api/cli.md                                       |  5 ++---
 src/node_options.cc                                  |  6 ++----
 src/node_options.h                                   |  2 +-
 test/parallel/test-heapsnapshot-near-heap-limit.js   | 10 +++++++++-
 test/pummel/test-heapsnapshot-near-heap-limit-big.js |  2 +-
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index 27977f20ed0246..d8121f564b0a5a 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -341,9 +341,8 @@ added: REPLACEME
 > Stability: 1 - Experimental
 
 Writes a V8 heap snapshot to disk when the V8 heap usage is approaching the
-heap limit. `count` should be either -1 (in which case no snapshots would be
-written), or a positive integer (in which case Node.js will write no more than
-`max_count` snapshots to disk).
+heap limit. `count` should be a non-negative integer (in which case
+Node.js will write no more than `max_count` snapshots to disk).
 
 When generating snapshots, garbage collection may be triggered and bring
 the heap usage down, therefore multiple snapshots may be written to disk
diff --git a/src/node_options.cc b/src/node_options.cc
index 2d9d71e6ec6e91..04893fee408e18 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -118,10 +118,8 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
                       "used, not both");
   }
 
-  if (heap_snapshot_near_heap_limit < 1 &&
-      heap_snapshot_near_heap_limit != -1) {
-    errors->push_back(
-        "--heap-snapshot-near-heap-limit must be either postive or -1");
+  if (heap_snapshot_near_heap_limit < 0) {
+    errors->push_back("--heap-snapshot-near-heap-limit must not be negative");
   }
 
 #if HAVE_INSPECTOR
diff --git a/src/node_options.h b/src/node_options.h
index 4b98b9dff1546b..84ee8e34bcafcf 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -115,7 +115,7 @@ class EnvironmentOptions : public Options {
   bool experimental_vm_modules = false;
   bool expose_internals = false;
   bool frozen_intrinsics = false;
-  int64_t heap_snapshot_near_heap_limit = -1;
+  int64_t heap_snapshot_near_heap_limit = 0;
   std::string heap_snapshot_signal;
   uint64_t max_http_header_size = 16 * 1024;
   bool no_deprecation = false;
diff --git a/test/parallel/test-heapsnapshot-near-heap-limit.js b/test/parallel/test-heapsnapshot-near-heap-limit.js
index 254f686dc8df49..db75da221ab017 100644
--- a/test/parallel/test-heapsnapshot-near-heap-limit.js
+++ b/test/parallel/test-heapsnapshot-near-heap-limit.js
@@ -25,8 +25,10 @@ const env = {
 }
 
 {
+  console.log('\nTesting limit = 0');
   tmpdir.refresh();
   const child = spawnSync(process.execPath, [
+    '--trace-gc',
     '--heapsnapshot-near-heap-limit=0',
     '--max-old-space-size=50',
     fixtures.path('workload', 'grow.js')
@@ -34,7 +36,13 @@ const env = {
     cwd: tmpdir.path,
     env,
   });
-  assert.strictEqual(child.status, 9);
+  console.log(child.stdout.toString());
+  console.log(child.stderr.toString());
+  assert(common.nodeProcessAborted(child.status, child.signal),
+         'process should have aborted, but did not');
+  const list = fs.readdirSync(tmpdir.path)
+    .filter((file) => file.endsWith('.heapsnapshot'));
+  assert.strictEqual(list.length, 0);
 }
 
 {
diff --git a/test/pummel/test-heapsnapshot-near-heap-limit-big.js b/test/pummel/test-heapsnapshot-near-heap-limit-big.js
index 7cc4e4fdf85ac2..f70d562c1d1ee4 100644
--- a/test/pummel/test-heapsnapshot-near-heap-limit-big.js
+++ b/test/pummel/test-heapsnapshot-near-heap-limit-big.js
@@ -38,6 +38,6 @@ if (!common.enoughTestMem)
     assert(stderr.includes(
       'Not generating snapshots because it\'s too risky'));
   } else {
-    assert(list.length > 0 && list.length <= 5);
+    assert(list.length > 0 && list.length <= 3);
   }
 }

From f5402b22c3a7e0840a7b420d38a67959c88b7ce0 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Tue, 20 Oct 2020 05:26:51 +0800
Subject: [PATCH 3/3] fixup! fixup! src: add --heapsnapshot-near-heap-limit
 option

---
 doc/api/cli.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index d8121f564b0a5a..29dfce564762e5 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -351,7 +351,7 @@ can be compared to determine what objects are being allocated during the
 time consecutive snapshots are taken. It's not guaranteed that Node.js will
 write exactly `max_count` snapshots to disk, but it will try
 its best to generate at least one and up to `max_count` snapshots before the
-Node.js instance runs out of memory.
+Node.js instance runs out of memory when `max_count` is greater than `0`.
 
 Generating V8 snapshots takes time and memory (both memory managed by the
 V8 heap and native memory outside the V8 heap). The bigger the heap is,