fix(core): Fix client hook edge cases around multiple callbacks (#17706)

This PR fixes two edge cases around our client hook subscriber
management:

1. Registering the same callback instance twice on the same hook,
resulted in the callback only being invoked once. Fixed by wrapping the
passed callback in a function to "unique-ify" it.
2. Unregistering one callback synchronously within the callback, caused
other callbacks to not be invoked due to in-place, sync array mutation.
Fixed by converting the hooks data structure from `Array<Function>` to
`Set<Function>` which is resilient to sync, in-place mutation. This also
lets us remove the workaround introduced in
#17272 where we
initially discovered this bug.

Added regression tests for both cases that failed beforehand.

closes #17276

Author: Lms24

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));
-}

packages/core/src/client.ts

@@ -137,7 +137,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
   private _outcomes: { [key: string]: number };
 
   // eslint-disable-next-line @typescript-eslint/ban-types
-  private _hooks: Record<string, Function[]>;
+  private _hooks: Record<string, Set<Function>>;
 
   /**
    * Initializes this client instance.
@@ -685,21 +685,23 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
    * 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);
     };
   }
 

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,112 @@ 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());
+
+      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).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(getDefaultTestClientOptions());
+      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).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();
+    });
   });
 
   describe('withMonitor', () => {