Skip to content

Commit 1438d5b

Browse files
CopilotLms24
andcommitted
Fix client.flush() blocking process exit with unref'd timers and early exit
Co-authored-by: Lms24 <8420481+Lms24@users.noreply.github.com>
1 parent f40eb35 commit 1438d5b

File tree

5 files changed

+111
-2
lines changed

5 files changed

+111
-2
lines changed

packages/core/src/client.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1148,11 +1148,23 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
11481148
* `false` otherwise
11491149
*/
11501150
protected async _isClientDoneProcessing(timeout?: number): Promise<boolean> {
1151+
// Early exit if there's nothing to process
1152+
if (!this._numProcessing) {
1153+
return true;
1154+
}
1155+
11511156
let ticked = 0;
11521157

11531158
// if no timeout is provided, we wait "forever" until everything is processed
11541159
while (!timeout || ticked < timeout) {
1155-
await new Promise(resolve => setTimeout(resolve, 1));
1160+
await new Promise(resolve => {
1161+
const timer = setTimeout(resolve, 1);
1162+
// Use unref() in Node.js to allow the process to exit naturally
1163+
// In browsers, setTimeout returns a number, so we check for that
1164+
if (typeof timer !== 'number' && timer.unref) {
1165+
timer.unref();
1166+
}
1167+
});
11561168

11571169
if (!this._numProcessing) {
11581170
return true;

packages/core/src/utils/promisebuffer.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,17 @@ export function makePromiseBuffer<T>(limit: number = 100): PromiseBuffer<T> {
7777
return drainPromise;
7878
}
7979

80-
const promises = [drainPromise, new Promise<boolean>(resolve => setTimeout(() => resolve(false), timeout))];
80+
const promises = [
81+
drainPromise,
82+
new Promise<boolean>(resolve => {
83+
const timer = setTimeout(() => resolve(false), timeout);
84+
// Use unref() in Node.js to allow the process to exit naturally
85+
// In browsers, setTimeout returns a number, so we check for that
86+
if (typeof timer !== 'number' && timer.unref) {
87+
timer.unref();
88+
}
89+
}),
90+
];
8191

8292
// Promise.race will resolve to the first promise that resolves or rejects
8393
// So if the drainPromise resolves, the timeout promise will be ignored

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,6 +2205,50 @@ describe('Client', () => {
22052205
}),
22062206
]);
22072207
});
2208+
2209+
test('flush returns immediately when nothing is processing', async () => {
2210+
vi.useRealTimers();
2211+
expect.assertions(2);
2212+
2213+
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
2214+
const client = new TestClient(options);
2215+
2216+
const startTime = Date.now();
2217+
const result = await client.flush(1000);
2218+
const elapsed = Date.now() - startTime;
2219+
2220+
// Should return true immediately without waiting
2221+
expect(result).toBe(true);
2222+
// Should take less than 100ms (much less than the 1000ms timeout)
2223+
expect(elapsed).toBeLessThan(100);
2224+
});
2225+
2226+
test('flush with early exit when processing completes', async () => {
2227+
vi.useRealTimers();
2228+
expect.assertions(3);
2229+
2230+
const { makeTransport, getSendCalled, getSentCount, delay } = makeFakeTransport(50);
2231+
2232+
const client = new TestClient(
2233+
getDefaultTestClientOptions({
2234+
dsn: PUBLIC_DSN,
2235+
enableSend: true,
2236+
transport: makeTransport,
2237+
}),
2238+
);
2239+
2240+
client.captureMessage('test');
2241+
expect(getSendCalled()).toEqual(1);
2242+
2243+
const startTime = Date.now();
2244+
// Use a large timeout but processing should complete early
2245+
await client.flush(5000);
2246+
const elapsed = Date.now() - startTime;
2247+
2248+
expect(getSentCount()).toEqual(1);
2249+
// Should complete in ~50ms (the transport delay), not wait for the full 5000ms timeout
2250+
expect(elapsed).toBeLessThan(500);
2251+
});
22082252
});
22092253

22102254
describe('sendEvent', () => {

packages/core/test/lib/utils/promisebuffer.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,4 +252,17 @@ describe('PromiseBuffer', () => {
252252
expect(e).toEqual(new Error('whoops'));
253253
}
254254
});
255+
256+
test('drain returns immediately when buffer is empty', async () => {
257+
const buffer = makePromiseBuffer();
258+
expect(buffer.$.length).toEqual(0);
259+
260+
const startTime = Date.now();
261+
const result = await buffer.drain(5000);
262+
const elapsed = Date.now() - startTime;
263+
264+
expect(result).toBe(true);
265+
// Should return immediately, not wait for timeout
266+
expect(elapsed).toBeLessThan(100);
267+
});
255268
});

packages/node-core/test/sdk/client.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,4 +386,34 @@ describe('NodeClient', () => {
386386
expect(processOffSpy).toHaveBeenNthCalledWith(1, 'beforeExit', expect.any(Function));
387387
});
388388
});
389+
390+
describe('flush', () => {
391+
it('flush returns immediately when nothing is processing', async () => {
392+
const options = getDefaultNodeClientOptions();
393+
const client = new NodeClient(options);
394+
395+
const startTime = Date.now();
396+
const result = await client.flush(1000);
397+
const elapsed = Date.now() - startTime;
398+
399+
// Should return true immediately without waiting
400+
expect(result).toBe(true);
401+
// Should take less than 100ms (much less than the 1000ms timeout)
402+
expect(elapsed).toBeLessThan(100);
403+
});
404+
405+
it('flush does not block process exit when using unref timers', async () => {
406+
// This test verifies that timers are properly unref'd in Node.js
407+
// We create a client with no processing and a long timeout
408+
const options = getDefaultNodeClientOptions();
409+
const client = new NodeClient(options);
410+
411+
// Flush with a long timeout - if timers are not unref'd, this would block
412+
const flushPromise = client.flush(5000);
413+
414+
// The flush should complete immediately since there's nothing to process
415+
const result = await flushPromise;
416+
expect(result).toBe(true);
417+
});
418+
});
389419
});

0 commit comments

Comments
 (0)