-
Notifications
You must be signed in to change notification settings - Fork 46
feat: refactor SSE transport for multiple connections and add prepare script #69
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: main
Are you sure you want to change the base?
Conversation
Hi there, I’d like to enhance the SSE transport feature. Currently, it throws a 409 error when more than one client connects to the server simultaneously. Could you please review this and let me know if there’s anything I can do to improve it? I’m open to making updates as needed. |
@lmquang Hey, thanks for your PR. I appreciate it a lot. This is awesome stuff. Could you help me come up with a testing plan for this? I want to make sure it's robust - do you mind walking me through how you tested this? No worries if not, I can test this independently too but it would help me review this faster. Does the 409 error on multiple client connection resolve when applying your code? |
Here is a test scenario to compare before and after the SSE improvement Both old and new version will use the same configuration: // --- Configure Transport ---
let transportConfig: any;
const port = args['--port'] || 3001;
const host = args['--host'] || '0.0.0.0';
if (args['--transport'] === 'sse') {
logger.info(`\nConfiguring server for SSE transport on ${host}:${port}...`);
transportConfig = {
type: "sse", // Based on example
options: {
port: port,
host: host,
// Add other relevant options from example if needed (cors, auth, etc.)
cors: { // Basic CORS for broad compatibility
allowOrigin: "*",
allowMethods: "GET, POST, DELETE, OPTIONS",
allowHeaders: "Content-Type, Accept, Authorization, x-api-key, Mcp-Session-Id, Last-Event-ID",
exposeHeaders: "Content-Type, Authorization, x-api-key, Mcp-Session-Id",
maxAge: "86400"
}
}
};
} Root CauseThe server uses the initial Test ScenarioThe following recording (using mcp-framework v0.2.11) illustrates the issue:
mcp-framework-v0_2_11.mp4Test Scenario (with fix)The following recording (using my modifications) demonstrates the fix:
mcp-framework-improvement.mp4NoteTo avoid confusion with the sessionId in the MCP framework, I’ve replaced it with connectionId. The connectionId is a unique identifier for each client connection, while the sessionId is a unique identifier for the MCP server. This change could be beneficial for future scalability features. |
I apologize for the poor recording quality, as the GitHub file size is only 10 MB. |
This is a very smart way to approach it, thanks for the details. I will review again with more time tomorrow and merge. Thanks. Are you on our discord server? |
thank you @QuantGeekDev , could you send me link to your discord server? |
This PR introduces two key improvements to the
mcp-framework
:Multiple SSE Connection Support:
SSEServerTransport
(src/transports/sse/server.ts
) to handle multiple simultaneous client connections, removing the previous limitation of only supporting a single connection at a time.connectionId
.connectionId
(passed as thesessionId
query parameter) to validate the request against an active connection.send()
now correctly iterates over all active connections.Add
prepare
Script for Git Installs:prepare
script (npm run build
) topackage.json
.tsc
) when installed directly from a Git repository (e.g., using a GitHub URL independencies
). This resolves issues where consuming projects would fail to build due to missing compiled files (dist
folder) or type declarations.These changes enhance the robustness and usability of the SSE transport, particularly for scenarios involving multiple clients, and improve the developer experience when using the framework directly from Git.