From 23a3911cb107b0170c0ba803df574007c14ea72c Mon Sep 17 00:00:00 2001
From: Andreas Madsen <amwebdk@gmail.com>
Date: Thu, 19 Oct 2017 14:15:08 +0200
Subject: [PATCH] async_hooks: enable runtime checks by default

Ref: https://github.com/nodejs/node/pull/15454
PR-URL: https://github.com/nodejs/node/pull/16318
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
---
 doc/api/cli.md                                |  6 ++---
 doc/node.1                                    |  6 ++---
 src/env-inl.h                                 | 13 +++++++---
 src/env.h                                     |  2 +-
 src/node.cc                                   | 16 ++++++------
 test/async-hooks/test-force-checks-flag.js    | 25 -------------------
 .../test-no-assert-when-disabled.js           | 10 +-------
 test/common/index.js                          | 11 --------
 8 files changed, 26 insertions(+), 63 deletions(-)
 delete mode 100644 test/async-hooks/test-force-checks-flag.js

diff --git a/doc/api/cli.md b/doc/api/cli.md
index b6d778f5ac4..c0fd8f57615 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -208,13 +208,13 @@ added: v2.1.0
 Prints a stack trace whenever synchronous I/O is detected after the first turn
 of the event loop.
 
-### `--force-async-hooks-checks`
+### `--no-force-async-hooks-checks`
 <!-- YAML
 added: REPLACEME
 -->
 
-Enables runtime checks for `async_hooks`. These can also be enabled dynamically
-by enabling one of the `async_hooks` hooks.
+Disables runtime checks for `async_hooks`. These will still be enabled
+dynamically when `async_hooks` is enabled.
 
 ### `--trace-events-enabled`
 <!-- YAML
diff --git a/doc/node.1 b/doc/node.1
index 1d162b95de5..0fad250c6d5 100644
--- a/doc/node.1
+++ b/doc/node.1
@@ -153,9 +153,9 @@ Print a stack trace whenever synchronous I/O is detected after the first turn
 of the event loop.
 
 .TP
-.BR \-\-force\-async\-hooks\-checks
-Enables runtime checks for `async_hooks`. These can also be enabled dynamically
-by enabling one of the `async_hooks` hooks.
+.BR \-\-no\-force\-async\-hooks\-checks
+Disables runtime checks for `async_hooks`. These will still be enabled
+dynamically when `async_hooks` is enabled.
 
 .TP
 .BR \-\-trace\-events\-enabled
diff --git a/src/env-inl.h b/src/env-inl.h
index e6e189be3dc..bd13881cb13 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -88,6 +88,13 @@ inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate)
       async_id_fields_(isolate, kUidFieldsCount) {
   v8::HandleScope handle_scope(isolate_);
 
+  // Always perform async_hooks checks, not just when async_hooks is enabled.
+  // TODO(AndreasMadsen): Consider removing this for LTS releases.
+  // See discussion in https://github.com/nodejs/node/pull/15454
+  // When removing this, do it by reverting the commit. Otherwise the test
+  // and flag changes won't be included.
+  fields_[kCheck] = 1;
+
   // kAsyncIdCounter should start at 1 because that'll be the id the execution
   // context during bootstrap (code that runs before entering uv_run()).
   async_id_fields_[AsyncHooks::kAsyncIdCounter] = 1;
@@ -129,9 +136,9 @@ inline v8::Local<v8::String> Environment::AsyncHooks::provider_string(int idx) {
   return providers_[idx].Get(isolate_);
 }
 
-inline void Environment::AsyncHooks::force_checks() {
-  // fields_ does not have the += operator defined
-  fields_[kCheck] = fields_[kCheck] + 1;
+inline void Environment::AsyncHooks::no_force_checks() {
+  // fields_ does not have the -= operator defined
+  fields_[kCheck] = fields_[kCheck] - 1;
 }
 
 inline void Environment::AsyncHooks::push_async_ids(double async_id,
diff --git a/src/env.h b/src/env.h
index a5554337cb3..c8bb168df4d 100644
--- a/src/env.h
+++ b/src/env.h
@@ -410,7 +410,7 @@ class Environment {
 
     inline v8::Local<v8::String> provider_string(int idx);
 
-    inline void force_checks();
+    inline void no_force_checks();
 
     inline void push_async_ids(double async_id, double trigger_async_id);
     inline bool pop_async_id(double async_id);
diff --git a/src/node.cc b/src/node.cc
index b780d526531..2039edfeb9c 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -173,7 +173,7 @@ static bool syntax_check_only = false;
 static bool trace_deprecation = false;
 static bool throw_deprecation = false;
 static bool trace_sync_io = false;
-static bool force_async_hooks_checks = false;
+static bool no_force_async_hooks_checks = false;
 static bool track_heap_objects = false;
 static const char* eval_string = nullptr;
 static std::vector<std::string> preload_modules;
@@ -3899,8 +3899,8 @@ static void PrintHelp() {
          "                             stderr\n"
          "  --trace-sync-io            show stack trace when use of sync IO\n"
          "                             is detected after the first tick\n"
-         "  --force-async-hooks-checks\n"
-         "                             enables checks for async_hooks\n"
+         "  --no-force-async-hooks-checks\n"
+         "                             disable checks for async_hooks\n"
          "  --trace-events-enabled     track trace events\n"
          "  --trace-event-categories   comma separated list of trace event\n"
          "                             categories to record\n"
@@ -4026,7 +4026,7 @@ static void CheckIfAllowedInEnv(const char* exe, bool is_env,
     "--trace-warnings",
     "--redirect-warnings",
     "--trace-sync-io",
-    "--force-async-hooks-checks",
+    "--no-force-async-hooks-checks",
     "--trace-events-enabled",
     "--trace-events-categories",
     "--track-heap-objects",
@@ -4165,8 +4165,8 @@ static void ParseArgs(int* argc,
       trace_deprecation = true;
     } else if (strcmp(arg, "--trace-sync-io") == 0) {
       trace_sync_io = true;
-    } else if (strcmp(arg, "--force-async-hooks-checks") == 0) {
-      force_async_hooks_checks = true;
+    } else if (strcmp(arg, "--no-force-async-hooks-checks") == 0) {
+      no_force_async_hooks_checks = true;
     } else if (strcmp(arg, "--trace-events-enabled") == 0) {
       trace_enabled = true;
     } else if (strcmp(arg, "--trace-event-categories") == 0) {
@@ -4815,8 +4815,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
 
   env.set_abort_on_uncaught_exception(abort_on_uncaught_exception);
 
-  if (force_async_hooks_checks) {
-    env.async_hooks()->force_checks();
+  if (no_force_async_hooks_checks) {
+    env.async_hooks()->no_force_checks();
   }
 
   {
diff --git a/test/async-hooks/test-force-checks-flag.js b/test/async-hooks/test-force-checks-flag.js
deleted file mode 100644
index daafd56778b..00000000000
--- a/test/async-hooks/test-force-checks-flag.js
+++ /dev/null
@@ -1,25 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-const async_hooks = require('async_hooks');
-const spawnSync = require('child_process').spawnSync;
-
-// disabling this way will just decrement the kCheck counter. It won't actually
-// disable the checks, because they were enabled with the flag.
-common.revert_force_async_hooks_checks();
-
-switch (process.argv[2]) {
-  case 'test_invalid_async_id':
-    async_hooks.emitInit();
-    return;
-}
-assert.ok(!process.argv[2]);
-
-
-const c1 = spawnSync(process.execPath, [
-  '--force-async-hooks-checks', __filename, 'test_invalid_async_id'
-]);
-assert.strictEqual(
-  c1.stderr.toString().split(/[\r\n]+/g)[0],
-  'RangeError [ERR_INVALID_ASYNC_ID]: Invalid asyncId value: undefined');
-assert.strictEqual(c1.status, 1);
diff --git a/test/async-hooks/test-no-assert-when-disabled.js b/test/async-hooks/test-no-assert-when-disabled.js
index b3822dde3ae..47aafe52c35 100644
--- a/test/async-hooks/test-no-assert-when-disabled.js
+++ b/test/async-hooks/test-no-assert-when-disabled.js
@@ -1,18 +1,10 @@
 'use strict';
-// Flags: --expose-internals
+// Flags: --no-force-async-hooks-checks --expose-internals
 const common = require('../common');
 
 const async_hooks = require('async_hooks');
 const internal = require('internal/process/next_tick');
 
-// In tests async_hooks dynamic checks are enabled by default. To verify
-// that no checks are enabled ordinarily disable the checks in this test.
-common.revert_force_async_hooks_checks();
-
-// When async_hooks is diabled (or never enabled), the checks
-// should be disabled as well. This is important while async_hooks is
-// experimental and there are still critial bugs to fix.
-
 // Using undefined as the triggerAsyncId.
 // Ref: https://github.com/nodejs/node/issues/14386
 // Ref: https://github.com/nodejs/node/issues/14381
diff --git a/test/common/index.js b/test/common/index.js
index 04f0c79b2fd..838c7b60da2 100644
--- a/test/common/index.js
+++ b/test/common/index.js
@@ -65,17 +65,6 @@ exports.projectDir = path.resolve(__dirname, '..', '..');
 
 exports.buildType = process.config.target_defaults.default_configuration;
 
-// Always enable async_hooks checks in tests
-{
-  const async_wrap = process.binding('async_wrap');
-  const { kCheck } = async_wrap.constants;
-  async_wrap.async_hook_fields[kCheck] += 1;
-
-  exports.revert_force_async_hooks_checks = function() {
-    async_wrap.async_hook_fields[kCheck] -= 1;
-  };
-}
-
 // If env var is set then enable async_hook hooks for all tests.
 if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) {
   const destroydIdsList = {};