Skip to content

docs: correct create_mcp_http_client default timeout docstring#2683

Open
aryanmotgi wants to merge 2 commits into
modelcontextprotocol:mainfrom
aryanmotgi:fix/httpx-client-default-timeout-docstring
Open

docs: correct create_mcp_http_client default timeout docstring#2683
aryanmotgi wants to merge 2 commits into
modelcontextprotocol:mainfrom
aryanmotgi:fix/httpx-client-default-timeout-docstring

Conversation

@aryanmotgi
Copy link
Copy Markdown

Summary

The docstring for create_mcp_http_client in src/mcp/shared/_httpx_utils.py claims the default timeout is "30 seconds if not specified". However, the implementation sets:

kwargs[\"timeout\"] = httpx.Timeout(MCP_DEFAULT_TIMEOUT, read=MCP_DEFAULT_SSE_READ_TIMEOUT)
# i.e. httpx.Timeout(30.0, read=300.0)

So the read timeout is actually 5 minutes (to accommodate long-lived SSE streams), not 30 seconds. This is an inaccuracy users will hit if they rely on the docstring to reason about long-running requests.

This PR updates the two affected lines in the docstring to reflect what the code actually does. No behavior change.

Test plan

  • Docstring-only change; no functional impact
  • Matches the real defaults (MCP_DEFAULT_TIMEOUT = 30.0, MCP_DEFAULT_SSE_READ_TIMEOUT = 300.0) defined at the top of the same file

The docstring claimed the default timeout is 30 seconds, but the
implementation sets httpx.Timeout(30, read=300) so the read timeout is
actually 5 minutes to accommodate long-lived SSE streams. Update the
docstring to reflect the real defaults.
Copy link
Copy Markdown

@Shivansh12t Shivansh12t left a comment

Choose a reason for hiding this comment

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

Good catch, the previous wording implied a flat 30s timeout, which would be surprising to anyone debugging a dropped SSE connection or trying to understand why httpx.Timeout(30) behaves differently from the default. The split between connect/write/pool (30s) and read (300s) is the actual contract here, so surfacing it in the docstring is worth it.
One minor suggestion: the inline comment and the Args: block now say essentially the same thing in slightly different phrasing. Could consolidate to just the Args: section and drop the duplicate from the overview bullet, or make the bullet a one-liner that points to the rationale (SSE streams) without restating the numbers.

@aryanmotgi
Copy link
Copy Markdown
Author

Good call — consolidated the overview bullet to a one-liner that points to the Args section, which now carries the actual numbers and rationale. Pushed in 452b15e.

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.

2 participants