Repository navigation
[v1.x] fix(server): close StdioServerTransport when stdin ends or closes #2911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v1.x
Are you sure you want to change the base?
Changes from all commits
5fec54b
30f40b2
b124c29
c83162f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@modelcontextprotocol/sdk': patch | ||
| --- | ||
|
|
||
| `StdioServerTransport` now closes itself and fires `onclose` when its stdin ends or closes, so a server whose client hangs up tears down through `onclose` and can exit, instead of running on with no parent. A stdin that had already ended or been destroyed before `start()` is | ||
| treated the same way. `close()` is idempotent, so `onclose` fires once when `close()` is also called. Requests still in flight when stdin ends are aborted and not answered, and a notification sent after that rejects with `Not connected`; a client that expects responses keeps | ||
| stdin open until it has read them. An `onclose` that calls `process.exit()` can cut off output that is still being written. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { setInterval } from 'node:timers'; | ||
| import { McpServer } from '../server/mcp.js'; | ||
| import { StdioServerTransport } from '../server/stdio.js'; | ||
|
|
||
| const transport = new StdioServerTransport(); | ||
|
|
||
| const server = new McpServer({ | ||
| name: 'server-with-keep-alive', | ||
| version: '1.0.0' | ||
| }); | ||
|
|
||
| await server.connect(transport); | ||
|
|
||
| // Simulates a real server holding a keep-alive handle (connection pool, file | ||
| // watcher, heartbeat timer, ...). A well-behaved server releases its handles | ||
| // when the connection closes, and the process then exits naturally. | ||
| const keepAlive = setInterval(() => {}, 60_000); | ||
|
|
||
| server.server.onclose = () => { | ||
| clearInterval(keepAlive); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,16 @@ import { Transport } from '../shared/transport.js'; | |
| * Server transport for stdio: this communicates with an MCP client by reading from the current process' stdin and writing to stdout. | ||
| * | ||
| * This transport is only available in Node.js environments. | ||
| * | ||
| * When the client closes its end of the pipe (stdin reaches end-of-file), the transport | ||
| * closes itself and fires `onclose`. A server that holds no other keep-alive handles will | ||
| * then exit naturally. Requests still in flight when stdin ends are not answered; a client | ||
| * that expects responses keeps stdin open until it has read them. | ||
| */ | ||
| export class StdioServerTransport implements Transport { | ||
| private _readBuffer: ReadBuffer; | ||
| private _started = false; | ||
| private _closed = false; | ||
|
|
||
| constructor( | ||
| private _stdin: Readable = process.stdin, | ||
|
|
@@ -46,6 +52,16 @@ export class StdioServerTransport implements Transport { | |
| _onerror = (error: Error) => { | ||
| this.onerror?.(error); | ||
| }; | ||
| _onstdinclose = () => { | ||
| // stdin EOF means the client hung up and nothing more can arrive. | ||
| const open = !this._closed && !this._stdin.destroyed; | ||
| this.close().catch(() => {}); | ||
| // A socket can still fail a pending write after 'end': keep reporting errors until it closes. | ||
| if (open) { | ||
| this._stdin.on('error', this._onerror); | ||
| this._stdin.once('close', () => this._stdin.off('error', this._onerror)); | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Starts listening for messages on stdin. | ||
|
|
@@ -58,8 +74,15 @@ export class StdioServerTransport implements Transport { | |
| } | ||
|
|
||
| this._started = true; | ||
| // A stream that already ended or was destroyed will never emit 'end'/'close' again. | ||
| // Deferred so an onclose assigned right after connect() still sees it. | ||
| if (this._stdin.readableEnded || this._stdin.destroyed) { | ||
| setImmediate(this._onstdinclose); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Servers that reconnect a fresh transport from onclose now spin forever at 100% CPU once the client hangs up, where the base left them idle. src/server/stdio.ts:68-69 sees process.stdin already readableEnded on every new start() and schedules _onstdinclose via setImmediate, which closes, fires onclose, and the handler connects again, with no yield between iterations. Fix: do not re-arm a dead stdin on each start(); either throw from start() when stdin is already ended/destroyed, or fire onclose once per stream and refuse further starts on the same ended stream. Why this was flaggedTrigger: user code that resets the server on close, e.g. Verification: src/server/stdio.ts:68-69 fires setImmediate(this._onstdinclose) on every start() against an already-ended stdin; |
||
| } | ||
| this._stdin.on('data', this._ondata); | ||
| this._stdin.on('error', this._onerror); | ||
| this._stdin.on('end', this._onstdinclose); | ||
| this._stdin.on('close', this._onstdinclose); | ||
| } | ||
|
|
||
| private processReadBuffer() { | ||
|
|
@@ -78,9 +101,16 @@ export class StdioServerTransport implements Transport { | |
| } | ||
|
|
||
| async close(): Promise<void> { | ||
| if (this._closed) { | ||
| return; | ||
| } | ||
| this._closed = true; | ||
|
|
||
| // Remove our event listeners first | ||
| this._stdin.off('data', this._ondata); | ||
| this._stdin.off('error', this._onerror); | ||
| this._stdin.off('end', this._onstdinclose); | ||
| this._stdin.off('close', this._onstdinclose); | ||
|
|
||
| // Check if we were the only data listener | ||
| const remainingDataListeners = this._stdin.listenerCount('data'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Scripted callers that pipe requests in and close stdin (half-close, e.g.
printf '…' | node server.js) now get no response for any handler still running at EOF. src/server/stdio.ts:52 closes the transport on 'end', and Protocol._onclose (src/shared/protocol.ts:657) aborts every in-flight handler, so its response is skipped at src/shared/protocol.ts:788. The changeset calls this accepted; in fact it hits every one-shot pipe whose tool handler awaits anything, since the writer closes the pipe immediately. Fix: on stdin 'end' (as opposed to 'close'/'error'), let request handlers already received before EOF finish and write their responses to stdout before onclose fires and handlers are aborted; keep immediate close on destroy. A Protocol-level drain is the durable fix.Why this was flagged
A client or shell script writes one or more requests to the server's stdin and then closes its write end while still reading stdout, e.g.
printf '…' | node server.jswith a tool handler that awaits file or network I/O. The data chunk arrives, Protocol._onrequest schedules the handler, then the EOF read callback fires 'end' and src/server/stdio.ts:73 invokes _onstdinclose, which calls close() and onclose synchronously (src/server/stdio.ts:114). Protocol._onclose aborts every controller in _requestHandlerAbortControllers (src/shared/protocol.ts:657). When the handler resolves, src/shared/protocol.ts:788 sees signal.aborted and returns without sending the JSON-RPC response. On the base branch the transport ignored 'end', the handler completed and the response was written to stdout; after this change the caller reads nothing on stdout and the request silently vanishes. The new test 'should still deliver messages that arrived before stdin ended' only asserts onmessage was called, not that a response is written, so it does not catch this.Verification: src/server/stdio.ts:73 registers
this._stdin.on('end', this._onstdinclose), andclose()invokesthis.onclose?.()at line 114. Protocol's_oncloseat src/shared/protocol.ts:657-660 aborts every in-flight handler; lines 788-791 then return onabortController.signal.aborted, so no response is written to stdout. On c2ce003start()only listened for 'data' and 'error', so the response was written.