Remove OpenAPI timeout parameter, make client optional, surface timeout errors#3067
Remove OpenAPI timeout parameter, make client optional, surface timeout errors#3067
Conversation
jlowin
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38390ce788
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if client is None: | ||
| client = self._create_default_client(openapi_spec) | ||
| self._client = client |
There was a problem hiding this comment.
When client is omitted, the provider constructs its own httpx.AsyncClient, but OpenAPIProvider does not override lifespan() to close it, so servers created with the default client will leave connections open on shutdown (often emitting ResourceWarning and leaking sockets in long-lived processes). Consider tracking ownership of the created client and calling await client.aclose() in a provider lifespan teardown.
Useful? React with 👍 / 👎.
WalkthroughThis PR removes the Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @classmethod | ||
| def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient: | ||
| """Create a default httpx client from the OpenAPI spec's server URL.""" | ||
| servers = openapi_spec.get("servers", []) | ||
| if not servers or not servers[0].get("url"): | ||
| raise ValueError( | ||
| "No server URL found in OpenAPI spec. Either add a 'servers' " | ||
| "entry to the spec or provide an httpx.AsyncClient explicitly." | ||
| ) | ||
| base_url = servers[0]["url"] | ||
| return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
httpx.AsyncClient base_url absolute relative URL requirements
💡 Result:
base_urlonhttpx.AsyncClientis used only to resolve request URLs that are relative (no scheme/host). It’s “a URL to use as the base when building request URLs”, and the requesturlis “merged with anybase_urlset on the client.” [1]- If you pass an absolute request URL (e.g.
https://api.example.com/v1/users), it does not needbase_urland will effectively ignore it (no “merging” needed). [1] - If you pass a relative request URL (e.g.
"/v1/users"or"v1/users"), you must setbase_url(common example: in ASGI tests,base_url="http://testserver"), otherwise relative URLs like"/"won’t work. [2] - Practically,
base_urlshould itself be an absolute URL (include scheme + host, likehttps://api.example.com), because it’s the base used to construct the final absolute request URL. [1], [2]
Sources: HTTPX API reference for AsyncClient/build_request and base_url merging behavior [1]; Starlette docs noting you must set base_url for relative URLs with httpx.AsyncClient [2].
🌐 Web query:
httpx base_url template variables behavior
💡 Result:
httpx does not support “URL template variables” (like {version} / {id}) in base_url (or request URLs). It treats { and } as literal characters in the URL, and—because they’re not generally safe URL characters—clients commonly percent-encode them to %7B and %7D rather than interpreting them as placeholders. [2], [3]
What base_url does do is standard base + relative URL resolution: it prepends a base to outgoing requests when you pass a relative URL (e.g. client.get("/headers")). [1], [2]
So, instead of:
httpx.Client(base_url="https://api.example.com/{version}")
client.get("/users/{id}")you should fill variables yourself first, e.g.:
version = "v1"
user_id = 123
with httpx.Client(base_url=f"https://api.example.com/{version}") as client:
r = client.get(f"/users/{user_id}")Sources: HTTPX base_url client behavior [1], HTTPX URL parsing/encoding behavior [2], curly braces are typically percent-encoded in URLs [3].
[1] https://www.python-httpx.org/advanced/clients/
[2] https://www.encode.io/httpnext/urls
[3] https://stackoverflow.com/questions/53766725/curl-does-not-work-with-urls-with-curly-braces-in-parameters
Handle templated and relative server URLs before constructing the default client.
OpenAPI server URLs can be templated (e.g., "{protocol}://api.example.com/{version}") or relative (e.g., "/api"). The httpx.AsyncClient requires base_url to be absolute and does not support URL template variables—passing templated URLs will cause them to be percent-encoded as literal characters, resulting in invalid requests. Relative URLs will also cause failures.
Detect and resolve server URL variables against the OpenAPI variables object, validate that the result is absolute, and raise a clear error if not possible so users know to pass a client explicitly.
🔧 Suggested hardening for default client creation
+from string import Formatter
@@
`@classmethod`
def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient:
"""Create a default httpx client from the OpenAPI spec's server URL."""
servers = openapi_spec.get("servers", [])
- if not servers or not servers[0].get("url"):
- raise ValueError(
- "No server URL found in OpenAPI spec. Either add a 'servers' "
- "entry to the spec or provide an httpx.AsyncClient explicitly."
- )
- base_url = servers[0]["url"]
+ if not servers or not servers[0].get("url"):
+ raise ValueError("Missing OpenAPI server URL; provide an AsyncClient.")
+ server = servers[0]
+ base_url = server["url"]
+ if "{" in base_url:
+ variables = server.get("variables") or {}
+ formatter = Formatter()
+ parts: list[str] = []
+ for literal, field_name, *_ in formatter.parse(base_url):
+ parts.append(literal)
+ if field_name:
+ default = variables.get(field_name, {}).get("default")
+ if default is None:
+ raise ValueError(
+ f"Server URL variable '{field_name}' has no default; "
+ "provide an AsyncClient explicitly."
+ )
+ parts.append(str(default))
+ base_url = "".join(parts)
+ if not base_url.startswith(("http://", "https://")):
+ raise ValueError(
+ "Server URL must be absolute (http/https); provide an AsyncClient."
+ )
return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @classmethod | |
| def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient: | |
| """Create a default httpx client from the OpenAPI spec's server URL.""" | |
| servers = openapi_spec.get("servers", []) | |
| if not servers or not servers[0].get("url"): | |
| raise ValueError( | |
| "No server URL found in OpenAPI spec. Either add a 'servers' " | |
| "entry to the spec or provide an httpx.AsyncClient explicitly." | |
| ) | |
| base_url = servers[0]["url"] | |
| return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT) | |
| `@classmethod` | |
| def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient: | |
| """Create a default httpx client from the OpenAPI spec's server URL.""" | |
| servers = openapi_spec.get("servers", []) | |
| if not servers or not servers[0].get("url"): | |
| raise ValueError("Missing OpenAPI server URL; provide an AsyncClient.") | |
| server = servers[0] | |
| base_url = server["url"] | |
| if "{" in base_url: | |
| variables = server.get("variables") or {} | |
| formatter = Formatter() | |
| parts: list[str] = [] | |
| for literal, field_name, *_ in formatter.parse(base_url): | |
| parts.append(literal) | |
| if field_name: | |
| default = variables.get(field_name, {}).get("default") | |
| if default is None: | |
| raise ValueError( | |
| f"Server URL variable '{field_name}' has no default; " | |
| "provide an AsyncClient explicitly." | |
| ) | |
| parts.append(str(default)) | |
| base_url = "".join(parts) | |
| if not base_url.startswith(("http://", "https://")): | |
| raise ValueError( | |
| "Server URL must be absolute (http/https); provide an AsyncClient." | |
| ) | |
| return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT) |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 167-170: Avoid specifying long messages outside the exception class
(TRY003)
1 participant
OpenAPI tools generated from specs were hitting httpx's default 5-second timeout with no useful error message — just
"Request error: "with nothing after the colon. This came fromhttpx.ReadTimeoutstringifying to an empty string.The
timeoutparameter onOpenAPIProvider/FastMCP.from_openapi()/from_fastapi()was accepted and stored but never actually applied to the HTTP call for tools (it worked for resources, inconsistently). Rather than fix a broken parameter, timeout is now controlled entirely through the httpx client, which is how httpx is designed to work.The
clientparameter is now optional — when omitted, a default client is created from the spec'sservers[0].urlwith a 30-second timeout:Timeout errors now surface clearly as
"HTTP request timed out (ReadTimeout)"instead of the empty message.Breaking:
timeoutparameter removed fromOpenAPIProvider,FastMCP.from_openapi(),FastMCP.from_fastapi(), andFastMCPOpenAPI.