-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add async support to client_factory in FastMCPProxy (#1286) #1375
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
Conversation
src/fastmcp/server/proxy.py
Outdated
|
||
async def _get_client(self) -> Client: | ||
"""Gets a client instance by calling the sync or async factory.""" | ||
if inspect.iscoroutinefunction(self.client_factory): |
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.
Minor nit - the pattern we prefer elsewhere is I think
client = self.client_factory()
if inspect.isawaitable(client):
client = await client
return client
This way the factory can be any function that returns an awaitable (e.g. a sync-decorated coroutine, or a class).
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.
Updated _get_client to use the isawaitable pattern as suggested.
client = cast(Client, proxy.client_factory()) | ||
assert isinstance(client.transport, StreamableHttpTransport) | ||
assert client.transport.url == "http://example.com/mcp/" # type: ignore[attr-defined] | ||
|
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.
Could you add a simple test to ensure that async factories are executed?
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.
Added a test for async factory usage as well.
5ae06ff
to
3189d96
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.
Thank you!
This adds async support for client_factory in FastMCPProxy.
Related Issue #1286
The change:
Accepts either a sync or async factory
Keeps existing sync behavior unchanged