From 0984898022bbd2e1260a90ceb7332253b8593794 Mon Sep 17 00:00:00 2001 From: shellRaining Date: Fri, 24 Oct 2025 19:35:40 +0800 Subject: [PATCH] fix: #780 onerror and other listener not remove after client close (stdio) --- packages/client/src/client/stdio.ts | 49 ++++++++++++------- .../client/test/client/cross-spawn.test.ts | 33 ++++++++++--- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/packages/client/src/client/stdio.ts b/packages/client/src/client/stdio.ts index 56d773a7b..0a051eb18 100644 --- a/packages/client/src/client/stdio.ts +++ b/packages/client/src/client/stdio.ts @@ -95,6 +95,8 @@ export class StdioClientTransport implements Transport { private _readBuffer: ReadBuffer = new ReadBuffer(); private _serverParams: StdioServerParameters; private _stderrStream: PassThrough | null = null; + private _onServerDataHandler?: (chunk: Buffer) => void; + private _onServerErrorHandler?: (error: Error) => void; onclose?: () => void; onerror?: (error: Error) => void; @@ -130,33 +132,31 @@ export class StdioClientTransport implements Transport { cwd: this._serverParams.cwd }); + this._onServerDataHandler = (chunk: Buffer) => { + this._readBuffer.append(chunk); + this.processReadBuffer(); + }; + this._onServerErrorHandler = (error: Error) => { + this.onerror?.(error); + }; + + this._process.stdout?.on('data', this._onServerDataHandler); + this._process.stdout?.on('error', this._onServerErrorHandler); + this._process.stdin?.on('error', this._onServerErrorHandler); + this._process.on('error', error => { reject(error); this.onerror?.(error); }); - - this._process.on('spawn', () => { - resolve(); - }); - - this._process.on('close', _code => { + this._process.once('spawn', () => resolve()); + this._process.once('close', _code => { + if (this._process) { + this.cleanupListeners(this._process); + } this._process = undefined; this.onclose?.(); }); - this._process.stdin?.on('error', error => { - this.onerror?.(error); - }); - - this._process.stdout?.on('data', chunk => { - this._readBuffer.append(chunk); - this.processReadBuffer(); - }); - - this._process.stdout?.on('error', error => { - this.onerror?.(error); - }); - if (this._stderrStream && this._process.stderr) { this._process.stderr.pipe(this._stderrStream); } @@ -202,8 +202,19 @@ export class StdioClientTransport implements Transport { } } + private cleanupListeners(process: ChildProcess) { + if (this._onServerDataHandler) { + process.stdout?.off('data', this._onServerDataHandler); + } + if (this._onServerErrorHandler) { + process.stdout?.off('error', this._onServerErrorHandler); + process.stdin?.off('error', this._onServerErrorHandler); + } + } + async close(): Promise { if (this._process) { + this.cleanupListeners(this._process); const processToClose = this._process; this._process = undefined; diff --git a/packages/client/test/client/cross-spawn.test.ts b/packages/client/test/client/cross-spawn.test.ts index 8e4a80fc2..71c47f7ac 100644 --- a/packages/client/test/client/cross-spawn.test.ts +++ b/packages/client/test/client/cross-spawn.test.ts @@ -3,6 +3,7 @@ import type { ChildProcess } from 'node:child_process'; import type { JSONRPCMessage } from '@modelcontextprotocol/core'; import spawn from 'cross-spawn'; import type { Mock, MockedFunction } from 'vitest'; +import { vi } from 'vitest'; import { getDefaultEnvironment, StdioClientTransport } from '../../src/client/stdio.js'; @@ -16,8 +17,9 @@ describe('StdioClientTransport using cross-spawn', () => { mockSpawn.mockImplementation(() => { const mockProcess: { on: Mock; - stdin?: { on: Mock; write: Mock }; - stdout?: { on: Mock }; + once: Mock; + stdin?: { on: Mock; write: Mock; off: Mock }; + stdout?: { on: Mock; off: Mock }; stderr?: null; } = { on: vi.fn((event: string, callback: () => void) => { @@ -26,12 +28,20 @@ describe('StdioClientTransport using cross-spawn', () => { } return mockProcess; }), + once: vi.fn((event: string, callback: () => void) => { + if (event === 'spawn') { + callback(); + } + return mockProcess; + }), stdin: { on: vi.fn(), - write: vi.fn().mockReturnValue(true) + write: vi.fn().mockReturnValue(true), + off: vi.fn() }, stdout: { - on: vi.fn() + on: vi.fn(), + off: vi.fn() }, stderr: null }; @@ -109,13 +119,16 @@ describe('StdioClientTransport using cross-spawn', () => { // get the mock process object const mockProcess: { on: Mock; + once: Mock; stdin: { on: Mock; write: Mock; once: Mock; + off: Mock; }; stdout: { on: Mock; + off: Mock; }; stderr: null; } = { @@ -125,13 +138,21 @@ describe('StdioClientTransport using cross-spawn', () => { } return mockProcess; }), + once: vi.fn((event: string, callback: () => void) => { + if (event === 'spawn') { + callback(); + } + return mockProcess; + }), stdin: { on: vi.fn(), write: vi.fn().mockReturnValue(true), - once: vi.fn() + once: vi.fn(), + off: vi.fn() }, stdout: { - on: vi.fn() + on: vi.fn(), + off: vi.fn() }, stderr: null };