refactor: replace per-call httpx clients with module-level singletons#67
Closed
bolinocroustibat wants to merge 1 commit into
Closed
refactor: replace per-call httpx clients with module-level singletons#67bolinocroustibat wants to merge 1 commit into
bolinocroustibat wants to merge 1 commit into
Conversation
bolinocroustibat
force-pushed
the
refactor/httpx-singleton-clients
branch
from
7ae3b21 to
07d82f5
Compare
bolinocroustibat
added
3 - Low priority
bolinocroustibat
force-pushed
the
refactor/httpx-singleton-clients
branch
from
07d82f5 to
29bbf47
Compare
Each helper module previously created a new httpx.AsyncClient on every function call and closed it immediately after, causing a full TLS handshake on every outbound request. A memray profiling session showed this pattern accounted for 295 MB of cumulative SSL context allocations across a single short LLM session (~40 outbound calls). Each helper now holds one shared AsyncClient created at import time. The optional `session` parameter is kept on all public functions as a test override hook. pytest-asyncio is configured with a session-scoped event loop so module-level clients survive across tests without triggering "event loop is closed" errors on teardown. Made-with: Cursor # Conflicts: # helpers/datagouv_api_client.py # Conflicts: # helpers/matomo.py
bolinocroustibat
force-pushed
the
refactor/httpx-singleton-clients
branch
from
9f1bb5a to
b51a293
Compare
bolinocroustibat
added
2 - Moderate priority
Collaborator
Author
|
Since #86 seems to be solved with fewer profiling/performance calls to Sentry, this PR might not be useful anymore. Let's confirm all is good on production for a few days and than close it without merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #86.
httpx.AsyncClientis costly to spin up and tear down on every request: HTTPS repeats TLS handshakes and SSL context work. Clients are closed infinallytoday, so this is not a leak, but repeating this cycle may still cost CPU and memory under high load.When removing Matomo httpx clients, production then saw fewer OOM-related restarts (see #86).
#88 used one shared client for Matomo to try to solve this issue, reducing the number of short-lived clients.
If that helps reducing memory overhead, then sharing clients for the API helpers could reduce similar overhead the same way. This PR applies the same idea for the remaining API clients.
Profiling
Memray on ~40 outbound calls in one short LLM session showed ~295 MB cumulative SSL-related allocations with the old per-call pattern.
Most of that is transient, so a local profiling didn't prove production wins, but we hope connection pooling lowers that work under real traffic.
Significant Changes
AsyncClientper helper with pooled connectionssessionunchanged for testspytest-asynciosession-scoped event loop so teardown does not hit"event loop is closed".We do not know yet that this alone fixes #86, please treat it as a reasonable follow-up to #88 and validate with prod traffic metrics.