Make network timeout configurable #141
Conversation
Summary by CodeRabbit
WalkthroughMoves network timeout configuration to the Client instance: adds DEFAULT_NETWORK_TIMEOUT = 10.0, extends Client.init with timeout, passes timeout into Request, and refactors Request to store and use an instance timeout and instance-bound get/post callbacks (removes module-level NETWORK_TIMEOUT). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant Request
participant HTTP
User->>Client: Client(key, secret, timeout=...)
Client->>Request: Request(middlewares, timeout)
Client->>HTTP: Acquire token (timeout=Client.timeout)
User->>Client: client.get/post(...)
Client->>Request: _request(method, url, data)
Request->>Request: apply pre-middlewares
Request->>HTTP: _get/_post (timeout=Request.timeout)
HTTP-->>Request: response
Request->>Request: apply post-middlewares
Request-->>Client: response
Client-->>User: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
epo_ops/models.py (1)
111-116: Fix syntax error and avoid duplicate 'timeout' kwarg in requests callsCurrent calls use
**kwargs, timeout=self.timeout, which is invalid syntax and can also conflict iftimeoutis present inkwargs. Set it on kwargs first, then unpack.- def _post_callback(self, url, data, **kwargs): - return requests.post(url, data, **kwargs, timeout=self.timeout) + def _post_callback(self, url, data, **kwargs): + if self.timeout is not None: + kwargs.setdefault("timeout", self.timeout) + return requests.post(url, data=data, **kwargs) - def _get_callback(self, url, data, **kwargs): - return requests.get(url, **kwargs, timeout=self.timeout) + def _get_callback(self, url, data, **kwargs): + if self.timeout is not None: + kwargs.setdefault("timeout", self.timeout) + return requests.get(url, **kwargs)
🧹 Nitpick comments (3)
epo_ops/api.py (3)
38-38: Consider typing the timeout to match requests’ accepted typesRequests accepts float or (connect, read) tuple. Typing this improves clarity.
-from typing import List, Optional, Union +from typing import List, Optional, Union, Tuple @@ - def __init__(self, key, secret, accept_type="xml", middlewares=None, timeout=DEFAULT_NETWORK_TIMEOUT): + def __init__(self, key, secret, accept_type="xml", middlewares=None, timeout: Union[float, Tuple[float, float]] = DEFAULT_NETWORK_TIMEOUT):
43-43: Name the timeout argument when constructing RequestMinor readability and future-proofing against positional changes.
- self.request = Request(self.middlewares, timeout) + self.request = Request(self.middlewares, timeout=timeout)
46-46: Optionally assign self.timeout before creating RequestPurely for readability so the instance state is set before dependent components are built.
- self.request = Request(self.middlewares, timeout) - self.key = key - self.secret = secret - self.timeout = timeout + self.key = key + self.secret = secret + self.timeout = timeout + self.request = Request(self.middlewares, timeout=timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
epo_ops/api.py(3 hunks)epo_ops/models.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
epo_ops/api.py (3)
epo_ops/middlewares/throttle/throttler.py (1)
Throttler(13-26)epo_ops/models.py (1)
Request(66-115)tests/conftest.py (2)
default_client(35-40)reset_cached_client(13-21)
🔇 Additional comments (2)
epo_ops/models.py (1)
86-90: Good refactor to instance-bound callbacksDispatching via
self._post_callback/self._get_callbackenables per-instance timeouts cleanly.epo_ops/api.py (1)
355-356: Token acquisition now honors per-instance timeout — goodUsing
self.timeouthere aligns token requests with the client’s configured timeout.
This also increases the default to 30 seconds
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
===========================================
- Coverage 99.31% 77.72% -21.59%
===========================================
Files 18 18
Lines 438 440 +2
===========================================
- Hits 435 342 -93
- Misses 3 98 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@amotl thanks for merging this. Much appreciated. |
2 participants
Over the last few days we've noticed that the default timeout of 10 seconds was a bit too low. This PR makes this value configurable.
and increases it to 30 seconds. Originally I did not want to increase the default value, but then ran into timeouts when running the tests locally so decided to bump it to 30 seconds.Default timeout of 10 seconds was kept.Fixes #140