fix: scope aiohttp session to client lifetime and fix Store.delete query params#84
Open
SiddarthAA wants to merge 1 commit intoJigsawStack:mainfrom
Open
fix: scope aiohttp session to client lifetime and fix Store.delete query params#84SiddarthAA wants to merge 1 commit intoJigsawStack:mainfrom
SiddarthAA wants to merge 1 commit intoJigsawStack:mainfrom
Conversation
Claim 1 - session lifetime scoped to AsyncJigsawStack (async_request.py, __init__.py):
- Add optional 'session' field to AsyncRequestConfig.
- _SessionContext wraps a shared session without closing it on exit.
- AsyncRequest reads the injected session; falls back to a per-request
ClientSession when used standalone.
- AsyncJigsawStack gains __aenter__ / __aexit__ / aclose() that open a
single aiohttp.ClientSession and share it across all service calls.
Claim 3 - Store.delete passes raw string as URL query params (store.py):
- params=key sent the file key as a malformed query string. The key is
already in the URL path. Fix is params={}.
(DELETE has no body, so there is nothing to corrupt.)
Tests:
- tests/test_session_lifecycle.py 8 tests (claim 1)
- tests/test_store_delete.py 3 tests (claim 3)
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
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.
Picked up the review feedback and got both issues sorted.
Fix 1 — Scope aiohttp session to client lifetime
You were right about the session handling — it was creating a fresh
aiohttp.ClientSessionon every request inside__get_session(), which is what was causing the "Unclosed client session" warnings and burning connections for no reason. I did think about just making it a module-level singleton but that's actually worse — it locks onto the first event loop and blows up on a secondasyncio.run()call, so I went with scoping it toAsyncJigsawStackvia an async context manager instead.Here's what I changed:
sessionfield (NotRequired) toAsyncRequestConfig._SessionContextto wrap an existing session without closing it on exit.AsyncRequestto read the injected session, falling back to a short-lived per-request session when used standalone — so existing behaviour is unchanged.__aenter__/__aexit__/aclose()toAsyncJigsawStack.It now works like this:
Anyone not using the context manager gets the same behaviour as before.
Fix 2 — Store.delete passing raw string as query params
Also fixed the
Store.deleteissue.params=keywas passing a raw string instead of a dict, which was just appending a malformed query string to the request. Since the key is already in the URL path (/store/file/read/{key}) it wasn't needed there at all — changed it toparams={}and that's done.Just to flag — DELETE requests don't have a body, so this was only affecting the query string, not the request body as originally described.
Tests
test_session_lifecycle.py_SessionContext, session injection,AsyncJigsawStacklifecycletest_store_delete.pyp.s. had alot of fun fixing these issue, thank you for the feedback, please let me know if you run into any problems!
Contact
Thanks for reviewing — happy to adjust anything or split into separate PRs if preferred :D
siddartha_ay@protonmail.com | (everything about me at siddartha-ay.xyz )