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

@@ -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

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

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,

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);

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();
   }
 
   {

test/async-hooks/test-force-checks-flag.js

@@ -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);

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

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 = {};