Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 670c624

Browse files
authored
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 
1 parent 062d684 commit 670c624

File tree

3 files changed

+123
-25
lines changed

3 files changed

+123
-25
lines changed

‎packages/browser-utils/src/metrics/utils.ts‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -226,21 +226,13 @@ export function listenForWebVitalReportEvents(
226226
// we only want to collect LCP if we actually navigate. Redirects should be ignored.
227227
if (!options?.isRedirect) {
228228
_runCollectorCallbackOnce('navigation');
229-
safeUnsubscribe(unsubscribeStartNavigation, unsubscribeAfterStartPageLoadSpan);
229+
unsubscribeStartNavigation();
230+
unsubscribeAfterStartPageLoadSpan();
230231
}
231232
});
232233

233234
const unsubscribeAfterStartPageLoadSpan = client.on('afterStartPageLoadSpan', span => {
234235
pageloadSpanId = span.spanContext().spanId;
235-
safeUnsubscribe(unsubscribeAfterStartPageLoadSpan);
236+
unsubscribeAfterStartPageLoadSpan();
236237
});
237238
}
238-
239-
/**
240-
* Invoke a list of unsubscribers in a safe way, by deferring the invocation to the next tick.
241-
* This is necessary because unsubscribing in sync can lead to other callbacks no longer being invoked
242-
* due to in-place array mutation of the subscribers array on the client.
243-
*/
244-
function safeUnsubscribe(...unsubscribers: (() => void | undefined)[]): void {
245-
unsubscribers.forEach(u => u && setTimeout(u, 0));
246-
}

‎packages/core/src/client.ts‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
137137
private _outcomes: { [key: string]: number };
138138

139139
// eslint-disable-next-line @typescript-eslint/ban-types
140-
private _hooks: Record<string, Function[]>;
140+
private _hooks: Record<string, Set<Function>>;
141141

142142
/**
143143
* Initializes this client instance.
@@ -685,21 +685,23 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
685685
* Register a hook on this client.
686686
*/
687687
public on(hook: string, callback: unknown): () => void {
688-
const hooks = (this._hooks[hook] = this._hooks[hook] || []);
688+
const hookCallbacks = (this._hooks[hook] = this._hooks[hook] || newSet());
689689

690-
// @ts-expect-error We assume the types are correct
691-
hooks.push(callback);
690+
// Wrap the callback in a function so that registering the same callback instance multiple
691+
// times results in the callback being called multiple times.
692+
// @ts-expect-error - The `callback` type is correct and must be a function due to the
693+
// individual, specific overloads of this function.
694+
// eslint-disable-next-line @typescript-eslint/ban-types
695+
const uniqueCallback: Function = (...args: unknown[]) => callback(...args);
696+
697+
hookCallbacks.add(uniqueCallback);
692698

693699
// This function returns a callback execution handler that, when invoked,
694700
// deregisters a callback. This is crucial for managing instances where callbacks
695701
// need to be unregistered to prevent self-referencing in callback closures,
696702
// ensuring proper garbage collection.
697703
return () => {
698-
// @ts-expect-error We assume the types are correct
699-
const cbIndex = hooks.indexOf(callback);
700-
if (cbIndex > -1) {
701-
hooks.splice(cbIndex, 1);
702-
}
704+
hookCallbacks.delete(uniqueCallback);
703705
};
704706
}
705707

‎packages/core/test/lib/client.test.ts‎

Lines changed: 109 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2397,10 +2397,8 @@ describe('Client', () => {
23972397

23982398
client.emit('beforeEnvelope', mockEnvelope);
23992399
});
2400-
});
24012400

