Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show a warning on a cache access when a CachedSession context manager has been exited #290

Merged

Conversation

alessio-locatelli
Copy link
Collaborator

@alessio-locatelli alessio-locatelli commented Nov 7, 2024

Closes #241

MRE

import asyncio
from aiohttp_client_cache import CachedSession, SQLiteBackend

cache = SQLiteBackend(cache_name="./test_cache.sqlite")

async def task(n: int):
    url = f"https://httpbin.org/ip?count={n}"
    async with CachedSession(cache=cache) as session:
        return await session.get(url, ssl=False)
    
async def tasks(arr: list):
    tasks = [task(i) for i in arr]
    return await asyncio.gather(*tasks)


asyncio.run(tasks(list(range(10))))

1st run (there is no cache file yet)

We can see the warning inside get_connection() after the yield:

python .gather_sessions.py
/usr/lib64/python3.13/contextlib.py:221: UserWarning: Cache access after closing the `Cachedsession` context manager is discouraged and can be forbidden in the future to prevent errors related to a closed database connection.
  await anext(self.gen)
Traceback (most recent call last):
[...]
sqlite3.ProgrammingError: Cannot operate on a closed database.

2nd run (a cache file already exists)

We can see the warning inside the def read():

/usr/lib64/python3.13/contextlib.py:221: UserWarning: Cache access after closing the `Cachedsession` context manager is discouraged and can be forbidden in the future to prevent errors related to a closed database connection.
  await anext(self.gen)
Traceback (most recent call last):
[...]
sqlite3.ProgrammingError: Cannot operate on a closed database.

Notes

Not related to this PR and the mentioned issue, but Redis and MongoDB must have the def close() method too because the connections must be closed.

@alessio-locatelli alessio-locatelli changed the title Show a warning on a cache access when a CachedSession context manager have been exited Show a warning on a cache access when a CachedSession context manager has been exited Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.17%. Comparing base (26bf273) to head (fb00280).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
aiohttp_client_cache/backends/sqlite.py 50.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
- Coverage   96.51%   96.17%   -0.35%     
==========================================
  Files          10       10              
  Lines        1063     1072       +9     
  Branches      186      188       +2     
==========================================
+ Hits         1026     1031       +5     
- Misses         28       30       +2     
- Partials        9       11       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alessio-locatelli alessio-locatelli force-pushed the forbid_closed_cache_reuse branch from 96e4436 to fb00280 Compare November 7, 2024 13:42
@alessio-locatelli alessio-locatelli marked this pull request as ready for review November 7, 2024 13:43
@alessio-locatelli alessio-locatelli force-pushed the forbid_closed_cache_reuse branch from fb00280 to 8dd292e Compare November 7, 2024 14:16
@alessio-locatelli alessio-locatelli self-assigned this Nov 7, 2024
Copy link
Member

@JWCook JWCook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a nice improvement to me. Thanks!

My only thought for further improvement was whether we could use an existing method/attribute on the connection object to determine if it's closed instead of adding our own _closed flag. But it's an internal variable (aiosqlite.Connection._connection), so it's probably best to not rely on that.

@JWCook JWCook merged commit b9a4b2a into requests-cache:main Nov 7, 2024
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.

ProgrammingError: Cannot operate on a closed database.
2 participants