Skip to content

fix: scope aiohttp session to client lifetime and fix Store.delete query params#84

Open
SiddarthAA wants to merge 1 commit intoJigsawStack:mainfrom
SiddarthAA:fix/session-and-store-delete
Open

fix: scope aiohttp session to client lifetime and fix Store.delete query params#84
SiddarthAA wants to merge 1 commit intoJigsawStack:mainfrom
SiddarthAA:fix/session-and-store-delete

Conversation

@SiddarthAA
Copy link
Copy Markdown

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.ClientSession on 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 second asyncio.run() call, so I went with scoping it to AsyncJigsawStack via an async context manager instead.

Here's what I changed:

  • Added an optional session field (NotRequired) to AsyncRequestConfig.
  • Added _SessionContext to wrap an existing session without closing it on exit.
  • Updated AsyncRequest to read the injected session, falling back to a short-lived per-request session when used standalone — so existing behaviour is unchanged.
  • Added __aenter__ / __aexit__ / aclose() to AsyncJigsawStack.

It now works like this:

async with AsyncJigsawStack(api_key="...") as client:
    result = await client.store.get("my-key")  # session closed here

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.delete issue. params=key was 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 to params={} 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

File Tests
test_session_lifecycle.py 8 — _SessionContext, session injection, AsyncJigsawStack lifecycle
test_store_delete.py 3 — params value, verb, key in URL path

p.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 )

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant