Skip to content

Streamable HTTP - improve usability, fast mcp and auth #641

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

Merged
merged 106 commits into from
May 8, 2025
Merged

Conversation

ihrpr
Copy link
Contributor

@ihrpr ihrpr commented May 6, 2025

  • introducing StreamableHTTPSessionManager
  • adding streamable http to fastmcp
  • adding auth

[on top of auth inspector changes + some changes to handle shttp]
image

bhosmer-ant
bhosmer-ant previously approved these changes May 7, 2025
Copy link
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

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

Super nice fastmcp glowup :) A couple comments inline and I wouldn't mind taking another pass in the morning if you're still working on it, but approving to unblock in the meantime

Base automatically changed from ihrpr/auth-example to main May 7, 2025 16:52
@ihrpr ihrpr dismissed bhosmer-ant’s stale review May 7, 2025 16:52

The base branch was changed.

@@ -669,6 +700,80 @@ async def sse_endpoint(request: Request) -> None:
debug=self.settings.debug, routes=routes, middleware=middleware
)

def streamable_http_app(self) -> Starlette:
Copy link

Choose a reason for hiding this comment

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

I'm not sure if you've thought about the use case, but we're hoping to be able to use one FastAPI app with multiple routes that all have separate stateless MCP servers.

On your branch this works:

 """Example Echo MCP Server."""

 from fastapi import APIRouter
 from mcp.server.fastmcp import FastMCP
 from starlette.applications import Starlette

 mcp = FastMCP(name="EchoServer")


 @mcp.tool(description="A simple echo tool")
 def echo(message: str) -> str:
     return f"Echo: {message}"


 # Get the Starlette app and convert to router
 app: Starlette = mcp.streamable_http_app()

with a parent app like

from routers import echo

app = FastAPI(lifespan=echo.app.router.lifespan_context)
app.mount("/echo", echo.app)

Although that involves kind of digging into your implementation to know that the lifespan function sets up the session manager and that it ends up internally in the starlette router. It might be helpful to have this exposed on the FastMCP object to make that more clear/explicit? either way it'll take a bit of documentation

from routers import echo

app = FastAPI(lifespan=lambda app: echo.mcp.session_manager.run())
app.mount("/echo", echo.mcp.streamable_http_app())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baxen, thank you, yeah, haven't considered that use case, would definitely add documentation!
Do you have any suggestions on how to make it more clear/explicit?

Copy link

Choose a reason for hiding this comment

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

i haven't thought of anything better than having the FastMCP object having the StreamableHTTPSessionManager as a member. The error it already raises when run hasn't been called is a good hint, and as a member on FastMCP it'll be easier to find it and call it's run method (like in that last snippet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, not super happy to add session_manager but also don't have a better option. Added to fast mcp, also adding to README this example: #664

thank you @baxen, let me know if this works!

Comment on lines 208 to 209
else: # transport == "streamable_http"
anyio.run(self.run_streamable_http_async)

Choose a reason for hiding this comment

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

nitpick: it might be better to have another elif here and then throw an exception in the else block. If someone introduces a new transport but misses updating this code they might believe their code is working even though it is really using a different transport.

Comment on lines +704 to +705
from starlette.middleware import Middleware
from starlette.routing import Mount

Choose a reason for hiding this comment

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

question: what's the intention behind importing these things inside this method? Is it to not force dev to have these dependencies if they are not using this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's right, if one only uses run(transport="stdio"), they never need those dependencies

Choose a reason for hiding this comment

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

got it -- makes sense and is pretty clean since this method is only invoked during server startup as far as i understand. we should just be careful that people dont start doing this in methods that are called after server startup since then import errors will only become visible in the runtime.

Comment on lines +86 to +87
# Store the task group for later use
self._task_group = tg

Choose a reason for hiding this comment

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

nitpick: Does this mean that StreamableHTTPSessionManager is not thread safe (two threads using same manager instance will overwrite eachother's self._task_group)? Might be worth documenting.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this

Copy link
Contributor

Choose a reason for hiding this comment

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

same, maybe not worth a fix but def worth a note in the comment header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, making only once instance being able to use run + docs

This was referenced May 8, 2025
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.

Looks good to me!

one concern around the .run() function being called on the same instance across multiple tasks, happy for you to do another loop on that (either document, fix, accept as is)

Comment on lines +86 to +87
# Store the task group for later use
self._task_group = tg
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this

bhosmer-ant
bhosmer-ant previously approved these changes May 8, 2025
Copy link
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

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

thread safety note on is prob worth it but new changes LGTM!

@ihrpr ihrpr dismissed stale reviews from bhosmer-ant and jerome3o-anthropic via 8c8d80d May 8, 2025 19:21
@ihrpr ihrpr merged commit e4e119b into main May 8, 2025
9 checks passed
@ihrpr ihrpr deleted the ihrpr/shttp branch May 8, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants