-
Notifications
You must be signed in to change notification settings - Fork 4k
Document the timeout a hand-built httpx2 client needs #3618
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,16 @@ | ||
| """`docs/client/transports.md`: every claim the page makes, proved against the real SDK.""" | ||
|
|
||
| import inspect | ||
| from typing import Any | ||
|
|
||
| import httpx2 | ||
| import pytest | ||
|
|
||
| from docs_src.client_transports import tutorial001, tutorial004 | ||
| from docs_src.client_transports import tutorial001, tutorial002, tutorial003, tutorial004 | ||
| from mcp import Client | ||
| from mcp.client.stdio import get_default_environment | ||
| from mcp.client.streamable_http import streamable_http_client | ||
| from mcp.server import MCPServer | ||
|
|
||
| # See test_index.py for why this is a per-module mark and not a conftest hook. | ||
| pytestmark = [pytest.mark.anyio, pytest.mark.filterwarnings("error::mcp.MCPDeprecationWarning")] | ||
|
|
@@ -46,6 +49,40 @@ async def test_streamable_http_configuration_lives_on_the_httpx_client() -> None | |
| ] | ||
|
|
||
|
|
||
| async def test_the_timeout_on_the_page_is_the_sdk_clients_and_a_client_without_one_has_five_seconds( | ||
| monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] | ||
| ) -> None: | ||
| """tutorial002 and tutorial003: the `timeout=` tutorial003 passes is what `Client(url)` builds for itself | ||
| (30 seconds, 300 for reads); an `httpx2.AsyncClient` built without one has httpx2's 5-second default.""" | ||
| async with httpx2.AsyncClient() as bare: | ||
| assert bare.timeout == httpx2.Timeout(5.0) | ||
|
|
||
| mcp = MCPServer("Bookshop") | ||
|
|
||
| @mcp.tool() | ||
| def search_books(query: str) -> str: | ||
| """Search the catalog.""" | ||
| raise NotImplementedError | ||
|
|
||
| app = mcp.streamable_http_app() | ||
| built: list[httpx2.Timeout] = [] | ||
|
|
||
| class InProcessClient(httpx2.AsyncClient): | ||
| """Every `httpx2.AsyncClient` the two programs build, routed to the server above.""" | ||
|
|
||
| def __init__(self, **kwargs: Any) -> None: | ||
| super().__init__(transport=httpx2.ASGITransport(app=app), **kwargs) | ||
| built.append(self.timeout) | ||
|
|
||
| monkeypatch.setattr(httpx2, "AsyncClient", InProcessClient) | ||
| async with mcp.session_manager.run(): | ||
| await tutorial002.main() | ||
| await tutorial003.main() | ||
|
|
||
|
Comment on lines
+79
to
+81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): a maintainer whose change breaks the in-process handshake gets a hung CI job here instead of a failed test. The awaits of tutorial002.main() and tutorial003.main() at tests/docs_src/test_client_transports.py:79-81 are full client round trips over an ASGI transport with nothing bounding them, and httpx2.ASGITransport does not apply the client's timeout. Fix: wrap the two program runs (and the session_manager.run() block around them) in anyio.fail_after(5) so a stalled list_tools fails the test instead of stalling the run. Why this was flaggedThe new test enters mcp.session_manager.run() and awaits tutorial002.main() then tutorial003.main() at tests/docs_src/test_client_transports.py:79-81. Each main() does Client.list_tools() over streamable_http_client routed through httpx2.ASGITransport (tests/docs_src/test_client_transports.py:74). If the server never answers a POST or the session manager stalls, that await never returns: ClientSession has no default read timeout and the in-process ASGI transport does not enforce the Timeout(30.0, read=300.0) the client carries. AGENTS.md asks that indefinite waits be wrapped in anyio.fail_after(5) to prevent hangs; the test adds none, so a regression shows up as a hung job rather than a test failure. On the base branch this test does not exist, so no new unbounded wait was present. Verification: nit. Trigger: any future regression that stalls the streamable-HTTP handshake or a tools/list POST turns this test into a hang rather than a failure. The test at tests/docs_src/test_client_transports.py:78-80 runs |
||
| assert capsys.readouterr().out == "['search_books']\n['search_books']\n" | ||
| assert built == [httpx2.Timeout(30.0, read=300.0), httpx2.Timeout(30.0, read=300.0)] | ||
|
|
||
|
|
||
| async def test_stdio_parameters_go_straight_to_client() -> None: | ||
| """tutorial004: `Client` takes the `StdioServerParameters` directly, and nothing is spawned until you enter it.""" | ||
| client = Client(tutorial004.server) | ||
|
|
||
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.
P2: This overstates
httpx2’s read timeout as a maximum tool-call duration. A call can run longer than five seconds when response data or keepalives arrive within the read interval, while a shorter call can time out if an HTTP read is idle; document the idle-read condition instead.Prompt for AI agents