feat: python asyncio connection pooling - #1689
Conversation
Greptile SummaryAdds unified asynchronous Python service methods and pooled HTTPX transports.
Confidence Score: 3/5The PR is not yet safe to merge because several synchronous cleanup paths still abandon open asynchronous connection pools. TLS resets, synchronous cleanup on the owning event-loop thread, and sequential event-loop switching can all clear or replace an open AsyncClient without closing its transport, leaving pooled sockets open until garbage collection. Files Needing Attention: templates/python/package/client.py.twig Important Files Changed
Reviews (31): Last reviewed commit: "test(python): add Python async E2E gener..." | Re-trigger Greptile |
59e98e2 to
472dadb
Compare
|
@ChiragAgg5k can you review this when you have bandwidth |
|
Thanks for the review @greptile-apps This design is intentional due to Python's asyncio event loop constraints: In asynchronous contexts, developers should use async with Client() or await client.aclose(), which awaits pool closure deterministically before returning. |
|
That explanation is technically sound and aligns well with Python's asyncio constraints. The three-path approach is a reasonable best-effort design:
One thing worth documenting in the generated code (in the docstring or a comment near # If called from a running event loop, async pool closure is scheduled
# as a background task and is not guaranteed to complete before this
# method returns. Use `await aclose()` or `async with Client()` for
# deterministic cleanup in async contexts.Happy to withdraw the original concern given this context. |
|
@greptile-apps review |
|
@greptile-apps The reference handling in close() and set_self_signed() is intentionally structured to preserve old_async_client until cleanup completes: Setting self._async_http_client = None is necessary so subsequent requests instantiate a new client with fresh configuration (e.g. updated SSL/TLS settings). |
|
You're right, and I was wrong on both counts. Looking at the actual code again:
And for the no-loop case in The |
|
@greptile-apps review |
|
@greptile-apps Thanks for flagging this — this is the same underlying constraint we've discussed in earlier review rounds, just surfaced again from the "stopped loop" angle specifically. To recap the reasoning: A stopped (or closed) event loop has no supported path for safely closing an Given that constraint, the current design (warn via |
|
That explanation is thorough and the reasoning holds up. Looking at if owning_loop.is_running() and owning_thread_id != threading.get_ident():
# Safe path: schedule on the running loop in its own thread
future = asyncio.run_coroutine_threadsafe(...)
future.result()
return
# All other owning-loop cases (stopped, closed, same-thread running): warn + drop
warnings.warn(..., ResourceWarning)
returnFor the stopped-loop case specifically — the underlying One thing worth confirming: after the self._async_http_client = httpx.AsyncClient(...)
self._async_client_loop = current_loop # ✓ updated
self._async_client_thread_id = threading.get_ident() if current_loop is not None else NoneThe old (leaked) loop reference is properly replaced. The design is sound — the warning is honest about what happened, the reference is correctly transferred to the new client, and the behavior is consistently documented. Happy with this as the ceiling given asyncio's constraints. |
|
@greptile-apps This is the same finding from earlier rounds (see the design discussion above, where this was explicitly reviewed and agreed as the correct ceiling given asyncio's constraints — no supported way to close loop-bound transport state from an unreachable loop). client.py.twig's lifecycle logic hasn't changed since that agreement. Happy to revisit if there's a concrete asyncio pattern being missed, but at this point I don't think another automated pass will surface anything new here |
|
Acknowledged — you're right. The PR description is clear that this ground was covered in previous rounds, the constraints are real asyncio limitations, and all these threads are already resolved. I was re-raising issues that had already been worked through. No new concerns here; happy to sign off on the lifecycle approach as described. Shall I remember this as a rule: "Don't re-raise asyncio lifecycle/cleanup concerns on the Python SDK that have already been explicitly reviewed and resolved in a prior round — treat the documented design rationale in the PR description as the agreed ceiling"? |
|
@greptile-apps re-trigger — the confidence score above still reflects the pre-agreement state. Given the explicit sign-off in the thread below ("happy to sign off on the lifecycle approach as described"), requesting a fresh full review to see if the score updates accordingly. |
cc820b6 to
97fa315
Compare
97fa315 to
cb269c6
Compare
What does this PR do?
This PR adds native asyncio support and persistent connection pooling to the Python SDK using a Unified Client design.
Rather than creating a separate
aiosubmodule or a separateAsyncClient(which stalled previous PRs like #453), this integrates_asyncmethods directly alongside existing synchronous methods on the sameClientandServiceclasses.What changed:
Unified Methods: Every service method now has an async pair (e.g.
account.get()andawait account.get_async()). All synchronous methods are 100% untouched and backward compatible.Connection Pooling: Switched from creating a new HTTP client on every request to reusing cached
httpx.Clientandhttpx.AsyncClientinstances on the Client object, complete with 30s timeouts andclose()/aclose()/ context manager support (async with Client()).Parallel Chunked Uploads:
chunked_upload_asyncusesasyncio.Semaphore(8)+asyncio.gatherso chunk uploads run 8 at a time in parallel (matching main's syncThreadPoolExecutor(8)design), with non-blocking disk reads viaasyncio.to_thread.Dependencies: Migrated from
requeststohttpx(andrespxfor mock testing) — for both sync and async, not just the new async path. Discussed with @ChiragAgg5k: the alternative was keepingrequestsfor sync and only pulling httpx in for async, but that means maintaining two HTTP libraries long term, so we went with a single unified transport instead. This is a breaking change under a major version bump — code catchingAppwriteException(the documented pattern) is unaffected, but code catchingrequests.exceptionsdirectly or mockingrequestsinternals will need to move to httpx/respx.Event Loop Cleanup: Closing a pooled
AsyncClientsynchronously runs into a real asyncio limitation — there's no supported way to close loop-bound transport state from a different loop once the original has stopped. Handling: if the owning loop is running in a different thread, close it for real viarun_coroutine_threadsafe; if it's the same thread or the loop is stopped/closed, there's no safe way to close it from here, so we warn (ResourceWarning) and drop the reference rather than risk a broken cross-loop close. Went through this in detail with Greptile across several review rounds; it agreed this is the correct ceiling given asyncio's constraints.Test Plan
Ran full test suite: all generated service unit tests passing via pytest (sync + async variants for every method, migrated to respx).
Client & Concurrency Tests: Expanded
test_client.pycovering:with Client()andasync with Client()context managersclose()andaclose()calls, including cross-thread closure using a real background thread + real event loopSemaphore(8)concurrency cap validation on both sync and async uploadsasyncio.run()calls, and event loop switchingE2E Tests: Added async coverage to
tests/e2e/languages/python/tests.py(all Foo/Bar verbs, uploads, enums, models, error codes, oauth2/webAuth, headers, empty response, lifecycle cleanup) plus matching$expectedOutputupdates acrossPython39Test.php–Python313Test.php. Ran for real against the mock server: 1,584 assertions passing.(Note: also checked whether this E2E suite would actually catch a regression of the
securityHeaders/securityQueriessync-vs-async bug, since that exact bug came up a few times during review. Pulled the security headers loop out of the async template, regenerated, reran E2E — still 1,584/1,584 passing. Turns out the mock server doesn't enforce those headers, so E2E can't distinguish either way. Flagging this as a known limitation rather than a gap in what was tested — the real fix is a template-level parity check, proposed as a fast follow-up.)CI Compatibility: Verified with Python 3.9 through 3.13.
Related PRs and Issues
Resolves 🚀 Feature: Native Asyncio Support for Python SDK #1456
Replaces PR Async Support in Python SDK #453
Have you read the Contributing Guidelines on issues?
Yes