feat: add async HTTP client support - #49
Conversation
📝 WalkthroughWalkthroughChangesAsync HTTP client migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Gateway
participant AsyncHTTPClient
participant httpxAsyncClient
Gateway->>AsyncHTTPClient: process_payment_async request
AsyncHTTPClient->>httpxAsyncClient: request with URL, headers, and JSON payload
httpxAsyncClient-->>AsyncHTTPClient: httpx.Response
AsyncHTTPClient-->>Gateway: parsed response
Gateway->>AsyncHTTPClient: aclose()
AsyncHTTPClient->>httpxAsyncClient: aclose()
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/shade/http.py (2)
549-568: 🩺 Stability & Availability | 🔵 TrivialClosing the shared
httpx.AsyncClientwhile requests are in-flight will raiseRuntimeError.
aclose()/__aexit__unconditionally close and null outself._client. If another coroutine is mid-request()on the sameAsyncHTTPClientinstance (e.g.Gateway.aclose()ingateway.pycalled concurrently withprocess_payment_async), httpx raisesRuntimeError: The connection pool was closed while ... requests/responses were still in-flight, per httpx's documented client lifecycle semantics. This is a legitimate design point for callers sharing oneGateway/AsyncHTTPClientacross concurrent tasks — worth documenting (or guarding with an in-flight request counter) rather than only in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/http.py` around lines 549 - 568, Document the concurrency contract around AsyncHTTPClient.aclose() and __aexit__: callers must not close the client while requests started through _get_client/request are still in flight, including shared Gateway usage. Add the guidance to the relevant method or class docstrings without changing the existing lifecycle behavior.
231-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
_retry_with_backoffhelper.
_retry_with_backoffis defined only insrc/shade/http.pyand is not referenced elsewhere; the sync and async request paths implement their own retry loops instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/http.py` around lines 231 - 248, Remove the unused _retry_with_backoff function, including its retry loop and docstring, from the module. Leave the existing synchronous and asynchronous request retry implementations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shade/http.py`:
- Around line 123-129: Update _is_retryable_transport_error to treat the broader
httpx.NetworkError hierarchy as retryable, while preserving the existing
timeout, built-in transport, and urllib.error.URLError handling. This shared
helper must cover httpx.ReadError, httpx.WriteError, and httpx.CloseError for
both SyncHTTPClient.request and AsyncHTTPClient.request.
---
Nitpick comments:
In `@src/shade/http.py`:
- Around line 549-568: Document the concurrency contract around
AsyncHTTPClient.aclose() and __aexit__: callers must not close the client while
requests started through _get_client/request are still in flight, including
shared Gateway usage. Add the guidance to the relevant method or class
docstrings without changing the existing lifecycle behavior.
- Around line 231-248: Remove the unused _retry_with_backoff function, including
its retry loop and docstring, from the module. Leave the existing synchronous
and asynchronous request retry implementations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f41ec26-ceae-4c0d-bef1-1f658bf9d043
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
README.mdTODO.mdpyproject.tomlsrc/shade/gateway.pysrc/shade/http.pytests/test_rate_limit.py
| def _is_retryable_transport_error(exc: Exception) -> bool: | ||
| """Return True for transient network failures that should be retried.""" | ||
| if httpx is not None and isinstance(exc, (httpx.ConnectError, httpx.TimeoutException)): | ||
| return True | ||
|
|
||
| try: | ||
| import aiohttp | ||
| except ImportError: | ||
| aiohttp = None | ||
|
|
||
| if aiohttp is not None and isinstance( | ||
| exc, | ||
| ( | ||
| aiohttp.ClientConnectionError, | ||
| aiohttp.ClientConnectorError, | ||
| aiohttp.ClientOSError, | ||
| aiohttp.ServerDisconnectedError, | ||
| ), | ||
| ): | ||
| if isinstance(exc, (httpx.ConnectError, httpx.TimeoutException)): | ||
| return True | ||
|
|
||
| if isinstance(exc, (ConnectionResetError, TimeoutError, urllib.error.URLError)): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_is_retryable_transport_error misses common transient httpx network errors.
Only httpx.ConnectError and httpx.TimeoutException are treated as retryable. httpx.ConnectError is a subclass of httpx.NetworkError, which also covers httpx.ReadError, httpx.WriteError, and httpx.CloseError — all genuine transient network failures (e.g. a connection reset mid-response) that will now propagate unretried through both SyncHTTPClient.request and AsyncHTTPClient.request (both call this shared helper). This narrows retry coverage compared to the sync-side urllib.error.URLError catch-all on the same line.
🛡️ Proposed fix
- if isinstance(exc, (httpx.ConnectError, httpx.TimeoutException)):
+ if isinstance(exc, (httpx.NetworkError, httpx.TimeoutException)):
return True📝 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.
| def _is_retryable_transport_error(exc: Exception) -> bool: | |
| """Return True for transient network failures that should be retried.""" | |
| if httpx is not None and isinstance(exc, (httpx.ConnectError, httpx.TimeoutException)): | |
| return True | |
| try: | |
| import aiohttp | |
| except ImportError: | |
| aiohttp = None | |
| if aiohttp is not None and isinstance( | |
| exc, | |
| ( | |
| aiohttp.ClientConnectionError, | |
| aiohttp.ClientConnectorError, | |
| aiohttp.ClientOSError, | |
| aiohttp.ServerDisconnectedError, | |
| ), | |
| ): | |
| if isinstance(exc, (httpx.ConnectError, httpx.TimeoutException)): | |
| return True | |
| if isinstance(exc, (ConnectionResetError, TimeoutError, urllib.error.URLError)): | |
| return True | |
| return False | |
| def _is_retryable_transport_error(exc: Exception) -> bool: | |
| """Return True for transient network failures that should be retried.""" | |
| if isinstance(exc, (httpx.NetworkError, httpx.TimeoutException)): | |
| return True | |
| if isinstance(exc, (ConnectionResetError, TimeoutError, urllib.error.URLError)): | |
| return True | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shade/http.py` around lines 123 - 129, Update
_is_retryable_transport_error to treat the broader httpx.NetworkError hierarchy
as retryable, while preserving the existing timeout, built-in transport, and
urllib.error.URLError handling. This shared helper must cover httpx.ReadError,
httpx.WriteError, and httpx.CloseError for both SyncHTTPClient.request and
AsyncHTTPClient.request.
|
Hello @fathiaoyinloye |
|
Hello @fathiaoyinloye |
2 participants
Summary
Add an asynchronous HTTP client counterpart to the existing synchronous client, enabling the SDK to be used cleanly in async frameworks such as FastAPI, Starlette, and Django async views while preserving shared request construction and configuration behavior.
Changes
_AsyncHTTPClientusinghttpx.AsyncClient.request(...)support with the same response shape as the sync client.aclose()and async context manager support for proper client cleanup.pytest-asyncioand updated the lockfile.http.pythat caused a syntax error.Test plan
aclose()testedValidation:
178 passedSummary by CodeRabbit
New Features
Bug Fixes