async_hooks: enable runtime checks by default

Ref: nodejs/node#15454 PR-URL: nodejs/node#16318 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
ayojs · Nov 2, 2017 · a9ced7f · a9ced7f
1 parent ee585b2
commit a9ced7f
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 63 deletions.
diff --git 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/ayo.1 b/doc/ayo.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
@@ -60,6 +60,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;
@@ -101,9 +108,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
@@ -434,7 +434,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
@@ -176,7 +176,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;
@@ -3969,8 +3969,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"
@@ -4096,7 +4096,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",
@@ -4235,8 +4235,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) {
@@ -4892,8 +4892,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
diff --git 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
@@ -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 = {};