Skip to content

Commit 937f690

Browse files
committed
Fix abort signal listener leak in _requestWithSchema
The abort listener added to the caller's signal was never removed, causing listeners to accumulate across requests sharing the same signal. Named the listener, added explicit removeEventListener on all exit paths, and added once:true as a defensive fallback.
1 parent 108f2f3 commit 937f690

2 files changed

Lines changed: 160 additions & 2 deletions

File tree

packages/core/src/shared/protocol.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,6 +1248,7 @@ export abstract class Protocol<ContextT extends BaseContext> {
12481248
}
12491249

12501250
const cancel = (reason: unknown) => {
1251+
options?.signal?.removeEventListener('abort', onAbort);
12511252
this._responseHandlers.delete(messageId);
12521253
this._progressHandlers.delete(messageId);
12531254
this._cleanupTimeout(messageId);
@@ -1272,6 +1273,8 @@ export abstract class Protocol<ContextT extends BaseContext> {
12721273
};
12731274

12741275
this._responseHandlers.set(messageId, response => {
1276+
options?.signal?.removeEventListener('abort', onAbort);
1277+
12751278
if (options?.signal?.aborted) {
12761279
return;
12771280
}
@@ -1292,9 +1295,10 @@ export abstract class Protocol<ContextT extends BaseContext> {
12921295
}
12931296
});
12941297

1295-
options?.signal?.addEventListener('abort', () => {
1298+
const onAbort = () => {
12961299
cancel(options?.signal?.reason);
1297-
});
1300+
};
1301+
options?.signal?.addEventListener('abort', onAbort, { once: true });
12981302

12991303
const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC;
13001304
const timeoutHandler = () => cancel(new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout }));
@@ -1321,6 +1325,7 @@ export abstract class Protocol<ContextT extends BaseContext> {
13211325
message: jsonrpcRequest,
13221326
timestamp: Date.now()
13231327
}).catch(error => {
1328+
options?.signal?.removeEventListener('abort', onAbort);
13241329
this._cleanupTimeout(messageId);
13251330
reject(error);
13261331
});
@@ -1330,6 +1335,7 @@ export abstract class Protocol<ContextT extends BaseContext> {
13301335
} else {
13311336
// No related task - send through transport normally
13321337
this._transport.send(jsonrpcRequest, { relatedRequestId, resumptionToken, onresumptiontoken }).catch(error => {
1338+
options?.signal?.removeEventListener('abort', onAbort);
13331339
this._cleanupTimeout(messageId);
13341340
reject(error);
13351341
});

packages/core/test/shared/protocol.test.ts

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5723,3 +5723,155 @@ describe('Error handling for missing resolvers', () => {
57235723
});
57245724
});
57255725
});
5726+
5727+
describe('Abort signal listener cleanup', () => {
5728+
let protocol: Protocol<BaseContext>;
5729+
let transport: MockTransport;
5730+
5731+
beforeEach(() => {
5732+
vi.useFakeTimers();
5733+
transport = new MockTransport();
5734+
vi.spyOn(transport, 'send');
5735+
protocol = new (class extends Protocol<BaseContext> {
5736+
protected assertCapabilityForMethod(): void {}
5737+
protected assertNotificationCapability(): void {}
5738+
protected assertRequestHandlerCapability(): void {}
5739+
protected assertTaskCapability(): void {}
5740+
protected buildContext(ctx: BaseContext): BaseContext {
5741+
return ctx;
5742+
}
5743+
protected assertTaskHandlerCapability(): void {}
5744+
})();
5745+
});
5746+
5747+
afterEach(() => {
5748+
vi.useRealTimers();
5749+
});
5750+
5751+
test('should remove abort listener when request completes successfully', async () => {
5752+
await protocol.connect(transport);
5753+
5754+
const abortController = new AbortController();
5755+
const removeEventListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
5756+
5757+
const mockSchema: ZodType<{ result: string }> = z.object({
5758+
result: z.string()
5759+
});
5760+
5761+
const requestPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
5762+
timeout: 5000,
5763+
signal: abortController.signal
5764+
});
5765+
5766+
// Simulate a successful response
5767+
if (transport.onmessage) {
5768+
transport.onmessage({
5769+
jsonrpc: '2.0',
5770+
id: 0,
5771+
result: { result: 'success' }
5772+
});
5773+
}
5774+
5775+
await expect(requestPromise).resolves.toEqual({ result: 'success' });
5776+
expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function));
5777+
});
5778+
5779+
test('should remove abort listener when request times out', async () => {
5780+
await protocol.connect(transport);
5781+
5782+
const abortController = new AbortController();
5783+
const removeEventListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
5784+
5785+
const mockSchema: ZodType<{ result: string }> = z.object({
5786+
result: z.string()
5787+
});
5788+
5789+
const requestPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
5790+
timeout: 100,
5791+
signal: abortController.signal
5792+
});
5793+
5794+
vi.advanceTimersByTime(101);
5795+
5796+
await expect(requestPromise).rejects.toThrow('Request timed out');
5797+
expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function));
5798+
});
5799+
5800+
test('should not accumulate listeners across multiple requests on the same signal', async () => {
5801+
await protocol.connect(transport);
5802+
5803+
const abortController = new AbortController();
5804+
const addEventListenerSpy = vi.spyOn(abortController.signal, 'addEventListener');
5805+
const removeEventListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
5806+
5807+
const mockSchema: ZodType<{ result: string }> = z.object({
5808+
result: z.string()
5809+
});
5810+
5811+
// Make 3 sequential requests on the same signal
5812+
for (let i = 0; i < 3; i++) {
5813+
const requestPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
5814+
timeout: 5000,
5815+
signal: abortController.signal
5816+
});
5817+
5818+
if (transport.onmessage) {
5819+
transport.onmessage({
5820+
jsonrpc: '2.0',
5821+
id: i,
5822+
result: { result: 'success' }
5823+
});
5824+
}
5825+
5826+
await expect(requestPromise).resolves.toEqual({ result: 'success' });
5827+
}
5828+
5829+
// Each request should have added and removed exactly one listener
5830+
expect(addEventListenerSpy).toHaveBeenCalledTimes(3);
5831+
expect(removeEventListenerSpy).toHaveBeenCalledTimes(3);
5832+
});
5833+
5834+
test('should remove abort listener when abort signal is triggered', async () => {
5835+
await protocol.connect(transport);
5836+
5837+
const abortController = new AbortController();
5838+
const removeEventListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
5839+
5840+
const mockSchema: ZodType<{ result: string }> = z.object({
5841+
result: z.string()
5842+
});
5843+
5844+
const requestPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
5845+
timeout: 5000,
5846+
signal: abortController.signal
5847+
});
5848+
5849+
abortController.abort('User cancelled');
5850+
5851+
await expect(requestPromise).rejects.toThrow();
5852+
// cancel() calls removeEventListener even though once:true also cleans up
5853+
expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function));
5854+
});
5855+
5856+
test('should remove abort listener when transport.send fails', async () => {
5857+
await protocol.connect(transport);
5858+
5859+
const abortController = new AbortController();
5860+
const removeEventListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
5861+
5862+
// Make transport.send reject
5863+
vi.spyOn(transport, 'send').mockRejectedValueOnce(new Error('Transport failure'));
5864+
5865+
const mockSchema: ZodType<{ result: string }> = z.object({
5866+
result: z.string()
5867+
});
5868+
5869+
const requestPromise = testRequest(protocol, { method: 'example', params: {} }, mockSchema, {
5870+
timeout: 5000,
5871+
signal: abortController.signal
5872+
});
5873+
5874+
await expect(requestPromise).rejects.toThrow('Transport failure');
5875+
expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function));
5876+
});
5877+
});

0 commit comments

Comments
 (0)