From 059e2dcd13262644f8e7e34ced9d2fc796f66cbc Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 19 Sep 2025 11:32:45 +0200 Subject: [PATCH 1/3] fix(core): Fix client hook edge cases around multiple callbacks --- packages/core/src/client.ts | 20 ++++---- packages/core/test/lib/client.test.ts | 69 +++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 6a7451fe96cb..936e8ac3ebe0 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -137,7 +137,7 @@ export abstract class Client { private _outcomes: { [key: string]: number }; // eslint-disable-next-line @typescript-eslint/ban-types - private _hooks: Record; + private _hooks: Record>; /** * Initializes this client instance. @@ -685,21 +685,23 @@ export abstract class Client { * Register a hook on this client. */ public on(hook: string, callback: unknown): () => void { - const hooks = (this._hooks[hook] = this._hooks[hook] || []); + const hookCallbacks = (this._hooks[hook] = this._hooks[hook] || new Set()); - // @ts-expect-error We assume the types are correct - hooks.push(callback); + // Wrap the callback in a function so that registering the same callback instance multiple + // times results in the callback being called multiple times. + // @ts-expect-error - The `callback` type is correct and must be a function due to the + // individual, specific overloads of this function. + // eslint-disable-next-line @typescript-eslint/ban-types + const uniqueCallback: Function = (...args: unknown[]) => callback(...args); + + hookCallbacks.add(uniqueCallback); // This function returns a callback execution handler that, when invoked, // deregisters a callback. This is crucial for managing instances where callbacks // need to be unregistered to prevent self-referencing in callback closures, // ensuring proper garbage collection. return () => { - // @ts-expect-error We assume the types are correct - const cbIndex = hooks.indexOf(callback); - if (cbIndex > -1) { - hooks.splice(cbIndex, 1); - } + hookCallbacks.delete(uniqueCallback); }; } diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index afca376393ee..f1995b074073 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -2397,10 +2397,8 @@ describe('Client', () => { client.emit('beforeEnvelope', mockEnvelope); }); - }); - describe('hook removal with `on`', () => { - it('should return a cleanup function that, when executed, unregisters a hook', async () => { + it('returns a cleanup function that, when executed, unregisters a hook', async () => { vi.useFakeTimers(); expect.assertions(8); @@ -2420,7 +2418,7 @@ describe('Client', () => { const callback = vi.fn(); const removeAfterSendEventListenerFn = client.on('afterSendEvent', callback); - expect(client['_hooks']['afterSendEvent']).toEqual([callback]); + expect(client['_hooks']['afterSendEvent']!.size).toBe(1); client.sendEvent(errorEvent); vi.runAllTimers(); @@ -2435,7 +2433,7 @@ describe('Client', () => { // Should unregister `afterSendEvent` callback. removeAfterSendEventListenerFn(); - expect(client['_hooks']['afterSendEvent']).toEqual([]); + expect(client['_hooks']['afterSendEvent']!.size).toBe(0); client.sendEvent(errorEvent); vi.runAllTimers(); @@ -2450,6 +2448,67 @@ describe('Client', () => { expect(callback).toBeCalledTimes(1); expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 }); }); + + it('allows synchronously unregistering multiple callbacks from within the callback', () => { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + enableSend: true, + }), + ); + + const callback1 = vi.fn(); + const callback2 = vi.fn(); + + const removeCallback1 = client.on('close', () => { + callback1(); + removeCallback1(); + }); + const removeCallback2 = client.on('close', () => { + callback2(); + removeCallback2(); + }); + + client.emit('close'); + + expect(callback1).toHaveBeenCalledTimes(1); + expect(callback2).toHaveBeenCalledTimes(1); + + callback1.mockReset(); + callback2.mockReset(); + + client.emit('close'); + + expect(callback1).toHaveBeenCalledTimes(0); + expect(callback2).toHaveBeenCalledTimes(0); + }); + + it('allows registering and unregistering the same callback multiple times', () => { + const client = new TestClient(options); + const callback = vi.fn(); + + const unregister1 = client.on('close', callback); + const unregister2 = client.on('close', callback); + + client.emit('close'); + + expect(callback).toHaveBeenCalledTimes(2); + + unregister1(); + + callback.mockReset(); + + client.emit('close'); + + expect(callback).toHaveBeenCalledTimes(1); + + unregister2(); + + callback.mockReset(); + client.emit('close'); + + expect(callback).toHaveBeenCalledTimes(0); + }); }); describe('withMonitor', () => { From 2934b55c33241b15106412ec2d1de68857945875 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 19 Sep 2025 11:36:28 +0200 Subject: [PATCH 2/3] remove safeUnsubscribe --- packages/browser-utils/src/metrics/utils.ts | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/packages/browser-utils/src/metrics/utils.ts b/packages/browser-utils/src/metrics/utils.ts index 5caab5bc75cc..4012d4118ad3 100644 --- a/packages/browser-utils/src/metrics/utils.ts +++ b/packages/browser-utils/src/metrics/utils.ts @@ -226,21 +226,13 @@ export function listenForWebVitalReportEvents( // we only want to collect LCP if we actually navigate. Redirects should be ignored. if (!options?.isRedirect) { _runCollectorCallbackOnce('navigation'); - safeUnsubscribe(unsubscribeStartNavigation, unsubscribeAfterStartPageLoadSpan); + unsubscribeStartNavigation(); + unsubscribeAfterStartPageLoadSpan(); } }); const unsubscribeAfterStartPageLoadSpan = client.on('afterStartPageLoadSpan', span => { pageloadSpanId = span.spanContext().spanId; - safeUnsubscribe(unsubscribeAfterStartPageLoadSpan); + unsubscribeAfterStartPageLoadSpan(); }); } - -/** - * Invoke a list of unsubscribers in a safe way, by deferring the invocation to the next tick. - * This is necessary because unsubscribing in sync can lead to other callbacks no longer being invoked - * due to in-place array mutation of the subscribers array on the client. - */ -function safeUnsubscribe(...unsubscribers: (() => void | undefined)[]): void { - unsubscribers.forEach(u => u && setTimeout(u, 0)); -} From 08bd508c7eae82545168e020534b4bece8195abf Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 19 Sep 2025 11:55:29 +0200 Subject: [PATCH 3/3] more tests --- packages/core/test/lib/client.test.ts | 65 ++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index f1995b074073..c7cbe7ab4a97 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -2450,12 +2450,7 @@ describe('Client', () => { }); it('allows synchronously unregistering multiple callbacks from within the callback', () => { - const client = new TestClient( - getDefaultTestClientOptions({ - dsn: PUBLIC_DSN, - enableSend: true, - }), - ); + const client = new TestClient(getDefaultTestClientOptions()); const callback1 = vi.fn(); const callback2 = vi.fn(); @@ -2479,12 +2474,44 @@ describe('Client', () => { client.emit('close'); - expect(callback1).toHaveBeenCalledTimes(0); - expect(callback2).toHaveBeenCalledTimes(0); + expect(callback1).not.toHaveBeenCalled(); + expect(callback2).not.toHaveBeenCalled(); + }); + + it('allows synchronously unregistering other callbacks from within one callback', () => { + const client = new TestClient(getDefaultTestClientOptions()); + + const callback1 = vi.fn(); + const callback2 = vi.fn(); + + const removeCallback1 = client.on('close', () => { + callback1(); + removeCallback1(); + removeCallback2(); + }); + const removeCallback2 = client.on('close', () => { + callback2(); + removeCallback2(); + removeCallback1(); + }); + + client.emit('close'); + + expect(callback1).toHaveBeenCalledTimes(1); + // callback2 was already cancelled from within callback1, so it must not be called + expect(callback2).not.toHaveBeenCalled(); + + callback1.mockReset(); + callback2.mockReset(); + + client.emit('close'); + + expect(callback1).not.toHaveBeenCalled(); + expect(callback2).not.toHaveBeenCalled(); }); it('allows registering and unregistering the same callback multiple times', () => { - const client = new TestClient(options); + const client = new TestClient(getDefaultTestClientOptions()); const callback = vi.fn(); const unregister1 = client.on('close', callback); @@ -2507,7 +2534,25 @@ describe('Client', () => { callback.mockReset(); client.emit('close'); - expect(callback).toHaveBeenCalledTimes(0); + expect(callback).not.toHaveBeenCalled(); + }); + + it('handles unregistering a callback multiple times', () => { + const client = new TestClient(getDefaultTestClientOptions()); + const callback = vi.fn(); + + const unregister = client.on('close', callback); + client.emit('close'); + expect(callback).toHaveBeenCalledTimes(1); + + callback.mockReset(); + unregister(); + unregister(); + unregister(); + + client.emit('close'); + + expect(callback).not.toHaveBeenCalled(); }); });