2402-
describe('hook removal with `on`', () => {
2403-
it('should return a cleanup function that, when executed, unregisters a hook', async () => {
2401+
it('returns a cleanup function that, when executed, unregisters a hook', async () => {
24042402
vi.useFakeTimers();
24052403
expect.assertions(8);
24062404

@@ -2420,7 +2418,7 @@ describe('Client', () => {
24202418
const callback = vi.fn();
24212419
const removeAfterSendEventListenerFn = client.on('afterSendEvent', callback);
24222420

2423-
expect(client['_hooks']['afterSendEvent']).toEqual([callback]);
2421+
expect(client['_hooks']['afterSendEvent']!.size).toBe(1);
24242422

24252423
client.sendEvent(errorEvent);
24262424
vi.runAllTimers();
@@ -2435,7 +2433,7 @@ describe('Client', () => {
24352433

24362434
// Should unregister `afterSendEvent` callback.
24372435
removeAfterSendEventListenerFn();
2438-
expect(client['_hooks']['afterSendEvent']).toEqual([]);
2436+
expect(client['_hooks']['afterSendEvent']!.size).toBe(0);
24392437

24402438
client.sendEvent(errorEvent);
24412439
vi.runAllTimers();
@@ -2450,6 +2448,112 @@ describe('Client', () => {
24502448
expect(callback).toBeCalledTimes(1);
24512449
expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 });
24522450
});
2451+
2452+
it('allows synchronously unregistering multiple callbacks from within the callback', () => {
2453+
const client = new TestClient(getDefaultTestClientOptions());
2454+
2455+
const callback1 = vi.fn();
2456+
const callback2 = vi.fn();
2457+
2458+
const removeCallback1 = client.on('close', () => {
2459+
callback1();
2460+
removeCallback1();
2461+
});
2462+
const removeCallback2 = client.on('close', () => {
2463+
callback2();
2464+
removeCallback2();
2465+
});
2466+
2467+
client.emit('close');
2468+
2469+
expect(callback1).toHaveBeenCalledTimes(1);
2470+
expect(callback2).toHaveBeenCalledTimes(1);
2471+
2472+
callback1.mockReset();
2473+
callback2.mockReset();
2474+
2475+
client.emit('close');
2476+
2477+
expect(callback1).not.toHaveBeenCalled();
2478+
expect(callback2).not.toHaveBeenCalled();
2479+
});
2480+
2481+
it('allows synchronously unregistering other callbacks from within one callback', () => {
2482+
const client = new TestClient(getDefaultTestClientOptions());
2483+
2484+
const callback1 = vi.fn();
2485+
const callback2 = vi.fn();
2486+
2487+
const removeCallback1 = client.on('close', () => {
2488+
callback1();
2489+
removeCallback1();
2490+
removeCallback2();
2491+
});
2492+
const removeCallback2 = client.on('close', () => {
2493+
callback2();
2494+
removeCallback2();
2495+
removeCallback1();
2496+
});
2497+
2498+
client.emit('close');
2499+
2500+
expect(callback1).toHaveBeenCalledTimes(1);
2501+
// callback2 was already cancelled from within callback1, so it must not be called
2502+
expect(callback2).not.toHaveBeenCalled();
2503+
2504+
callback1.mockReset();
2505+
callback2.mockReset();
2506+
2507+
client.emit('close');
2508+
2509+
expect(callback1).not.toHaveBeenCalled();
2510+
expect(callback2).not.toHaveBeenCalled();
2511+
});
2512+
2513+
it('allows registering and unregistering the same callback multiple times', () => {
2514+
const client = new TestClient(getDefaultTestClientOptions());
2515+
const callback = vi.fn();
2516+
2517+
const unregister1 = client.on('close', callback);
2518+
const unregister2 = client.on('close', callback);
2519+
2520+
client.emit('close');
2521+
2522+
expect(callback).toHaveBeenCalledTimes(2);
2523+
2524+
unregister1();
2525+
2526+
callback.mockReset();
2527+
2528+
client.emit('close');
2529+
2530+
expect(callback).toHaveBeenCalledTimes(1);
2531+
2532+
unregister2();
2533+
2534+
callback.mockReset();
2535+
client.emit('close');
2536+
2537+
expect(callback).not.toHaveBeenCalled();
2538+
});
2539+
2540+
it('handles unregistering a callback multiple times', () => {
2541+
const client = new TestClient(getDefaultTestClientOptions());
2542+
const callback = vi.fn();
2543+
2544+
const unregister = client.on('close', callback);
2545+
client.emit('close');
2546+
expect(callback).toHaveBeenCalledTimes(1);
2547+
2548+
callback.mockReset();
2549+
unregister();
2550+
unregister();
2551+
unregister();
2552+
2553+
client.emit('close');
2554+
2555+
expect(callback).not.toHaveBeenCalled();
2556+
});
24532557
});
24542558

24552559
describe('withMonitor', () => {

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /