Fix abort signal listener leak in _requestWithSchema#1672
Fix abort signal listener leak in _requestWithSchema#1672lucaalbinati wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
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.
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
travisbreaks
left a comment
There was a problem hiding this comment.
Clean fix for a real leak. The core issue (anonymous listener on a shared AbortSignal never removed) is correctly identified and the solution is straightforward.
What works well:
The removeEventListener placement covers all the exit paths I can trace:
- Success: response handler callback removes it (line 1276)
- Timeout: fires
cancel()which removes it (line 1251) - Abort:
onAbortfirescancel()which removes it, plus{ once: true }as a belt-and-suspenders fallback - Transport send failure:
.catchremoves it (lines 1328, 1337) - Connection close:
_onclose()iterates all response handlers, which calls the callback that removes it
The { once: true } option is a good defensive addition. If the listener fires, the runtime won't hold a reference afterward regardless. The explicit removeEventListener inside cancel() on the abort path is technically redundant with once: true, but that's fine since removeEventListener on an already-removed listener is a no-op and the explicitness aids readability.
Tests are solid. The sequential-requests-same-signal test (the accumulation test) is the most valuable one since it directly demonstrates the leak scenario. Good that all five tests fail without the fix and pass with it.
One minor observation (not blocking): The _enqueueTaskMessage catch path and transport .send catch path both clean up the timeout and reject, but neither removes the response handler or progress handler from their respective maps. That's a pre-existing gap, not introduced here, so not a concern for this PR. Just noting it in case it's worth a follow-up.
Changeset bot is flagging the missing changeset. Might want to add one for a patch bump.
Summary
AbortSignalin_requestWithSchemawas never removed, causing listeners to accumulate across requests sharing the same signalremoveEventListeneron all exit paths (response handler, cancel, timeout, transport send failure, task enqueue failure), and addedonce: trueas a defensive fallbackTest plan