From 3bc8532d16f89b8660ef14358b6278feefa0e939 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 15 Sep 2025 12:59:12 +0200 Subject: [PATCH 1/2] ref: Avoid some usage of `SyncPromise` where not needed We make quite heavy use of it in event processing, not touching this for now, but in other places it should not be needed IMHO. fix transport using async refactor some more sync promise stuff fix node client fix some flushing stuff fix check oops small fixees fix tests fix lint fix to promise like small ref avoid special casing of no fetch implementation fetch stuff --- .../test/profiling/integration.test.ts | 7 +- packages/bun/src/transports/index.ts | 4 +- packages/core/src/client.ts | 74 ++++++++++--------- packages/core/src/transports/base.ts | 5 +- packages/core/test/lib/client.test.ts | 18 +---- packages/deno/src/integrations/deno-cron.ts | 2 - packages/deno/src/transports/index.ts | 4 +- packages/node-core/src/sdk/client.ts | 23 +++--- packages/node-core/test/sdk/client.test.ts | 4 +- .../src/util/sendReplayRequest.ts | 6 +- 10 files changed, 65 insertions(+), 82 deletions(-) diff --git a/packages/browser/test/profiling/integration.test.ts b/packages/browser/test/profiling/integration.test.ts index baf0b5f64d14..2af3cb662689 100644 --- a/packages/browser/test/profiling/integration.test.ts +++ b/packages/browser/test/profiling/integration.test.ts @@ -2,7 +2,6 @@ * @vitest-environment jsdom */ -import type { BrowserClient } from '@sentry/browser'; import * as Sentry from '@sentry/browser'; import { describe, expect, it, vi } from 'vitest'; import type { JSSelfProfile } from '../../src/profiling/jsSelfProfiling'; @@ -36,7 +35,7 @@ describe('BrowserProfilingIntegration', () => { const flush = vi.fn().mockImplementation(() => Promise.resolve(true)); const send = vi.fn().mockImplementation(() => Promise.resolve()); - Sentry.init({ + const client = Sentry.init({ tracesSampleRate: 1, profilesSampleRate: 1, environment: 'test-environment', @@ -50,13 +49,11 @@ describe('BrowserProfilingIntegration', () => { integrations: [Sentry.browserTracingIntegration(), Sentry.browserProfilingIntegration()], }); - const client = Sentry.getClient(); - const currentTransaction = Sentry.getActiveSpan(); expect(currentTransaction).toBeDefined(); expect(Sentry.spanToJSON(currentTransaction!).op).toBe('pageload'); currentTransaction?.end(); - await client?.flush(1000); + await client!.flush(1000); expect(send).toHaveBeenCalledTimes(1); diff --git a/packages/bun/src/transports/index.ts b/packages/bun/src/transports/index.ts index efe10ab22646..7a27846548b3 100644 --- a/packages/bun/src/transports/index.ts +++ b/packages/bun/src/transports/index.ts @@ -1,5 +1,5 @@ import type { BaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/core'; -import { createTransport, rejectedSyncPromise, suppressTracing } from '@sentry/core'; +import { createTransport, suppressTracing } from '@sentry/core'; export interface BunTransportOptions extends BaseTransportOptions { /** Custom headers for the transport. Used by the XHRTransport and FetchTransport */ @@ -30,7 +30,7 @@ export function makeFetchTransport(options: BunTransportOptions): Transport { }); }); } catch (e) { - return rejectedSyncPromise(e); + return Promise.reject(e); } } diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 75a0d5ed49d2..91c29b86e24d 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -44,7 +44,7 @@ import { parseSampleRate } from './utils/parseSampleRate'; import { prepareEvent } from './utils/prepareEvent'; import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span'; import { getActiveSpan, showSpanDropWarning, spanToTraceContext } from './utils/spanUtils'; -import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from './utils/syncpromise'; +import { rejectedSyncPromise } from './utils/syncpromise'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; @@ -316,16 +316,19 @@ export abstract class Client { * @returns A promise that will resolve with `true` if all events are sent before the timeout, or `false` if there are * still events in the queue when the timeout is reached. */ - public flush(timeout?: number): PromiseLike { + // @ts-expect-error - PromiseLike is a subset of Promise + public async flush(timeout?: number): PromiseLike { const transport = this._transport; - if (transport) { - this.emit('flush'); - return this._isClientDoneProcessing(timeout).then(clientFinished => { - return transport.flush(timeout).then(transportFlushed => clientFinished && transportFlushed); - }); - } else { - return resolvedSyncPromise(true); + if (!transport) { + return Promise.resolve(true); } + + this.emit('flush'); + + const clientFinished = await this._isClientDoneProcessing(timeout); + const transportFlushed = await transport.flush(timeout); + + return clientFinished && transportFlushed; } /** @@ -336,12 +339,12 @@ export abstract class Client { * @returns {Promise} A promise which resolves to `true` if the flush completes successfully before the timeout, or `false` if * it doesn't. */ - public close(timeout?: number): PromiseLike { - return this.flush(timeout).then(result => { - this.getOptions().enabled = false; - this.emit('close'); - return result; - }); + // @ts-expect-error - PromiseLike is a subset of Promise + public async close(timeout?: number): PromiseLike { + const result = await this.flush(timeout); + this.getOptions().enabled = false; + this.emit('close'); + return result; } /** @@ -872,18 +875,21 @@ export abstract class Client { /** * Send an envelope to Sentry. */ - public sendEnvelope(envelope: Envelope): PromiseLike { + // @ts-expect-error - PromiseLike is a subset of Promise + public async sendEnvelope(envelope: Envelope): PromiseLike { this.emit('beforeEnvelope', envelope); if (this._isEnabled() && this._transport) { - return this._transport.send(envelope).then(null, reason => { + try { + return await this._transport.send(envelope); + } catch (reason) { DEBUG_BUILD && debug.error('Error while sending envelope:', reason); return {}; - }); + } } DEBUG_BUILD && debug.error('Transport disabled'); - return resolvedSyncPromise({}); + return {}; } /* eslint-enable @typescript-eslint/unified-signatures */ @@ -938,24 +944,20 @@ export abstract class Client { * @returns A promise which will resolve to `true` if processing is already done or finishes before the timeout, and * `false` otherwise */ - protected _isClientDoneProcessing(timeout?: number): PromiseLike { - return new SyncPromise(resolve => { - let ticked: number = 0; - const tick: number = 1; + protected async _isClientDoneProcessing(timeout?: number): Promise { + let ticked = 0; - const interval = setInterval(() => { - if (this._numProcessing == 0) { - clearInterval(interval); - resolve(true); - } else { - ticked += tick; - if (timeout && ticked >= timeout) { - clearInterval(interval); - resolve(false); - } - } - }, tick); - }); + // if no timeout is provided, we wait "forever" until everything is processed + while (!timeout || ticked < timeout) { + await new Promise(resolve => setTimeout(resolve, 1)); + + if (!this._numProcessing) { + return true; + } + ticked++; + } + + return false; } /** Determines whether this SDK is enabled and a transport is present. */ diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index c475c338db2f..822020070b86 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -16,7 +16,6 @@ import { } from '../utils/envelope'; import { type PromiseBuffer, makePromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from '../utils/promisebuffer'; import { type RateLimits, isRateLimited, updateRateLimits } from '../utils/ratelimit'; -import { resolvedSyncPromise } from '../utils/syncpromise'; export const DEFAULT_TRANSPORT_BUFFER_SIZE = 64; @@ -51,7 +50,7 @@ export function createTransport( // Skip sending if envelope is empty after filtering out rate limited events if (filteredEnvelopeItems.length === 0) { - return resolvedSyncPromise({}); + return Promise.resolve({}); } const filteredEnvelope: Envelope = createEnvelope(envelope[0], filteredEnvelopeItems as (typeof envelope)[1]); @@ -87,7 +86,7 @@ export function createTransport( if (error === SENTRY_BUFFER_FULL_ERROR) { DEBUG_BUILD && debug.error('Skipped sending event because buffer is full.'); recordEnvelopeLoss('queue_overflow'); - return resolvedSyncPromise({}); + return Promise.resolve({}); } else { throw error; } diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index b7767a7e6c58..b903b689fb70 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -2200,11 +2200,7 @@ describe('Client', () => { client.on('afterSendEvent', callback); client.sendEvent(errorEvent); - vi.runAllTimers(); - // Wait for two ticks - // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang - await undefined; - await undefined; + await vi.runAllTimersAsync(); expect(mockSend).toBeCalledTimes(1); expect(callback).toBeCalledTimes(1); @@ -2228,11 +2224,7 @@ describe('Client', () => { client.on('afterSendEvent', callback); client.sendEvent(transactionEvent); - vi.runAllTimers(); - // Wait for two ticks - // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang - await undefined; - await undefined; + await vi.runAllTimersAsync(); expect(mockSend).toBeCalledTimes(1); expect(callback).toBeCalledTimes(1); @@ -2260,11 +2252,7 @@ describe('Client', () => { client.on('afterSendEvent', callback); client.sendEvent(errorEvent); - vi.runAllTimers(); - // Wait for two ticks - // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang - await undefined; - await undefined; + await vi.runAllTimersAsync(); expect(mockSend).toBeCalledTimes(1); expect(callback).toBeCalledTimes(1); diff --git a/packages/deno/src/integrations/deno-cron.ts b/packages/deno/src/integrations/deno-cron.ts index ad856479aaee..b94c68bf967e 100644 --- a/packages/deno/src/integrations/deno-cron.ts +++ b/packages/deno/src/integrations/deno-cron.ts @@ -15,13 +15,11 @@ const _denoCronIntegration = (() => { return { name: INTEGRATION_NAME, setupOnce() { - // eslint-disable-next-line deprecation/deprecation if (!Deno.cron) { // The cron API is not available in this Deno version use --unstable flag! return; } - // eslint-disable-next-line deprecation/deprecation Deno.cron = new Proxy(Deno.cron, { apply(target, thisArg, argArray: CronParams) { const [monitorSlug, schedule, opt1, opt2] = argArray; diff --git a/packages/deno/src/transports/index.ts b/packages/deno/src/transports/index.ts index f6c0ed8d1c52..c5b6594b1c4d 100644 --- a/packages/deno/src/transports/index.ts +++ b/packages/deno/src/transports/index.ts @@ -1,5 +1,5 @@ import type { BaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/core'; -import { consoleSandbox, createTransport, debug, rejectedSyncPromise, suppressTracing } from '@sentry/core'; +import { consoleSandbox, createTransport, debug, suppressTracing } from '@sentry/core'; export interface DenoTransportOptions extends BaseTransportOptions { /** Custom headers for the transport. Used by the XHRTransport and FetchTransport */ @@ -48,7 +48,7 @@ export function makeFetchTransport(options: DenoTransportOptions): Transport { }); }); } catch (e) { - return rejectedSyncPromise(e); + return Promise.reject(e); } } diff --git a/packages/node-core/src/sdk/client.ts b/packages/node-core/src/sdk/client.ts index 0d7bea423ace..e631508c7392 100644 --- a/packages/node-core/src/sdk/client.ts +++ b/packages/node-core/src/sdk/client.ts @@ -77,9 +77,9 @@ export class NodeClient extends ServerRuntimeClient { return tracer; } - // Eslint ignore explanation: This is already documented in super. - // eslint-disable-next-line jsdoc/require-jsdoc - public async flush(timeout?: number): Promise { + /** @inheritDoc */ + // @ts-expect-error - PromiseLike is a subset of Promise + public async flush(timeout?: number): PromiseLike { await this.traceProvider?.forceFlush(); if (this.getOptions().sendClientReports) { @@ -89,9 +89,9 @@ export class NodeClient extends ServerRuntimeClient { return super.flush(timeout); } - // Eslint ignore explanation: This is already documented in super. - // eslint-disable-next-line jsdoc/require-jsdoc - public close(timeout?: number | undefined): PromiseLike { + /** @inheritDoc */ + // @ts-expect-error - PromiseLike is a subset of Promise + public async close(timeout?: number | undefined): PromiseLike { if (this._clientReportInterval) { clearInterval(this._clientReportInterval); } @@ -104,11 +104,12 @@ export class NodeClient extends ServerRuntimeClient { process.off('beforeExit', this._logOnExitFlushListener); } - return super - .close(timeout) - .then(allEventsSent => - this.traceProvider ? this.traceProvider.shutdown().then(() => allEventsSent) : allEventsSent, - ); + const allEventsSent = await super.close(timeout); + if (this.traceProvider) { + await this.traceProvider.shutdown(); + } + + return allEventsSent; } /** diff --git a/packages/node-core/test/sdk/client.test.ts b/packages/node-core/test/sdk/client.test.ts index 33548d621c13..01623f49f0a3 100644 --- a/packages/node-core/test/sdk/client.test.ts +++ b/packages/node-core/test/sdk/client.test.ts @@ -362,9 +362,7 @@ describe('NodeClient', () => { expect(result).toBe(true); - // once call directly in close to stop client reports, - // the other in core client `_isClientDoneProcessing` - expect(clearIntervalSpy).toHaveBeenCalledTimes(2); + expect(clearIntervalSpy).toHaveBeenCalledTimes(1); // removes `_clientReportOnExitFlushListener` expect(processOffSpy).toHaveBeenNthCalledWith(1, 'beforeExit', expect.any(Function)); diff --git a/packages/replay-internal/src/util/sendReplayRequest.ts b/packages/replay-internal/src/util/sendReplayRequest.ts index 5edb94f721f9..4f40934f37d3 100644 --- a/packages/replay-internal/src/util/sendReplayRequest.ts +++ b/packages/replay-internal/src/util/sendReplayRequest.ts @@ -1,5 +1,5 @@ import type { RateLimits, ReplayEvent, TransportMakeRequestResponse } from '@sentry/core'; -import { getClient, getCurrentScope, isRateLimited, resolvedSyncPromise, updateRateLimits } from '@sentry/core'; +import { getClient, getCurrentScope, isRateLimited, updateRateLimits } from '@sentry/core'; import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; import { DEBUG_BUILD } from '../debug-build'; import type { SendReplayData } from '../types'; @@ -34,7 +34,7 @@ export async function sendReplayRequest({ const dsn = client?.getDsn(); if (!client || !transport || !dsn || !session.sampled) { - return resolvedSyncPromise({}); + return Promise.resolve({}); } const baseEvent: ReplayEvent = { @@ -55,7 +55,7 @@ export async function sendReplayRequest({ // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions client.recordDroppedEvent('event_processor', 'replay'); DEBUG_BUILD && debug.log('An event processor returned `null`, will not send event.'); - return resolvedSyncPromise({}); + return Promise.resolve({}); } /* From 952be1ff873cba5e60c631c4d5e1612221f15094 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Wed, 17 Sep 2025 14:43:03 +0200 Subject: [PATCH 2/2] Apply suggestion from @Lms24 Co-authored-by: Lukas Stracke --- packages/core/src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 91c29b86e24d..924bd1810ea3 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -320,7 +320,7 @@ export abstract class Client { public async flush(timeout?: number): PromiseLike { const transport = this._transport; if (!transport) { - return Promise.resolve(true); + return true; } this.emit('flush');