Skip to content

feat(python): support timeout=None to disable timeout#402

Merged
barjin merged 11 commits intomasterfrom
copilot/support-none-as-timeout
Mar 6, 2026
Merged

feat(python): support timeout=None to disable timeout#402
barjin merged 11 commits intomasterfrom
copilot/support-none-as-timeout

Conversation

Copy link
Contributor

Copilot AI commented Mar 5, 2026

timeout=None on per-request methods fell back to the client-level default instead of disabling the timeout, making it impossible to issue truly unbounded requests and diverging from HTTPX conventions.

Core changes

  • RequestOptions.timeout changed from Option<Duration> to Option<Option<Duration>>:
    • None → inherit client default (unchanged)
    • Some(None) → disable timeout entirely
    • Some(Some(d)) → specific override
  • impit.rs: maps Some(None)Duration::MAX before handing off to reqwest (the only per-request escape hatch reqwest exposes without rebuilding the client)
  • impit-node: trivial one-liner wrap of the existing value in Some(…)

Python binding changes

Distinguishing "not passed" from "explicitly None" requires a sentinel because PyO3 maps both to Rust None for Option<f64>. Solution:

  • Per-request timeout parameter changed from Option<f64> to Py<PyAny> with an interned string sentinel ("__impit_use_client_default__") as the PyO3 default
  • timeout_from_pyobj() parses the three states; default_timeout() returns the sentinel for use in #[pyo3(signature = (...))]
  • USE_CLIENT_DEFAULT is exposed as a public module constant (matching HTTPX's convention) so callers can pass it explicitly

Usage

from impit import Client, USE_CLIENT_DEFAULT

client = Client(timeout=5.0)

client.get(url)                          # uses 5s client default
client.get(url, timeout=USE_CLIENT_DEFAULT)  # same — explicit form
client.get(url, timeout=None)            # no timeout (wait indefinitely)
client.get(url, timeout=30.0)            # 30s override

Type stubs and docstrings updated to reflect the new timeout: float | None = USE_CLIENT_DEFAULT signature on all per-request methods.

Original prompt

This section details on the original issue you should resolve

<issue_title>Support None as timeout to disable timeout entirely</issue_title>
<issue_description>Currently, passing timeout=None to Client.request() / AsyncClient.request() falls back to the client-level default (30s) because the Rust side skips client_request.timeout() when the value is None:

// impit.rs:443-444
if let Some(timeout) = timeout {
    client_request = client_request.timeout(timeout);
}

HTTPX treats timeout=None as "no timeout" (wait indefinitely). Since impit's Python API aims to follow HTTPX conventions, it would be good to match this behavior.

Proposed: When timeout=None is passed per-request, actively disable the timeout instead of falling back to the client default.</issue_description>

<agent_instructions>Implement the changes in impit-python. Make minimal changes to achieve the goal. Do not add excessive comments. Make sure to describe the new behaviour in the docblock (or generated documentation). Use conventional commits for the commit messages and PR titles. </agent_instructions>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits March 5, 2026 10:20
When `timeout=None` is explicitly passed to any per-request method, the
timeout is now actively disabled (wait indefinitely), matching HTTPX
behaviour.

A `USE_CLIENT_DEFAULT` sentinel constant is introduced so users can
also explicitly say "use the client-level timeout" when needed.

Changes:
- `impit/src/request.rs`: `RequestOptions.timeout` is now
  `Option<Option<Duration>>` (None = use default, Some(None) = disabled,
  Some(Some(d)) = specific value)
- `impit/src/impit.rs`: translates the three-state timeout before
  passing to reqwest
- `impit-node/src/lib.rs`: trivially wraps the existing timeout value
  in `Some(…)` to match the new type
- `impit-python/src/request.rs`: adds `USE_CLIENT_DEFAULT_SENTINEL`,
  `default_timeout()` and `timeout_from_pyobj()` helpers
- `impit-python/src/client.rs` / `async_client.rs` / `lib.rs`:
  per-request `timeout` parameter changed from `Option<f64>` to
  `Py<PyAny>` with the sentinel as default; parsed via `timeout_from_pyobj`
- `impit-python/python/impit/__init__.py`: exports `USE_CLIENT_DEFAULT`
- `impit-python/python/impit/impit.pyi`: updated type stubs and docs
- `impit-python/test/basic_client_test.py`: new `TestTimeoutBehaviour` class

Co-authored-by: barjin <61918049+barjin@users.noreply.github.com>
… quality

Co-authored-by: barjin <61918049+barjin@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix timeout handling for None value in requests feat(python): support timeout=None to disable timeout per-request Mar 5, 2026
…L-acquired sentinel

Replace Py<PyAny> timeout parameters with Option<Either<f64, &str>> leveraging pyo3's built-in either feature for type extraction. This removes the default_timeout() function that acquired the GIL just to create a Python string.
@github-actions github-actions bot added this to the 135th sprint - Tooling team milestone Mar 6, 2026
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Mar 6, 2026
Comment on lines +17 to +20
/// - `None` — inherit the client-level default timeout set via [`ImpitBuilder::with_default_timeout`](crate::impit::ImpitBuilder::with_default_timeout).
/// - `Some(None)` — disable the timeout entirely for this request (wait indefinitely).
/// - `Some(Some(d))` — use the given duration, overriding the client-level default.
pub timeout: Option<Option<Duration>>,
Copy link
Member

Choose a reason for hiding this comment

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

This is rather messy and a candidate for refactoring, but I suppose it's as good as we can do without changing the API too much.

@barjin barjin marked this pull request as ready for review March 6, 2026 10:13
@barjin barjin changed the title feat(python): support timeout=None to disable timeout per-request feat(python): support timeout=None to disable timeout Mar 6, 2026
@barjin barjin merged commit 2f553b6 into master Mar 6, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support None as timeout to disable timeout entirely

2 participants