From 4af53273c204f99807339f53da756cd98cde182d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 9 Apr 2023 13:13:10 +0200 Subject: [PATCH 1/4] Add `async_hooks` strategy --- packages/node/src/async/hooks.ts | 62 +++++++++++++++++ packages/node/test/async/hooks.test.ts | 92 ++++++++++++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 packages/node/src/async/hooks.ts create mode 100644 packages/node/test/async/hooks.test.ts diff --git a/packages/node/src/async/hooks.ts b/packages/node/src/async/hooks.ts new file mode 100644 index 000000000000..01f8f7ab9478 --- /dev/null +++ b/packages/node/src/async/hooks.ts @@ -0,0 +1,62 @@ +import type { Carrier, Hub, RunWithAsyncContextOptions } from '@sentry/core'; +import { + ensureHubOnCarrier, + // getCurrentHub as getCurrentHubCore, + getHubFromCarrier, + setAsyncContextStrategy, +} from '@sentry/core'; + +interface AsyncLocalStorage { + disable(): void; + getStore(): T | undefined; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + run(store: T, callback: (...args: TArgs) => R, ...args: TArgs): R; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + exit(callback: (...args: TArgs) => R, ...args: TArgs): R; + enterWith(store: T): void; +} + +function createAsyncLocalStorage(): AsyncLocalStorage { + // async_hooks does not exist before Node v12.17 + + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { AsyncLocalStorage } = require('async_hooks'); + return new AsyncLocalStorage(); +} + +/** + * Sets the async context strategy to use Node.js async_hooks. + */ +export function setHooksAsyncContextStrategy(): void { + const asyncStorage = createAsyncLocalStorage(); + + function getCurrentHub(): Hub | undefined { + return asyncStorage.getStore(); + } + + function createNewHub(): Hub { + const carrier: Carrier = {}; + ensureHubOnCarrier(carrier); + return getHubFromCarrier(carrier); + } + + function runWithAsyncContext(callback: (hub: Hub) => T, options: RunWithAsyncContextOptions): T { + if (options?.reuseExisting) { + const existingHub = getCurrentHub(); + + if (existingHub) { + // We're already in an async context, so we don't need to create a new one + // just call the callback with the current hub + return callback(existingHub); + } + } + + const newHub = createNewHub(); + + return asyncStorage.run(newHub, () => { + return callback(newHub); + }); + } + + setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext }); +} diff --git a/packages/node/test/async/hooks.test.ts b/packages/node/test/async/hooks.test.ts new file mode 100644 index 000000000000..5e24223f7946 --- /dev/null +++ b/packages/node/test/async/hooks.test.ts @@ -0,0 +1,92 @@ +import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; +import * as domain from 'domain'; + +import { setHooksAsyncContextStrategy } from '../../src/async/hooks'; + +describe('async hooks', () => { + afterAll(() => { + // clear the strategy + setAsyncContextStrategy(undefined); + }); + + test('without hooks', () => { + // @ts-ignore property active does not exist on domain + expect(domain.active).toBeFalsy(); + const hub = getCurrentHub(); + expect(hub).toEqual(new Hub()); + }); + + test('hub scope inheritance', () => { + const globalHub = getCurrentHub(); + globalHub.configureScope(scope => { + scope.setExtra('a', 'b'); + scope.setTag('a', 'b'); + scope.addBreadcrumb({ message: 'a' }); + }); + runWithAsyncContext(hub => { + expect(globalHub).toEqual(hub); + }); + }); + + test('hub single instance', () => { + setHooksAsyncContextStrategy(); + + runWithAsyncContext(hub => { + expect(hub).toBe(getCurrentHub()); + }); + }); + + test('context within context', () => { + setHooksAsyncContextStrategy(); + + runWithAsyncContext(hub1 => { + runWithAsyncContext(hub2 => { + expect(hub1).not.toBe(hub2); + }); + }); + }); + + test('context within context reused', () => { + setHooksAsyncContextStrategy(); + + runWithAsyncContext(hub1 => { + runWithAsyncContext( + hub2 => { + expect(hub1).toBe(hub2); + }, + { reuseExisting: true }, + ); + }); + }); + + test('concurrent contexts', done => { + setHooksAsyncContextStrategy(); + + let d1done = false; + let d2done = false; + + runWithAsyncContext(hub => { + hub.getStack().push({ client: 'process' } as any); + expect(hub.getStack()[1]).toEqual({ client: 'process' }); + // Just in case so we don't have to worry which one finishes first + // (although it always should be d2) + setTimeout(() => { + d1done = true; + if (d2done) { + done(); + } + }); + }); + + runWithAsyncContext(hub => { + hub.getStack().push({ client: 'local' } as any); + expect(hub.getStack()[1]).toEqual({ client: 'local' }); + setTimeout(() => { + d2done = true; + if (d1done) { + done(); + } + }); + }); + }); +}); From bdf8e25b1ddcf4633fbcf0188be394dcee2f1225 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 9 Apr 2023 14:46:32 +0200 Subject: [PATCH 2/4] Fix scope inheritance --- packages/core/src/hub.ts | 4 +-- packages/node/src/async/hooks.ts | 20 +++++------ packages/node/test/async/hooks.test.ts | 47 +++++++++++++++++--------- 3 files changed, 42 insertions(+), 29 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 905fb4f3bcde..7ca428f71c9e 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -575,10 +575,10 @@ function getGlobalHub(registry: Carrier = getMainCarrier()): Hub { * * If the carrier does not contain a hub, a new hub is created with the global hub client and scope. */ -export function ensureHubOnCarrier(carrier: Carrier): void { +export function ensureHubOnCarrier(carrier: Carrier, parent: Hub = getGlobalHub()): void { // If there's no hub on current domain, or it's an old API, assign a new one if (!hasHubOnCarrier(carrier) || getHubFromCarrier(carrier).isOlderThan(API_VERSION)) { - const globalHubTopStack = getGlobalHub().getStackTop(); + const globalHubTopStack = parent.getStackTop(); setHubOnCarrier(carrier, new Hub(globalHubTopStack.client, Scope.clone(globalHubTopStack.scope))); } } diff --git a/packages/node/src/async/hooks.ts b/packages/node/src/async/hooks.ts index 01f8f7ab9478..e0bd4a8fb2d4 100644 --- a/packages/node/src/async/hooks.ts +++ b/packages/node/src/async/hooks.ts @@ -34,24 +34,22 @@ export function setHooksAsyncContextStrategy(): void { return asyncStorage.getStore(); } - function createNewHub(): Hub { + function createNewHub(parent: Hub | undefined): Hub { const carrier: Carrier = {}; - ensureHubOnCarrier(carrier); + ensureHubOnCarrier(carrier, parent); return getHubFromCarrier(carrier); } function runWithAsyncContext(callback: (hub: Hub) => T, options: RunWithAsyncContextOptions): T { - if (options?.reuseExisting) { - const existingHub = getCurrentHub(); - - if (existingHub) { - // We're already in an async context, so we don't need to create a new one - // just call the callback with the current hub - return callback(existingHub); - } + const existingHub = getCurrentHub(); + + if (existingHub && options?.reuseExisting) { + // We're already in an async context, so we don't need to create a new one + // just call the callback with the current hub + return callback(existingHub); } - const newHub = createNewHub(); + const newHub = createNewHub(existingHub); return asyncStorage.run(newHub, () => { return callback(newHub); diff --git a/packages/node/test/async/hooks.test.ts b/packages/node/test/async/hooks.test.ts index 5e24223f7946..aed66abff2c2 100644 --- a/packages/node/test/async/hooks.test.ts +++ b/packages/node/test/async/hooks.test.ts @@ -1,34 +1,49 @@ import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; -import * as domain from 'domain'; import { setHooksAsyncContextStrategy } from '../../src/async/hooks'; -describe('async hooks', () => { +describe('async_hooks', () => { afterAll(() => { // clear the strategy setAsyncContextStrategy(undefined); }); - test('without hooks', () => { - // @ts-ignore property active does not exist on domain - expect(domain.active).toBeFalsy(); + test('without context', () => { const hub = getCurrentHub(); expect(hub).toEqual(new Hub()); }); + test('without strategy hubs should be equal', () => { + runWithAsyncContext(hub1 => { + runWithAsyncContext(hub2 => { + expect(hub1).toBe(hub2); + }); + }); + }); + test('hub scope inheritance', () => { + setHooksAsyncContextStrategy(); + const globalHub = getCurrentHub(); - globalHub.configureScope(scope => { - scope.setExtra('a', 'b'); - scope.setTag('a', 'b'); - scope.addBreadcrumb({ message: 'a' }); - }); - runWithAsyncContext(hub => { - expect(globalHub).toEqual(hub); + globalHub.setExtra('a', 'b'); + + runWithAsyncContext(hub1 => { + expect(hub1).toEqual(globalHub); + + hub1.setExtra('c', 'd'); + expect(hub1).not.toEqual(globalHub); + + runWithAsyncContext(hub2 => { + expect(hub2).toEqual(hub1); + expect(hub2).not.toEqual(globalHub); + + hub2.setExtra('e', 'f'); + expect(hub2).not.toEqual(hub1); + }); }); }); - test('hub single instance', () => { + test('context single instance', () => { setHooksAsyncContextStrategy(); runWithAsyncContext(hub => { @@ -36,7 +51,7 @@ describe('async hooks', () => { }); }); - test('context within context', () => { + test('context within a context not reused', () => { setHooksAsyncContextStrategy(); runWithAsyncContext(hub1 => { @@ -46,7 +61,7 @@ describe('async hooks', () => { }); }); - test('context within context reused', () => { + test('context within a context reused when requested', () => { setHooksAsyncContextStrategy(); runWithAsyncContext(hub1 => { @@ -59,7 +74,7 @@ describe('async hooks', () => { }); }); - test('concurrent contexts', done => { + test('concurrent hub contexts', done => { setHooksAsyncContextStrategy(); let d1done = false; From 2a3007c760a1b195626e361d6fbe43f29b3464fe Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 9 Apr 2023 17:16:39 +0200 Subject: [PATCH 3/4] Only run hooks test when node >= v12 --- packages/node/test/async/hooks.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/test/async/hooks.test.ts b/packages/node/test/async/hooks.test.ts index aed66abff2c2..dbd1904c34dc 100644 --- a/packages/node/test/async/hooks.test.ts +++ b/packages/node/test/async/hooks.test.ts @@ -1,8 +1,9 @@ import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; import { setHooksAsyncContextStrategy } from '../../src/async/hooks'; +import { conditionalTest } from '../utils'; -describe('async_hooks', () => { +conditionalTest({ min: 12 })('async_hooks', () => { afterAll(() => { // clear the strategy setAsyncContextStrategy(undefined); From 94eb4bfeb9f277f81085eef34ed393223a47aac2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 9 Apr 2023 19:06:27 +0200 Subject: [PATCH 4/4] Remove require --- packages/node/src/async/hooks.ts | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/packages/node/src/async/hooks.ts b/packages/node/src/async/hooks.ts index e0bd4a8fb2d4..840236d5b2c1 100644 --- a/packages/node/src/async/hooks.ts +++ b/packages/node/src/async/hooks.ts @@ -1,34 +1,22 @@ import type { Carrier, Hub, RunWithAsyncContextOptions } from '@sentry/core'; -import { - ensureHubOnCarrier, - // getCurrentHub as getCurrentHubCore, - getHubFromCarrier, - setAsyncContextStrategy, -} from '@sentry/core'; +import { ensureHubOnCarrier, getHubFromCarrier, setAsyncContextStrategy } from '@sentry/core'; +import * as async_hooks from 'async_hooks'; interface AsyncLocalStorage { - disable(): void; getStore(): T | undefined; // eslint-disable-next-line @typescript-eslint/no-explicit-any run(store: T, callback: (...args: TArgs) => R, ...args: TArgs): R; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - exit(callback: (...args: TArgs) => R, ...args: TArgs): R; - enterWith(store: T): void; } -function createAsyncLocalStorage(): AsyncLocalStorage { - // async_hooks does not exist before Node v12.17 - - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { AsyncLocalStorage } = require('async_hooks'); - return new AsyncLocalStorage(); -} +type AsyncLocalStorageConstructor = { new (): AsyncLocalStorage }; +// AsyncLocalStorage only exists in async_hook after Node v12.17.0 or v13.10.0 +type NewerAsyncHooks = typeof async_hooks & { AsyncLocalStorage: AsyncLocalStorageConstructor }; /** - * Sets the async context strategy to use Node.js async_hooks. + * Sets the async context strategy to use AsyncLocalStorage which requires Node v12.17.0 or v13.10.0. */ export function setHooksAsyncContextStrategy(): void { - const asyncStorage = createAsyncLocalStorage(); + const asyncStorage = new (async_hooks as NewerAsyncHooks).AsyncLocalStorage(); function getCurrentHub(): Hub | undefined { return asyncStorage.getStore();