diff --git a/src/common/scope.ts b/src/common/scope.ts index 5651a83b..2680e155 100644 --- a/src/common/scope.ts +++ b/src/common/scope.ts @@ -12,6 +12,17 @@ export function getScopeData(): ScopeData { return globalScope; } +/** + * Calls back on any change to the global, isolation or current scope + * without building the merged scope. Use this when the callback reads + * the scope lazily, so bursts of changes cost nothing until it does. + */ +export function onScopeChange(callback: () => void): void { + getGlobalScope().addScopeListener(callback); + getIsolationScope().addScopeListener(callback); + getCurrentScope().addScopeListener(callback); +} + /** Hooks both current and isolation scope changes and passes merged scope on changes */ export function addScopeListener(callback: (merged: ScopeData, changed: Scope) => void): void { getIsolationScope().addScopeListener((isolation) => { diff --git a/src/main/context.ts b/src/main/context.ts index 4282ec1a..d111a99a 100644 --- a/src/main/context.ts +++ b/src/main/context.ts @@ -32,6 +32,12 @@ export function getDefaultEnvironment(): string { return app.isPackaged ? 'production' : 'development'; } +/** + * How long scope saves may reuse the event defaults. Building them runs + * every event processor and some are slow, such as `app.getAppMetrics()`. + */ +export const EVENT_DEFAULTS_TTL_MS = 5_000; + /** * Gets the default event data by calling all event processors. */ diff --git a/src/main/integrations/electron-minidump.ts b/src/main/integrations/electron-minidump.ts index 239d55d2..2de2f91c 100644 --- a/src/main/integrations/electron-minidump.ts +++ b/src/main/integrations/electron-minidump.ts @@ -2,11 +2,12 @@ import type { Event, ScopeData } from '@sentry/core'; import { applyScopeDataToEvent, debug, defineIntegration, makeDsn, uuid4 } from '@sentry/core'; import type { NodeClient, NodeOptions } from '@sentry/node'; import { app, crashReporter } from 'electron'; -import { addScopeListener, getScopeData } from '../../common/scope.js'; -import { getEventDefaults, getSdkInfo } from '../context.js'; +import { getScopeData, onScopeChange } from '../../common/scope.js'; +import { EVENT_DEFAULTS_TTL_MS, getEventDefaults, getSdkInfo } from '../context.js'; import { CRASH_REASONS } from '../electron-normalize.js'; import { mergeEvents } from '../merge.js'; import { normalizePaths } from '../normalize.js'; +import { createScopeSaver } from '../scope-updates.js'; import { checkPreviousSession, sessionCrashed, unreportedDuringLastSession } from '../sessions.js'; /** Is object defined and has keys */ @@ -93,14 +94,12 @@ export function minidumpUrlFromDsn(dsn: string): string | undefined { * Sends minidumps via the Electron built-in uploader. */ export const electronMinidumpIntegration = defineIntegration(() => { - /** Counter used to ensure no race condition when updating extra params */ - let updateEpoch = 0; let lastParamCount = 0; - async function getNativeUploaderEvent(client: NodeClient, scope: ScopeData): Promise { + function getNativeUploaderEvent(client: NodeClient, scope: ScopeData, defaults: Event): Event { const { userInfo: inferIpAddress } = client.getDataCollectionOptions(); - const event = mergeEvents(await getEventDefaults(client), { + const event = mergeEvents(defaults, { sdk: getSdkInfo(inferIpAddress), event_id: uuid4(), level: 'fatal', @@ -116,35 +115,26 @@ export const electronMinidumpIntegration = defineIntegration(() => { return normalizePaths(event, app.getAppPath()); } - function updateExtraParams(client: NodeClient, scope: ScopeData): void { - updateEpoch += 1; - const currentEpoch = updateEpoch; + function updateExtraParams(client: NodeClient, scope: ScopeData, defaults: Event): void { + const event = getNativeUploaderEvent(client, scope, defaults); - getNativeUploaderEvent(client, scope) - .then((event) => { - if (currentEpoch !== updateEpoch) { - return; - } - - // Update the extra parameters in the main process - const mainParams = getNativeUploaderExtraParams(event); - const count = Object.keys(mainParams).length; + // Update the extra parameters in the main process + const mainParams = getNativeUploaderExtraParams(event); + const count = Object.keys(mainParams).length; - // Remove any extra parameters that extend beyond the current count - if (lastParamCount > count) { - for (let i = count + 1; i <= lastParamCount; i++) { - crashReporter.removeExtraParameter(`sentry__${i}`); - } - } + // Remove any extra parameters that extend beyond the current count + if (lastParamCount > count) { + for (let i = count + 1; i <= lastParamCount; i++) { + crashReporter.removeExtraParameter(`sentry__${i}`); + } + } - for (const [key, value] of Object.entries(mainParams)) { - crashReporter.addExtraParameter(key, value); - } + for (const [key, value] of Object.entries(mainParams)) { + crashReporter.addExtraParameter(key, value); + } - // Track how many parameters we added so we can remove extras next time - lastParamCount = count; - }) - .catch((error) => debug.error(error)); + // Track how many parameters we added so we can remove extras next time + lastParamCount = count; } function startCrashReporter(options: NodeOptions): void { @@ -172,9 +162,16 @@ export const electronMinidumpIntegration = defineIntegration(() => { } function setupScopeListener(client: NodeClient): void { - addScopeListener((scope) => { - updateExtraParams(client, scope); - }); + const saver = createScopeSaver( + getScopeData, + () => getEventDefaults(client), + (scope, defaults) => updateExtraParams(client, scope, defaults), + EVENT_DEFAULTS_TTL_MS, + ); + + onScopeChange(saver.save); + // Screen and GPU context only exist once the app is ready + void app.whenReady().then(saver.refresh); } return { diff --git a/src/main/integrations/sentry-minidump/index.ts b/src/main/integrations/sentry-minidump/index.ts index 61c3d2ce..dfa5f163 100644 --- a/src/main/integrations/sentry-minidump/index.ts +++ b/src/main/integrations/sentry-minidump/index.ts @@ -3,10 +3,11 @@ import { applyScopeDataToEvent, captureEvent, createStackParser, debug, defineIn import type { NodeClient } from '@sentry/node'; import { chromeStackLineParser } from '@sentry/browser'; import { app, crashReporter } from 'electron'; -import { addScopeListener, getScopeData } from '../../../common/scope.js'; -import { getEventDefaults } from '../../context.js'; +import { getScopeData, onScopeChange } from '../../../common/scope.js'; +import { EVENT_DEFAULTS_TTL_MS, getEventDefaults } from '../../context.js'; import { EXIT_REASONS, getSentryCachePath } from '../../electron-normalize.js'; import { getRendererProperties, trackRendererProperties } from '../../renderers.js'; +import { createScopeSaver } from '../../scope-updates.js'; import type { ElectronMainOptions } from '../../sdk.js'; import { previousSessionWasAbnormal, restorePreviousSession, setPreviousSessionAsCurrent } from '../../sessions.js'; import { BufferedWriteStore } from '../../store.js'; @@ -58,22 +59,17 @@ export const sentryMinidumpIntegration = defineIntegration((options: Options = { } function setupScopeListener(client: NodeClient): void { - function scopeChanged(scope: ScopeData): void { - // Since the initial scope read is async, we need to ensure that any writes do not beat that - // https://github.com/getsentry/sentry-electron/issues/585 - setImmediate(async () => - scopeStore?.set({ - scope, - event: await getEventDefaults(client), - }), - ); - } - - addScopeListener((scope) => { - scopeChanged(scope); - }); - - scopeChanged(getScopeData()); + const saver = createScopeSaver( + getScopeData, + () => getEventDefaults(client), + (scope, event) => scopeStore?.set({ scope, event }), + EVENT_DEFAULTS_TTL_MS, + ); + + onScopeChange(saver.save); + // Screen and GPU context only exist once the app is ready + void app.whenReady().then(saver.refresh); + saver.save(); } async function sendNativeCrashes( diff --git a/src/main/scope-updates.ts b/src/main/scope-updates.ts new file mode 100644 index 00000000..54fe4933 --- /dev/null +++ b/src/main/scope-updates.ts @@ -0,0 +1,80 @@ +import { debug } from '@sentry/core'; + +export interface ScopeSaver { + /** Saves the current scope on the next event loop turn */ + save: () => void; + /** Drops the cached defaults and saves the current scope */ + refresh: () => void; +} + +/** + * Creates a saver that writes the current scope together with event + * defaults that are at most `ttlMs` old. + * + * Calls in one event loop turn are merged into one save on the next + * turn, and the scope is read once when that save runs. The defaults + * are rebuilt only when a save finds them expired, or after `refresh`. + */ +export function createScopeSaver( + getScope: () => TScope, + getDefaults: () => Promise, + save: (scope: TScope, defaults: TDefaults) => void | Promise, + ttlMs: number, +): ScopeSaver { + let scheduled = false; + let runs = 0; + let defaults: Promise | undefined; + let expires = 0; + + function schedule(): void { + if (scheduled) { + return; + } + + scheduled = true; + + // Deferring to the next turn ensures that the first save does not + // beat the initial async read of the store + // https://github.com/getsentry/sentry-electron/issues/585 + setImmediate(() => { + run().catch((error) => debug.error(error)); + }); + } + + async function run(): Promise { + scheduled = false; + const thisRun = ++runs; + const scope = getScope(); + const now = Date.now(); + + if (!defaults || now >= expires) { + const current = getDefaults(); + defaults = current; + expires = now + ttlMs; + + current.catch(() => { + if (defaults === current) { + defaults = undefined; + } + }); + } + + const resolved = await defaults; + + // A newer run holds a newer scope, so a slow older one must not + // write over it + if (thisRun !== runs) { + return; + } + + await save(scope, resolved); + } + + return { + save: schedule, + refresh: () => { + defaults = undefined; + schedule(); + }, + }; +} diff --git a/test/unit/scope-updates.test.ts b/test/unit/scope-updates.test.ts new file mode 100644 index 00000000..36760df8 --- /dev/null +++ b/test/unit/scope-updates.test.ts @@ -0,0 +1,163 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; +import { createScopeSaver } from '../../src/main/scope-updates'; + +const TTL = 1000; + +describe('createScopeSaver', () => { + let scope = 0; + const getScope = vi.fn(() => scope); + + beforeEach(() => { + vi.useFakeTimers(); + scope = 0; + getScope.mockClear(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + test('merges synchronous changes into one save and reads the scope once', async () => { + const getDefaults = vi.fn().mockResolvedValue('defaults'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + for (let i = 0; i < 1000; i++) { + scope = i; + saver.save(); + } + + expect(getScope).not.toHaveBeenCalled(); + expect(save).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(0); + expect(getScope).toHaveBeenCalledTimes(1); + expect(getDefaults).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledWith(999, 'defaults'); + }); + + test('saves on each turn but builds the defaults once within the ttl', async () => { + const getDefaults = vi.fn().mockResolvedValue('defaults'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + for (let i = 0; i < 10; i++) { + scope = i; + saver.save(); + await vi.advanceTimersByTimeAsync(10); + } + + expect(save).toHaveBeenCalledTimes(10); + expect(save).toHaveBeenLastCalledWith(9, 'defaults'); + expect(getDefaults).toHaveBeenCalledTimes(1); + }); + + test('rebuilds the defaults when a save finds them expired', async () => { + const getDefaults = vi.fn().mockResolvedValueOnce('first').mockResolvedValueOnce('second'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + scope = 1; + saver.save(); + await vi.advanceTimersByTimeAsync(TTL); + scope = 2; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + + expect(save).toHaveBeenNthCalledWith(1, 1, 'first'); + expect(save).toHaveBeenNthCalledWith(2, 2, 'second'); + }); + + test('refresh rebuilds the defaults and saves without a scope change', async () => { + const getDefaults = vi.fn().mockResolvedValueOnce('first').mockResolvedValueOnce('second'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + scope = 1; + saver.save(); + await vi.advanceTimersByTimeAsync(100); + expect(save).toHaveBeenLastCalledWith(1, 'first'); + + saver.refresh(); + await vi.advanceTimersByTimeAsync(0); + expect(save).toHaveBeenCalledTimes(2); + expect(save).toHaveBeenLastCalledWith(1, 'second'); + expect(getDefaults).toHaveBeenCalledTimes(2); + }); + + test('a save in the same turn as refresh is merged into it', async () => { + const getDefaults = vi.fn().mockResolvedValueOnce('first').mockResolvedValueOnce('second'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + saver.save(); + await vi.advanceTimersByTimeAsync(0); + + scope = 2; + saver.save(); + saver.refresh(); + await vi.advanceTimersByTimeAsync(0); + + expect(save).toHaveBeenCalledTimes(2); + expect(save).toHaveBeenLastCalledWith(2, 'second'); + }); + + test('a slow older run does not write over a newer save', async () => { + let resolveFirst: (value: string) => void = () => undefined; + const getDefaults = vi + .fn<() => Promise>() + .mockImplementationOnce(() => new Promise((resolve) => (resolveFirst = resolve))) + .mockResolvedValue('second'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + scope = 1; + saver.save(); + await vi.advanceTimersByTimeAsync(TTL); + expect(save).not.toHaveBeenCalled(); + + scope = 2; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + expect(save).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledWith(2, 'second'); + + resolveFirst('first'); + await vi.advanceTimersByTimeAsync(0); + expect(save).toHaveBeenCalledTimes(1); + }); + + test('does not cache rejected defaults', async () => { + const getDefaults = vi.fn().mockRejectedValueOnce(new Error('nope')).mockResolvedValue('defaults'); + const save = vi.fn(); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + scope = 1; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + expect(save).not.toHaveBeenCalled(); + + scope = 2; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + expect(save).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledWith(2, 'defaults'); + expect(getDefaults).toHaveBeenCalledTimes(2); + }); + + test('a rejecting save does not break later saves', async () => { + const getDefaults = vi.fn().mockResolvedValue('defaults'); + const save = vi.fn().mockRejectedValueOnce(new Error('nope')).mockResolvedValue(undefined); + const saver = createScopeSaver(getScope, getDefaults, save, TTL); + + scope = 1; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + scope = 2; + saver.save(); + await vi.advanceTimersByTimeAsync(0); + + expect(save).toHaveBeenCalledTimes(2); + expect(save).toHaveBeenLastCalledWith(2, 'defaults'); + }); +});