Skip to content

Conversation

bianning
Copy link
Contributor

@bianning bianning commented Aug 5, 2025

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

@github-actions github-actions bot added the tests label Aug 5, 2025

async def _get_client(self) -> Client:
"""Gets a client instance by calling the sync or async factory."""
if inspect.iscoroutinefunction(self.client_factory):
Copy link
Owner

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).

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 _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]

Copy link
Owner

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?

Copy link
Contributor Author

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.

@bianning bianning force-pushed the async_client_factory branch from 5ae06ff to 3189d96 Compare August 6, 2025 14:14
Copy link
Owner

@jlowin jlowin left a comment

Choose a reason for hiding this comment

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

Thank you!

@jlowin jlowin added the enhancement Improvement to existing functionality. For issues and smaller PR improvements. label Aug 6, 2025
@jlowin jlowin merged commit 78e9bab into jlowin:main Aug 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality. For issues and smaller PR improvements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants