-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Switch Lifespan to being a Server Lifespan #2013
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
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.
Pull Request Overview
This PR adds server-wide lifespan management to the FastMCP
class by introducing a server_lifespan
parameter that transforms the class into an async context manager. The existing lifespan
parameter now forwards to the deprecated session_lifespan
parameter for backward compatibility.
Key changes:
- Introduces
server_lifespan
parameter as an alternative to session-based lifespan management - Makes
FastMCP
an async context manager with__aenter__
and__aexit__
methods - Implements mutual exclusivity between
server_lifespan
andsession_lifespan
parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
tests/server/test_server_lifespan.py |
Comprehensive test suite covering server lifespan functionality, session lifespan deprecation, and parameter conflicts |
tests/server/test_mount.py |
Test ensuring mounted servers with server_lifespan default to proxy mode |
test_log.py |
Simple example demonstrating server lifespan usage |
src/fastmcp/server/server.py |
Core implementation of server lifespan functionality and async context manager |
src/fastmcp/client/transports.py |
Transport layer updates to handle server lifespan initialization |
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
5a53a93
to
6402e33
Compare
6402e33
to
ac067ed
Compare
b95feaa
to
c33d601
Compare
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
c0ef5fc
to
d134310
Compare
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.
I'm having trouble tracking the purpose of the _server_lifespan_proxy_factory (and, relatedly, the lifespan stack result). It's also unclear to me why server lifespans and session lifespans are exclusive -- I think you have an elegant approach to treating the server lifespan as a context manager for the server, why not let the SDK also have its session-level lifespan run?
@jlowin the current way users access the lifespan_context is via the RequestContext object from the underlying SDK
So the way it's currently implemented is that you're picking whether An alternative implementation could be that we expose the server_lifespan on the FastMCP Context object so that server_lifespan is available via the FastMCP Context and the session_lifespan is available via the MCP request context. I am a bit biased here -- I think the session_lifespan is useless and so was mostly trying to push this to work the way I think most people think it should work. |
I see I see -- in that case I'm tempted to say lets just remove the I think in this case we'd make the lifespan object availabl eon the FastMCP context rather than messing with the SDK object. |
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
@jlowin I think this version should be a lot easier to reason about. Switching from session to server lifespan is a breaking change though |
async with AsyncExitStack() as stack: | ||
context = await stack.enter_async_context(lifespan(app)) | ||
yield context | ||
if fastmcp_server._lifespan == default_lifespan: |
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.
does this equality work for context managers? does it need to be is
?
This looks good to me -- since this is a behavior change on the same kwarg, I don't see a great way to make it deprecate gracefully and I think calling it out as a breaking behavioral change (but not an error) is the right way to handle over the 2.13 boundary. |
As an end user, absolutely love this change! @strawgate Would it be possible to also update the docs as part of this PR itself in case anyone wants to try it right away? Since it's a breaking change, people will scratch their heads if there's no docs to specify the new behavior. |
Feel free to try this branch, the current code on the branch actually only uses I'll update docs but the note about it being a breaking change will be in the release notes |
Already uv synced the branch and it worked like a charm! 🔥 |
Description
Change lifespan on the server to be a server lifespan instead of a session lifespan
Closes #2012
Closes #166