Conversation
- Use shared httpx.AsyncClient with connection pooling instead of creating a new client per request (Python) - Add 30s timeout to all HTTP requests (both TypeScript and Python) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review ReportProject: x402 / BankOfAI SDK PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR improves reliability of the GasFree API client across both the Python and TypeScript SDKs through two distinct mechanisms: (1) switching the Python client from ephemeral per-request The TypeScript change is clean, minimal, and correct. The Python change, however, introduces a resource leak: the persistent Change Summary1. Python – Persistent
|
| File | Change Type | Description |
|---|---|---|
python/x402/src/bankofai/x402/utils/gasfree.py |
Modified | Replace four async with httpx.AsyncClient() as client: blocks with a single persistent self._client created in __init__, and remove per-method indentation nesting |
Purpose: Eliminate per-request TCP handshakes by reusing a connection pool, and add a uniform 30-second timeout to every outbound API call.
2. TypeScript – Timeout signal on all fetch calls
| File | Change Type | Description |
|---|---|---|
typescript/packages/x402/src/utils/gasfree.ts |
Modified | Add DEFAULT_TIMEOUT_MS = 30000 constant and apply signal: AbortSignal.timeout(DEFAULT_TIMEOUT_MS) to getProviders, getAddressInfo, getStatus, and submit |
Purpose: Prevent fetch from hanging indefinitely when the GasFree API is unresponsive.
Detailed Findings
Major
[MJ-01] Python httpx.AsyncClient Is Created but Never Closed — Resource Leak
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Observability |
| File | python/x402/src/bankofai/x402/utils/gasfree.py : Line 44 |
Description
httpx.AsyncClient holds an internal connection pool, connection limits, and async transports that must be explicitly torn down. The old code used async with httpx.AsyncClient() as client: which guaranteed aclose() was called on every exit path. The new code assigns the client to self._client in __init__ but the class defines neither an aclose() / close() coroutine, nor __aenter__ / __aexit__ methods, nor a __del__ finaliser.
In production — where GasFreeAPIClient instances may be created once and live for the lifetime of a request handler or a process — the underlying connections will never be released. Python 3.12+ will raise ResourceWarning: Unclosed client in non-production environments; in production the warnings are silent but the leak is real and will eventually exhaust OS file descriptors.
Code
# __init__ creates the client …
self._client = httpx.AsyncClient(timeout=30.0)
# … but no corresponding teardown exists anywhere in the class.
# No aclose(), no __aenter__/__aexit__, no __del__.Recommendation
Add an aclose() coroutine and the async context-manager dunder methods so callers can control the lifecycle:
async def aclose(self) -> None:
await self._client.aclose()
async def __aenter__(self) -> "GasFreeAPIClient":
return self
async def __aexit__(self, *args: Any) -> None:
await self.aclose()Alternatively, accept an optional httpx.AsyncClient as a constructor parameter (dependency injection), which lets callers manage the lifecycle and makes unit tests simpler:
def __init__(
self,
base_url: str,
api_key: str | None = None,
api_secret: str | None = None,
*,
client: httpx.AsyncClient | None = None,
) -> None:
...
self._client = client or httpx.AsyncClient(timeout=30.0)
self._owns_client = client is None # only close if we created itMinor
[MN-01] Python Tests Create Unclosed httpx.AsyncClient Instances
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Testing |
| File | python/x402/tests/utils/test_gasfree_utils.py : Lines 16, 45, 62, 88, 105, 122, 133, 157, 170, 184, 210 |
Description
Every test that calls GasFreeAPIClient("https://api.example.com", ...) now triggers httpx.AsyncClient(timeout=30.0) in __init__. The tests never close these clients, so each test case leaks an unclosed AsyncClient. In Python 3.12+ this emits ResourceWarning: Unclosed client during the test run (visible with -W error::ResourceWarning). The tests were written against the old per-method async with pattern and have not been updated to reflect the new lifecycle requirement.
Code
# test_gasfree_utils.py – example (same pattern repeated in ~10 tests)
async def test_get_address_info(self):
client = GasFreeAPIClient("https://api.example.com") # creates AsyncClient internally
with patch("httpx.AsyncClient.get") as mock_get:
...
# client (and its internal AsyncClient) are never closedRecommendation
Add a fixture or teardown that calls await client.aclose() (once [MJ-01] is fixed), or use async with GasFreeAPIClient(...) as client: in every test that exercises an HTTP path.
[MN-02] Timeout Value Is Hardcoded with No Per-Instance Override
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Code Quality |
| File | python/x402/src/bankofai/x402/utils/gasfree.py : Line 44 · typescript/packages/x402/src/utils/gasfree.ts : Line 81 |
Description
Both SDKs adopt a fixed 30-second wall-clock timeout. There is no mechanism for library consumers to specify a different value. Thirty seconds may be too long for latency-sensitive payment flows and too short for environments with heavy network congestion or cold-start API servers. The TypeScript constant (DEFAULT_TIMEOUT_MS) uses the word "default" but there is no corresponding non-default path.
Code
# Python – fixed value wired into the client at construction time
self._client = httpx.AsyncClient(timeout=30.0)// TypeScript – module-level constant, not configurable
const DEFAULT_TIMEOUT_MS = 30000;Recommendation
Accept an optional timeout parameter in both constructors (defaulting to the current value) so integrators can tune it:
# Python
def __init__(self, base_url: str, ..., timeout: float = 30.0) -> None:
self._client = httpx.AsyncClient(timeout=timeout)// TypeScript
constructor(baseUrl: string, apiKey?: string, apiSecret?: string, timeoutMs = 30_000) {
this._timeoutMs = timeoutMs;
}
// then: signal: AbortSignal.timeout(this._timeoutMs)[MN-03] AbortSignal.timeout() Produces DOMException — Error Classification May Mislead Callers
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness |
| File | typescript/packages/x402/src/utils/gasfree.ts : Lines 178, 217, 242, 334 |
Description
When the 30-second deadline fires, AbortSignal.timeout() causes fetch to reject with a DOMException whose .name is "TimeoutError", not a plain Error. The waitForSuccess polling loop catches these as generic err values and increments errorCount — which is correct — but the error logged is "GasFree status poll failed … ${err}". Because DOMException.toString() serialises as "TimeoutError: The operation was aborted due to timeout", the message is adequate in logs. However, callers who inspect the thrown Error type after maxErrors consecutive timeouts will receive a plain new Error(...) string-wrapping the DOMException, losing the structured type information. Any code that does err instanceof Error (e.g. error monitoring SDKs) will pass, but err.name won't be "TimeoutError" — it will be the wrapping Error's name.
Code
// waitForSuccess – catch block re-wraps without preserving type info
} catch (err) {
errorCount++;
if (errorCount >= maxErrors) {
throw new Error(
`GasFree status polling aborted after ${errorCount} consecutive errors: ${err}`
);
}Recommendation
Check for timeout errors explicitly so they can produce a more actionable message:
} catch (err) {
const isTimeout =
err instanceof DOMException && err.name === 'TimeoutError';
errorCount++;
console.warn(
`GasFree status poll failed for ${traceId} (error #${errorCount}, timeout=${isTimeout}): ${err}`
);
if (errorCount >= maxErrors) {
throw new Error(
`GasFree status polling aborted after ${errorCount} consecutive errors` +
(isTimeout ? ' (request timeout)' : '') +
`: ${err}`
);
}Suggestions
[S-01] No Test Coverage for Timeout Path
File: python/x402/tests/utils/test_gasfree_utils.py · typescript/packages/x402/src/utils/gasfree.test.ts
Description: The PR's primary functional contribution is adding timeouts, yet neither test suite exercises the timeout path. There are no tests that simulate a hung server (e.g. a mock that never resolves) and assert that the call rejects/raises within the expected window. Without this, a future regression that accidentally removes the signal parameter or changes the timeout constant will go undetected.
Suggestion: Add at least one test per SDK that mocks a request which stalls past the timeout duration and asserts that an appropriate error is thrown. For TypeScript with Vitest, AbortSignal.timeout can be replaced with a controllable AbortController.
[S-02] Python and TypeScript Connection-Pooling Strategies Now Diverge
File: python/x402/src/bankofai/x402/utils/gasfree.py · typescript/packages/x402/src/utils/gasfree.ts
Description: The Python client now explicitly owns and reuses a connection pool (via the persistent httpx.AsyncClient). The TypeScript client still issues bare fetch() calls, relying on the runtime's implicit connection management (undici in Node.js, the browser's fetch implementation elsewhere). This is an architectural asymmetry: Python connection behaviour is now explicitly defined and configurable, TypeScript's is entirely runtime-dependent. There is no GasFreeAPIClient-level connection-pool configuration in TypeScript.
Suggestion: Consider documenting this architectural difference, or (in a follow-up PR) align the TypeScript client by accepting a configurable fetch function or a pre-configured undici.Pool, which also makes unit testing more straightforward (no global vi.stubGlobal('fetch', ...)).
Positive Observations
| Area | Observation |
|---|---|
| TypeScript change quality | The four-line timeout addition is minimal, consistent, and touches every outbound fetch call without missing any. The named DEFAULT_TIMEOUT_MS constant avoids magic numbers and makes future changes easy. |
| Python refactor clarity | Removing the async with ... as client: nesting level reduces indentation by one level across all four methods, improving readability without altering any logic. |
| Timeout value alignment | Both SDKs converge on the same 30-second window, giving consistent behaviour for multi-SDK integrations. |
| Error handling preserved | The Python refactor faithfully preserves all existing error-handling paths (HTTPStatusError checks, business-error checks, null-data guards) without regression. |
| Test suite breadth (TS) | The TypeScript gasfree.test.ts has thorough coverage of waitForSuccess edge cases — null data, transient errors, error-count reset — all of which pass through the new timeout-equipped fetch. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 8 | 7 | 1 | 0 | AsyncClient lifecycle (MJ-01); DOMException type (MN-03) |
| Security | 5 | 5 | 0 | 3 | No auth/input changes in this diff |
| Performance | 4 | 4 | 0 | 3 | Connection reuse is a positive improvement |
| Code Quality | 6 | 5 | 1 | 0 | Timeout not configurable (MN-02) |
| Testing | 5 | 3 | 2 | 0 | No timeout tests; unclosed clients in test teardown |
| Documentation | 3 | 3 | 0 | 3 | N/A — no new public API surface |
| Compatibility | 3 | 3 | 0 | 0 | AbortSignal.timeout() is sufficiently modern |
| Observability | 3 | 3 | 0 | 0 | Logging preserved; timeout errors surfaced |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between the specified branches (main...fix/gasfree-client-reliability). Runtime behavior, integration testing, and deployment impact are not covered.
Report generated by Code Review Skill v1.0.0
Date: 2026-03-30
1 participant
Summary
httpx.AsyncClientinstance with connection pooling in PythonGasFreeAPIClient, instead of creating a new TCP connection per request (especially impactful duringwait_for_successpolling)AbortSignal.timeout) and Python (httpx.AsyncClient(timeout=30.0)) to prevent hanging on unresponsive proxy🤖 Generated with Claude Code