Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ const {
emitBefore,
emitAfter,
emitDestroy,
enabledHooksExist,
initHooksExist,
destroyHooksExist,
} = internal_async_hooks;
Expand Down Expand Up @@ -188,7 +187,7 @@ class AsyncResource {
this[trigger_async_id_symbol] = triggerAsyncId;

if (initHooksExist()) {
if (enabledHooksExist() && type.length === 0) {
if (type.length === 0) {
throw new ERR_ASYNC_TYPE(type);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ function hasHooks(key) {
}

function enabledHooksExist() {
return hasHooks(kCheck);
return active_hooks.array.length > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same as calling getHookArrays, I don't know if you already considering the implications of .array and tmp_array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the same code but to my understanding it fits the usecase it is used for.

}

function initHooksExist() {
Expand Down Expand Up @@ -563,7 +563,7 @@ function popAsyncContext(asyncId) {
const stackLength = async_hook_fields[kStackLength];
if (stackLength === 0) return false;

if (enabledHooksExist() && async_id_fields[kExecutionAsyncId] !== asyncId) {
if (async_hook_fields[kCheck] > 0 && async_id_fields[kExecutionAsyncId] !== asyncId) {
// Do the same thing as the native code (i.e. crash hard).
return popAsyncContext_(asyncId);
}
Expand Down
5 changes: 2 additions & 3 deletions lib/internal/streams/end-of-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const {
kIsClosedPromise,
} = require('internal/streams/utils');

const { getHookArrays } = require('internal/async_hooks');
const { enabledHooksExist } = require('internal/async_hooks');
const AsyncContextFrame = require('internal/async_context_frame');

// Lazy load
Expand Down Expand Up @@ -78,8 +78,7 @@ function eos(stream, options, callback) {
validateFunction(callback, 'callback');
validateAbortSignal(options.signal, 'options.signal');

if (AsyncContextFrame.current() ||
getHookArrays()[0].length > 0) {
if (AsyncContextFrame.current() || enabledHooksExist()) {
// Avoid AsyncResource.bind() because it calls ObjectDefineProperties which
// is a bottleneck here.
callback = once(bindAsyncResource(callback, 'STREAM_END_OF_STREAM'));
Expand Down
5 changes: 2 additions & 3 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ void AsyncHooks::push_async_context(
std::variant<Local<Object>*, Global<Object>*> resource) {
std::visit([](auto* ptr) { CHECK_IMPLIES(ptr != nullptr, !ptr->IsEmpty()); },
resource);
// Since async_hooks is experimental, do only perform the check
// when async_hooks is enabled.

if (fields_[kCheck] > 0) {
CHECK_GE(async_id, -1);
CHECK_GE(trigger_async_id, -1);
Expand Down Expand Up @@ -1756,7 +1755,7 @@ AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info)
clear_async_id_stack();

// Always perform async_hooks checks, not just when async_hooks is enabled.
// TODO(AndreasMadsen): Consider removing this for LTS releases.
// Can be disabled via CLI option --no-force-async-hooks-checks
// 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.
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-async-hooks-enabledhooksexits.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Flags: --expose-internals
'use strict';

require('../common');
const assert = require('assert');
const { createHook } = require('async_hooks');
const { enabledHooksExist } = require('internal/async_hooks');

assert.strictEqual(enabledHooksExist(), false);

const ah = createHook({});
assert.strictEqual(enabledHooksExist(), false);

ah.enable();
assert.strictEqual(enabledHooksExist(), true);

ah.disable();
assert.strictEqual(enabledHooksExist(), false);
5 changes: 2 additions & 3 deletions test/parallel/test-stream-finished-async-local-storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const { Readable, finished } = require('stream');
const { AsyncLocalStorage } = require('async_hooks');
const assert = require('assert');
const AsyncContextFrame = require('internal/async_context_frame');
const internalAsyncHooks = require('internal/async_hooks');
const { enabledHooksExist } = require('internal/async_hooks');

// This test verifies that ALS context is preserved when using stream.finished()

Expand All @@ -15,8 +15,7 @@ const readable = new Readable();

als.run('test-context-1', common.mustCall(() => {
finished(readable, common.mustCall(() => {
assert.strictEqual(AsyncContextFrame.enabled || internalAsyncHooks.getHookArrays()[0].length > 0,
true);
assert.strictEqual(AsyncContextFrame.enabled || enabledHooksExist(), true);
assert.strictEqual(als.getStore(), 'test-context-1');
}));
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-stream-finished-bindAsyncResource-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const common = require('../common');
const { Readable, finished } = require('stream');
const { createHook, executionAsyncId } = require('async_hooks');
const assert = require('assert');
const internalAsyncHooks = require('internal/async_hooks');
const { enabledHooksExist } = require('internal/async_hooks');

// This test verifies that when there are active async hooks, stream.finished() uses
// the bindAsyncResource path
Expand All @@ -27,7 +27,7 @@ const readable = new Readable();
finished(readable, common.mustCall(() => {
const currentAsyncId = executionAsyncId();
const ctx = contextMap.get(currentAsyncId);
assert.strictEqual(internalAsyncHooks.getHookArrays()[0].length > 0, true);
assert.ok(enabledHooksExist());
assert.strictEqual(ctx, 'abc-123');
}));

Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-stream-finished-default-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@ const common = require('../common');
const { Readable, finished } = require('stream');
const assert = require('assert');
const AsyncContextFrame = require('internal/async_context_frame');
const internalAsyncHooks = require('internal/async_hooks');
const { enabledHooksExist } = require('internal/async_hooks');

// This test verifies that when there are no active async hooks, stream.finished() uses the default callback path

const readable = new Readable();

finished(readable, common.mustCall(() => {
assert.strictEqual(internalAsyncHooks.getHookArrays()[0].length === 0, true);
assert.strictEqual(enabledHooksExist(), false);
assert.strictEqual(
AsyncContextFrame.current() || internalAsyncHooks.getHookArrays()[0].length > 0,
AsyncContextFrame.current() || enabledHooksExist(),
false,
);
}));
Expand Down
Loading