From f0a637a3f9a4c7d53b969871a2951e8e4a0d60b5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 5 Apr 2023 23:46:59 +0200 Subject: [PATCH 1/6] Adds `domain` `AsyncContextStrategy` --- packages/core/src/hub.ts | 28 ++++++++++++++++++++---- packages/core/src/index.ts | 1 + packages/node/src/async.ts | 45 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 packages/node/src/async.ts diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 1a2358b81555..403be8545483 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -536,19 +536,39 @@ export function getCurrentHub(): Hub { } } + // Prefer domains over global if they are there (applicable only to Node environment) + if (isNodeEnv()) { + return getHubFromActiveDomain(registry); + } + + // Return hub that lives on a global object + return getGlobalHub(registry); +} + +function getGlobalHub(registry: Carrier = getMainCarrier()): Hub { // If there's no hub, or its an old API, assign a new one if (!hasHubOnCarrier(registry) || getHubFromCarrier(registry).isOlderThan(API_VERSION)) { setHubOnCarrier(registry, new Hub()); } - // Prefer domains over global if they are there (applicable only to Node environment) - if (isNodeEnv()) { - return getHubFromActiveDomain(registry); - } // Return hub that lives on a global object return getHubFromCarrier(registry); } +/** + * @private Private API with no semver guarantees! + * + * Sets the hub on the supplied carrier. If the carrier does not contain a hub, a new hub is created with the global + * hubs client and scope. + */ +export function ensureHubOnCarrier(carrier: Carrier): 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(); + setHubOnCarrier(carrier, new Hub(globalHubTopStack.client, Scope.clone(globalHubTopStack.scope))); + } +} + /** * @private Private API with no semver guarantees! * diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ee8e624709a0..fd43f4565f88 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -26,6 +26,7 @@ export { getMainCarrier, runWithAsyncContext, setHubOnCarrier, + ensureHubOnCarrier, setAsyncContextStrategy, } from './hub'; export { makeSession, closeSession, updateSession } from './session'; diff --git a/packages/node/src/async.ts b/packages/node/src/async.ts new file mode 100644 index 000000000000..32a1e11c7dca --- /dev/null +++ b/packages/node/src/async.ts @@ -0,0 +1,45 @@ +import type { Carrier, Hub } from '@sentry/core'; +import { + ensureHubOnCarrier, + getCurrentHub as getCurrentHubCore, + getHubFromCarrier, + setAsyncContextStrategy, +} from '@sentry/core'; +import * as domain from 'domain'; +import EventEmitter from 'events'; + +/** + * Sets the async context strategy to use Node.js domains. + */ +export function setDomainAsyncContextStrategy(): void { + function getCurrentHub(): Hub | undefined { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + const activeDomain = (domain as any).active as Carrier; + + // If there's no active domain, just return undefined and the global hub will be used + if (!activeDomain) { + return undefined; + } + + ensureHubOnCarrier(activeDomain); + + return getHubFromCarrier(activeDomain); + } + + function runWithAsyncContext(callback: (hub: Hub, ...args: A[]) => T, ...args: A[]): T { + const local = domain.create(); + + for (const emitter of args) { + if (emitter instanceof EventEmitter) { + local.add(emitter); + } + } + + return local.bind(() => { + const hub = getCurrentHubCore(); + return callback(hub, ...args); + })(); + } + + setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext }); +} From 9f44abf15379f47953dae716651129bb1b86d98e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 6 Apr 2023 00:59:28 +0200 Subject: [PATCH 2/6] Use type --- packages/node/src/async.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/async.ts b/packages/node/src/async.ts index 32a1e11c7dca..d20cf93e7c7b 100644 --- a/packages/node/src/async.ts +++ b/packages/node/src/async.ts @@ -6,7 +6,7 @@ import { setAsyncContextStrategy, } from '@sentry/core'; import * as domain from 'domain'; -import EventEmitter from 'events'; +import { EventEmitter } from 'events'; /** * Sets the async context strategy to use Node.js domains. From 9194dd797be03973262980b09b1c0df799a52003 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 6 Apr 2023 13:06:33 +0200 Subject: [PATCH 3/6] Simplify a little --- packages/core/src/hub.ts | 6 ++-- packages/node/src/async.ts | 45 --------------------------- packages/node/src/async/domain.ts | 51 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 48 deletions(-) delete mode 100644 packages/node/src/async.ts create mode 100644 packages/node/src/async/domain.ts diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 403be8545483..eb65a7496352 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -55,7 +55,7 @@ const DEFAULT_BREADCRUMBS = 100; */ export interface AsyncContextStrategy { getCurrentHub: () => Hub | undefined; - runWithAsyncContext(callback: (hub: Hub, ...args: A[]) => T, ...args: A[]): T; + runWithAsyncContext(callback: (hub: Hub) => T, ...args: unknown[]): T; } /** @@ -586,7 +586,7 @@ export function setAsyncContextStrategy(strategy: AsyncContextStrategy): void { * * Runs the given callback function with the global async context strategy */ -export function runWithAsyncContext(callback: (hub: Hub, ...args: A[]) => T, ...args: A[]): T { +export function runWithAsyncContext(callback: (hub: Hub) => T, ...args: unknown[]): T { const registry = getMainCarrier(); if (registry.__SENTRY__ && registry.__SENTRY__.acs) { @@ -594,7 +594,7 @@ export function runWithAsyncContext(callback: (hub: Hub, ...args: A[]) => } // if there was no strategy, fallback to just calling the callback - return callback(getCurrentHub(), ...args); + return callback(getCurrentHub()); } /** diff --git a/packages/node/src/async.ts b/packages/node/src/async.ts deleted file mode 100644 index d20cf93e7c7b..000000000000 --- a/packages/node/src/async.ts +++ /dev/null @@ -1,45 +0,0 @@ -import type { Carrier, Hub } from '@sentry/core'; -import { - ensureHubOnCarrier, - getCurrentHub as getCurrentHubCore, - getHubFromCarrier, - setAsyncContextStrategy, -} from '@sentry/core'; -import * as domain from 'domain'; -import { EventEmitter } from 'events'; - -/** - * Sets the async context strategy to use Node.js domains. - */ -export function setDomainAsyncContextStrategy(): void { - function getCurrentHub(): Hub | undefined { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - const activeDomain = (domain as any).active as Carrier; - - // If there's no active domain, just return undefined and the global hub will be used - if (!activeDomain) { - return undefined; - } - - ensureHubOnCarrier(activeDomain); - - return getHubFromCarrier(activeDomain); - } - - function runWithAsyncContext(callback: (hub: Hub, ...args: A[]) => T, ...args: A[]): T { - const local = domain.create(); - - for (const emitter of args) { - if (emitter instanceof EventEmitter) { - local.add(emitter); - } - } - - return local.bind(() => { - const hub = getCurrentHubCore(); - return callback(hub, ...args); - })(); - } - - setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext }); -} diff --git a/packages/node/src/async/domain.ts b/packages/node/src/async/domain.ts new file mode 100644 index 000000000000..4251a4b27b18 --- /dev/null +++ b/packages/node/src/async/domain.ts @@ -0,0 +1,51 @@ +import type { Carrier, Hub } from '@sentry/core'; +import { + ensureHubOnCarrier, + getCurrentHub as getCurrentHubCore, + getHubFromCarrier, + setAsyncContextStrategy, +} from '@sentry/core'; +import * as domain from 'domain'; +import { EventEmitter } from 'events'; + +/** + * Returns the current hub. + */ +export function getCurrentHub(): Hub | undefined { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + const activeDomain = (domain as any).active as Carrier; + + // If there's no active domain, just return undefined and the global hub will be used + if (!activeDomain) { + return undefined; + } + + ensureHubOnCarrier(activeDomain); + + return getHubFromCarrier(activeDomain); +} + +/** + * Runs the given function inside a domain. + */ +export function runWithAsyncContext(callback: (hub: Hub) => T, ...args: A[]): T { + const local = domain.create(); + + for (const emitter of args) { + if (emitter instanceof EventEmitter) { + local.add(emitter); + } + } + + return local.bind(() => { + const hub = getCurrentHubCore(); + return callback(hub); + })(); +} + +/** + * Sets the async context strategy to use Node.js domains. + */ +export function setDomainAsyncContextStrategy(): void { + setAsyncContextStrategy({ getCurrentHub, runWithAsyncContext }); +} From 04dfc00d27152794b1628292b9a188df2a4f330f Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 6 Apr 2023 14:09:56 +0200 Subject: [PATCH 4/6] Add tests --- packages/core/src/hub.ts | 2 +- packages/node/src/async/domain.ts | 10 +--- packages/node/test/async/domain.test.ts | 71 +++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 packages/node/test/async/domain.test.ts diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index eb65a7496352..2515af7ac575 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -574,7 +574,7 @@ export function ensureHubOnCarrier(carrier: Carrier): void { * * Sets the global async context strategy */ -export function setAsyncContextStrategy(strategy: AsyncContextStrategy): void { +export function setAsyncContextStrategy(strategy: AsyncContextStrategy | undefined): void { // Get main carrier (global for every environment) const registry = getMainCarrier(); registry.__SENTRY__ = registry.__SENTRY__ || {}; diff --git a/packages/node/src/async/domain.ts b/packages/node/src/async/domain.ts index 4251a4b27b18..a63dac92849f 100644 --- a/packages/node/src/async/domain.ts +++ b/packages/node/src/async/domain.ts @@ -8,10 +8,7 @@ import { import * as domain from 'domain'; import { EventEmitter } from 'events'; -/** - * Returns the current hub. - */ -export function getCurrentHub(): Hub | undefined { +function getCurrentHub(): Hub | undefined { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any const activeDomain = (domain as any).active as Carrier; @@ -25,10 +22,7 @@ export function getCurrentHub(): Hub | undefined { return getHubFromCarrier(activeDomain); } -/** - * Runs the given function inside a domain. - */ -export function runWithAsyncContext(callback: (hub: Hub) => T, ...args: A[]): T { +function runWithAsyncContext(callback: (hub: Hub) => T, ...args: A[]): T { const local = domain.create(); for (const emitter of args) { diff --git a/packages/node/test/async/domain.test.ts b/packages/node/test/async/domain.test.ts new file mode 100644 index 000000000000..1cb4adbe4c4d --- /dev/null +++ b/packages/node/test/async/domain.test.ts @@ -0,0 +1,71 @@ +import { getCurrentHub, Hub, runWithAsyncContext, setAsyncContextStrategy } from '@sentry/core'; +import * as domain from 'domain'; + +import { setDomainAsyncContextStrategy } from '../../src/async/domain'; + +describe('domains', () => { + afterAll(() => { + // clear the strategy + setAsyncContextStrategy(undefined); + }); + + test('without domain', () => { + // @ts-ignore property active does not exist on domain + expect(domain.active).toBeFalsy(); + const hub = getCurrentHub(); + expect(hub).toEqual(new Hub()); + }); + + test('domain 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('domain hub single instance', () => { + setDomainAsyncContextStrategy(); + + runWithAsyncContext(hub => { + expect(hub).toBe(getCurrentHub()); + }); + }); + + test('concurrent domain hubs', done => { + setDomainAsyncContextStrategy(); + + getCurrentHub(); + + 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 f2a1e92d74ac3869428bb4a2b1043d8bc201873e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 6 Apr 2023 14:12:01 +0200 Subject: [PATCH 5/6] Fix jsdoc --- packages/core/src/hub.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 2515af7ac575..2168ebb3a490 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -558,8 +558,7 @@ function getGlobalHub(registry: Carrier = getMainCarrier()): Hub { /** * @private Private API with no semver guarantees! * - * Sets the hub on the supplied carrier. If the carrier does not contain a hub, a new hub is created with the global - * hubs client and scope. + * 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 { // If there's no hub on current domain, or it's an old API, assign a new one From e3e02062c90e008eb115112e99e817b087be086b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 6 Apr 2023 14:15:20 +0200 Subject: [PATCH 6/6] Remove redundant call --- packages/node/test/async/domain.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/node/test/async/domain.test.ts b/packages/node/test/async/domain.test.ts index 1cb4adbe4c4d..e12fb590d014 100644 --- a/packages/node/test/async/domain.test.ts +++ b/packages/node/test/async/domain.test.ts @@ -39,8 +39,6 @@ describe('domains', () => { test('concurrent domain hubs', done => { setDomainAsyncContextStrategy(); - getCurrentHub(); - let d1done = false; let d2done = false;