Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

enabledHooksExist() doesn't work as intended #61019

Copy link
Copy link
@gurgunday

Description

@gurgunday
Issue body actions

I took a look at enabledHooksExist(), but as discussed in #59873 (comment) and #60913, it seems to not work as intended at the moment:

Following the suggestion to use enabledHooksExist(), I spent quite some time on it and gave that a try—but found that it consistently returned true, even when no async hooks were active. So, I reverted to using getHookArrays()[0].length > 0, as it provides a more accurate reflection of the current hook state.


enabledHooksExist() checks async_hook_fields[kCheck] > 0, but kCheck is intentionally set to 1 at Node.js startup, even when no user async hooks have been registered:

From env.cc (L1763):

node/src/env.cc

Lines 1748 to 1763 in e28656a

AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info)
: async_ids_stack_(isolate, 16 * 2, MAYBE_FIELD_PTR(info, async_ids_stack)),
fields_(isolate, kFieldsCount, MAYBE_FIELD_PTR(info, fields)),
async_id_fields_(
isolate, kUidFieldsCount, MAYBE_FIELD_PTR(info, async_id_fields)),
info_(info) {
HandleScope handle_scope(isolate);
if (info == nullptr) {
clear_async_id_stack();
// 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;

This means enabledHooksExist() always returns true, regardless of whether any AsyncHook instances have actually been created and enabled by user code.


This behavior dates back to PR #15454 it seems, but even after going through that discussion, I don't really understand the reasoning behind it and the TODO that says we should revert this for LTS releases. I feel like fields_[kCheck] should instead be set to 0 during initialization.

Should we just change that or add a new helper function that would essentially do AsyncContextFrame.current() != null || getHookArrays()[0].length > 0?

Metadata

Metadata

Assignees

No one assigned

    Labels

    async_hooksIssues and PRs related to the async hooks subsystem.Issues and PRs related to the async hooks subsystem.async_local_storageAsyncLocalStorageAsyncLocalStorage

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.