-
Notifications
You must be signed in to change notification settings - Fork 0
feat(client): default request_timeout applied at the REST seam #41
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,5 +1,6 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import functools | ||
| import time | ||
| from collections.abc import Iterator | ||
| from dataclasses import asdict, dataclass | ||
|
|
@@ -71,6 +72,45 @@ def to_dict(self) -> dict[str, Any]: | |
| return asdict(self) | ||
|
|
||
|
|
||
|
|
||
| def apply_default_request_timeout( | ||
| api_client: ApiClient, timeout: float | tuple[float, float] | ||
| ) -> None: | ||
| """Give every request through this client a socket-level deadline. | ||
|
|
||
| The generated client forwards ``_request_timeout=None`` — urllib3's | ||
| no-timeout — on every call unless the caller passes one explicitly, and | ||
| most helper methods expose no such knob. A stalled or black-holed server | ||
| therefore blocks the calling thread indefinitely. Wrapping the REST seam | ||
| applies ``timeout`` (seconds, or a ``(connect, read)`` pair) as the | ||
| default while still honoring an explicit per-call ``_request_timeout``. | ||
| """ | ||
| rest_client = api_client.rest_client | ||
| original = rest_client.request | ||
|
|
||
| @functools.wraps(original) | ||
| def request_with_default_timeout( | ||
| method, | ||
| url, | ||
| headers=None, | ||
| body=None, | ||
| post_params=None, | ||
| _request_timeout=None, | ||
| ): | ||
|
Comment on lines
+92
to
+99
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: (not blocking) This nested function is fully unannotated (params + return), which trips this repo's strict mypy config ( Separately, hard-coding the full |
||
| if _request_timeout is None: | ||
| _request_timeout = timeout | ||
| return original( | ||
| method, | ||
| url, | ||
| headers=headers, | ||
| body=body, | ||
| post_params=post_params, | ||
| _request_timeout=_request_timeout, | ||
| ) | ||
|
|
||
| rest_client.request = request_with_default_timeout | ||
|
|
||
|
|
||
| class HotdataClient: | ||
| """Thin wrapper around the Hotdata Python SDK with query polling helpers.""" | ||
|
|
||
|
|
@@ -81,6 +121,7 @@ def __init__( | |
| *, | ||
| host: str | None = None, | ||
| session_id: str | None = None, | ||
| request_timeout: float | tuple[float, float] | None = None, | ||
| ) -> None: | ||
| self._host = normalize_host(host) if host else default_host() | ||
| self._api_key = api_key | ||
|
|
@@ -94,6 +135,8 @@ def __init__( | |
| retries=default_http_retries(), | ||
| ) | ||
| self._api = ApiClient(self._config) | ||
| if request_timeout is not None: | ||
| apply_default_request_timeout(self._api, request_timeout) | ||
|
|
||
| @classmethod | ||
| def from_env(cls) -> HotdataClient: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| """The default request timeout must reach every call through the client. | ||
|
|
||
| The generated client sends ``_request_timeout=None`` (urllib3 no-timeout) | ||
| unless a caller passes one per call — and the helper methods expose no such | ||
| knob — so a stalled server blocks the calling thread indefinitely. | ||
| ``apply_default_request_timeout`` wraps the REST seam once at construction. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from hotdata_framework.client import HotdataClient, apply_default_request_timeout | ||
| from hotdata_framework.managed_client import ManagedDatabaseClient | ||
|
|
||
|
|
||
| class _FakeRest: | ||
| def __init__(self) -> None: | ||
| self.seen: list[object] = [] | ||
|
|
||
| def request(self, method, url, headers=None, body=None, post_params=None, _request_timeout=None): | ||
| self.seen.append(_request_timeout) | ||
| return "resp" | ||
|
|
||
|
|
||
| class _FakeApi: | ||
| def __init__(self) -> None: | ||
| self.rest_client = _FakeRest() | ||
|
|
||
|
|
||
| def test_default_applied_when_no_per_call_timeout() -> None: | ||
| api = _FakeApi() | ||
| rest = api.rest_client | ||
| apply_default_request_timeout(api, (10.0, 60.0)) | ||
|
|
||
| assert api.rest_client.request("GET", "http://x") == "resp" | ||
| assert rest.seen == [(10.0, 60.0)] | ||
|
|
||
|
|
||
| def test_explicit_per_call_timeout_wins() -> None: | ||
| api = _FakeApi() | ||
| rest = api.rest_client | ||
| apply_default_request_timeout(api, 60.0) | ||
|
|
||
| api.rest_client.request("GET", "http://x", _request_timeout=5.0) | ||
| assert rest.seen == [5.0] | ||
|
|
||
|
|
||
| def test_hotdata_client_wraps_its_rest_seam() -> None: | ||
| client = HotdataClient("k", "w", host="https://example.test", request_timeout=7.0) | ||
| # The wrapper is installed in place of the generated bound method. | ||
| assert client.api.rest_client.request.__name__ == "request" | ||
| assert client.api.rest_client.request.__wrapped__ is not None | ||
|
|
||
|
|
||
| def test_hotdata_client_without_timeout_is_untouched() -> None: | ||
| client = HotdataClient("k", "w", host="https://example.test") | ||
| assert not hasattr(client.api.rest_client.request, "__wrapped__") | ||
|
|
||
|
|
||
| def test_managed_client_threads_timeout_through() -> None: | ||
| client = ManagedDatabaseClient( | ||
| api_key="k", | ||
| workspace_id="w", | ||
| api_base_url="https://example.test", | ||
| max_retries=1, | ||
| retry_backoff_seconds=0.0, | ||
| request_timeout=(5.0, 30.0), | ||
| ) | ||
| assert client._runtime.api.rest_client.request.__wrapped__ is not None | ||
| client.close() |
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.
super nit: (not blocking) Three blank lines here — ruff (E303, which this repo selects) wants at most two between top-level definitions. Drop one.