Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: enable runtime checks by default #16318

Closed
wants to merge 1 commit into from
Closed

async_hooks: enable runtime checks by default #16318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 10 additions & 3 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,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);
Expand Down
16 changes: 8 additions & 8 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}

{
Expand Down
25 changes: 0 additions & 25 deletions test/async-hooks/test-force-checks-flag.js
View file
Open in desktop

This file was deleted.

10 changes: 1 addition & 9 deletions test/async-hooks/test-no-assert-when-disabled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
11 changes: 0 additions & 11 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,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 = {};
Expand Down