From 7c91349363c33abe63937f2a2d75cb3f8aab1010 Mon Sep 17 00:00:00 2001 From: Jerome Date: Fri, 3 Jan 2025 16:29:06 +0000 Subject: [PATCH 1/4] Added clean shutdown + tests --- src/integration-tests/process-cleanup.test.ts | 28 ++++++++++++++ src/server/stdio.test.ts | 38 +++++++++++++++++++ src/server/stdio.ts | 8 ++++ 3 files changed, 74 insertions(+) create mode 100644 src/integration-tests/process-cleanup.test.ts diff --git a/src/integration-tests/process-cleanup.test.ts b/src/integration-tests/process-cleanup.test.ts new file mode 100644 index 00000000..0dd7861a --- /dev/null +++ b/src/integration-tests/process-cleanup.test.ts @@ -0,0 +1,28 @@ +import { Server } from "../server/index.js"; +import { StdioServerTransport } from "../server/stdio.js"; + +describe("Process cleanup", () => { + jest.setTimeout(5000); // 5 second timeout + + it("should exit cleanly after closing transport", async () => { + const server = new Server( + { + name: "test-server", + version: "1.0.0", + }, + { + capabilities: {}, + } + ); + + const transport = new StdioServerTransport(); + await server.connect(transport); + + // Close the transport + await transport.close(); + + // If we reach here without hanging, the test passes + // The test runner will fail if the process hangs + expect(true).toBe(true); + }); +}); \ No newline at end of file diff --git a/src/server/stdio.test.ts b/src/server/stdio.test.ts index 5243268d..d0414224 100644 --- a/src/server/stdio.test.ts +++ b/src/server/stdio.test.ts @@ -100,3 +100,41 @@ test("should read multiple messages", async () => { await finished; expect(readMessages).toEqual(messages); }); + +test("should properly clean up resources when closed", async () => { + // Create mock streams that track their destroyed state + const mockStdin = new Readable({ + read() {}, // No-op implementation + destroy() { + this.destroyed = true; + return this; + } + }); + const mockStdout = new Writable({ + write(chunk, encoding, callback) { + callback(); + }, + destroy() { + this.destroyed = true; + return this; + } + }); + + const transport = new StdioServerTransport(mockStdin, mockStdout); + await transport.start(); + + // Send a message to potentially create 'drain' listeners + await transport.send({ jsonrpc: "2.0", method: "test", id: 1 }); + + // Close the transport + await transport.close(); + + // Check that all listeners were removed + expect(mockStdin.listenerCount('data')).toBe(0); + expect(mockStdin.listenerCount('error')).toBe(0); + expect(mockStdout.listenerCount('drain')).toBe(0); + + // Check that streams were properly ended + expect(mockStdin.destroyed).toBe(true); + expect(mockStdout.destroyed).toBe(true); +}); diff --git a/src/server/stdio.ts b/src/server/stdio.ts index f0eff82c..4e44cc33 100644 --- a/src/server/stdio.ts +++ b/src/server/stdio.ts @@ -62,8 +62,16 @@ export class StdioServerTransport implements Transport { } async close(): Promise { + // Remove all event listeners this._stdin.off("data", this._ondata); this._stdin.off("error", this._onerror); + this._stdout.removeAllListeners('drain'); + + // Destroy both streams + this._stdin.destroy(); + this._stdout.destroy(); + + // Clear the buffer and notify closure this._readBuffer.clear(); this.onclose?.(); } From 6ebf67ea7369f732f728974baeed721a90cbc1de Mon Sep 17 00:00:00 2001 From: Jerome Date: Fri, 3 Jan 2025 18:45:11 +0000 Subject: [PATCH 2/4] Don't remove drain listeners --- src/server/stdio.test.ts | 2 +- src/server/stdio.ts | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/server/stdio.test.ts b/src/server/stdio.test.ts index d0414224..5de990f2 100644 --- a/src/server/stdio.test.ts +++ b/src/server/stdio.test.ts @@ -123,7 +123,7 @@ test("should properly clean up resources when closed", async () => { const transport = new StdioServerTransport(mockStdin, mockStdout); await transport.start(); - // Send a message to potentially create 'drain' listeners + // Send a message to potentially create listeners await transport.send({ jsonrpc: "2.0", method: "test", id: 1 }); // Close the transport diff --git a/src/server/stdio.ts b/src/server/stdio.ts index 4e44cc33..e3c78b21 100644 --- a/src/server/stdio.ts +++ b/src/server/stdio.ts @@ -62,15 +62,14 @@ export class StdioServerTransport implements Transport { } async close(): Promise { - // Remove all event listeners + // Remove our event listeners this._stdin.off("data", this._ondata); this._stdin.off("error", this._onerror); - this._stdout.removeAllListeners('drain'); - - // Destroy both streams + + // Destroy streams to ensure they're fully closed this._stdin.destroy(); this._stdout.destroy(); - + // Clear the buffer and notify closure this._readBuffer.clear(); this.onclose?.(); From 8a077474a6f0e87fe3cba0e5ea820db06954d72c Mon Sep 17 00:00:00 2001 From: Jerome Date: Fri, 3 Jan 2025 19:05:38 +0000 Subject: [PATCH 3/4] Pause stdin if we were the only listeners to the data events --- src/server/stdio.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/server/stdio.ts b/src/server/stdio.ts index e3c78b21..30c80012 100644 --- a/src/server/stdio.ts +++ b/src/server/stdio.ts @@ -62,13 +62,17 @@ export class StdioServerTransport implements Transport { } async close(): Promise { - // Remove our event listeners + // Remove our event listeners first this._stdin.off("data", this._ondata); this._stdin.off("error", this._onerror); - - // Destroy streams to ensure they're fully closed - this._stdin.destroy(); - this._stdout.destroy(); + + // Check if we were the only data listener + const remainingDataListeners = this._stdin.listenerCount('data'); + if (remainingDataListeners === 0) { + // Only pause stdin if we were the only listener + // This prevents interfering with other parts of the application that might be using stdin + this._stdin.pause(); + } // Clear the buffer and notify closure this._readBuffer.clear(); From a2157dcbd499a46a326ac37d465640024c231b59 Mon Sep 17 00:00:00 2001 From: Jerome Date: Fri, 3 Jan 2025 19:09:11 +0000 Subject: [PATCH 4/4] Removed unneeded test --- src/server/stdio.test.ts | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/src/server/stdio.test.ts b/src/server/stdio.test.ts index 5de990f2..5243268d 100644 --- a/src/server/stdio.test.ts +++ b/src/server/stdio.test.ts @@ -100,41 +100,3 @@ test("should read multiple messages", async () => { await finished; expect(readMessages).toEqual(messages); }); - -test("should properly clean up resources when closed", async () => { - // Create mock streams that track their destroyed state - const mockStdin = new Readable({ - read() {}, // No-op implementation - destroy() { - this.destroyed = true; - return this; - } - }); - const mockStdout = new Writable({ - write(chunk, encoding, callback) { - callback(); - }, - destroy() { - this.destroyed = true; - return this; - } - }); - - const transport = new StdioServerTransport(mockStdin, mockStdout); - await transport.start(); - - // Send a message to potentially create listeners - await transport.send({ jsonrpc: "2.0", method: "test", id: 1 }); - - // Close the transport - await transport.close(); - - // Check that all listeners were removed - expect(mockStdin.listenerCount('data')).toBe(0); - expect(mockStdin.listenerCount('error')).toBe(0); - expect(mockStdout.listenerCount('drain')).toBe(0); - - // Check that streams were properly ended - expect(mockStdin.destroyed).toBe(true); - expect(mockStdout.destroyed).toBe(true); -});