Skip to content

StreamableHttp - Server transport with state management #553

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ihrpr
Copy link
Contributor

@ihrpr ihrpr commented Apr 21, 2025

Adding support for StreamableHTTP transport transport for MCP servers, providing bidirectional communication over
HTTP with streaming support.

Features

  • Session management
  • Implementation for server termination (DELETE)
  • Includes support for JSON and SSE response modes
  • Handles Related request IDs for notifications like logging and progress
  • Example of a server with tool notifications

Follow ups

  • Suport stateless mode
  • Support GET SSE
  • Streamable Http Client

#443

Copy link
Contributor

@jerome3o-anthropic jerome3o-anthropic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerns about related_request_id - seems like it's accidentally added. Otherwise looks good

# If no session ID provided but required, return error
if not request_session_id:
response = self._create_error_response(
"Bad Request: Missing session ID",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this error when I run the example server:

uv run mcp-simple-streamablehttp --port 3000 --log-level DEBUG

Then

curl http://localhost:3000/mcp/ -d '
{
    "jsonrpc": "2.0",
    "method": "initialize",
    "params": {
        "protocolVersion": "2025-03-26",
        "capabilities": {},
        "clientInfo": {
            "name": "curl-client",
            "version": "1.0.0"
        }
    }
}' \
    -H "Content-Type: application/json" \
    -H "Accept: application/json, text/event-stream" \
    -X POST | \
    jq

output:

{
  "jsonrpc": "2.0",
  "id": "server-error",
  "error": {
    "code": -32600,
    "message": "Bad Request: Missing session ID"
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is before I'm able to get the session from the server to start with

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I was missing the id, thats why. it mistook the message as a notification, and then didn't trigger the if is_initialization_request path. That could have a better error, but idt it should block here


app = Server("mcp-streamable-http-demo")

@app.call_tool()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit, also could be a follow up] Would be nice to use fastmcp here - i usually end up going:

server = FastMCP(...)
...
server._mcp_server # <- to access the lower level mcp interface

But perhaps we should just make it easier to get to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was quite confused here, we have two folders for servers: fastmcp and servers. I was planning to add fastmcp exaple as a follow up.

fastmcp might also help with related_request_id clarity - it's hidden under log. Low level server does not have a place where we can nicely inject related_request_id

@ihrpr
Copy link
Contributor Author

ihrpr commented Apr 23, 2025

@jerome3o-anthropic, very good point on notification params, somehow I completely ignored that fact it's part of the spec 🙈

What do you think about using meta field to propagate related_request_id?

@ihrpr
Copy link
Contributor Author

ihrpr commented Apr 27, 2025

@jerome3o-anthropic, very good point on notification params, somehow I completely ignored that fact it's part of the spec 🙈

What do you think about using meta field to propagate related_request_id?

I think I'd like to change the approach to use clean separation: #590

Copy link
Contributor

@jerome3o-anthropic jerome3o-anthropic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with the assumption we're gonna tidy up the related_request_id/_meta stuff.

@ihrpr ihrpr added this to the 2025-03-26 spec release milestone Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants