-
-
Notifications
You must be signed in to change notification settings - Fork 34.1k
Description
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):
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?