From 45ca97150a457f7f710a111c74b8531dae45cf5b Mon Sep 17 00:00:00 2001 From: Jordan Cook Date: Wed, 4 Jan 2023 14:54:30 -0600 Subject: [PATCH 1/2] Close database connections (if applicable) on CachedSession.__aexit__ and close() --- HISTORY.md | 3 +++ aiohttp_client_cache/__init__.py | 2 +- aiohttp_client_cache/backends/base.py | 5 +++++ aiohttp_client_cache/backends/sqlite.py | 3 --- aiohttp_client_cache/session.py | 5 +++++ pyproject.toml | 2 +- 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 5562b331..073c8099 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,8 @@ # History +## 0.8.1 (2023-01-04) +* For SQLite backend, close database connection on `ClientSession` context exit + ## 0.8.0 (2022-12-29) * Lazily initialize and reuse SQLite connection objects * Fix `AttributeError` when using a response cached with an older version of `attrs` diff --git a/aiohttp_client_cache/__init__.py b/aiohttp_client_cache/__init__.py index 62799f7f..bea1c926 100644 --- a/aiohttp_client_cache/__init__.py +++ b/aiohttp_client_cache/__init__.py @@ -1,4 +1,4 @@ -__version__ = '0.8.0' +__version__ = '0.8.1' # flake8: noqa: F401, F403 try: diff --git a/aiohttp_client_cache/backends/base.py b/aiohttp_client_cache/backends/base.py index c4653211..e97ff576 100644 --- a/aiohttp_client_cache/backends/base.py +++ b/aiohttp_client_cache/backends/base.py @@ -244,6 +244,8 @@ async def get_urls(self) -> AsyncIterable[str]: async def close(self): """Close any active connections, if applicable""" + await self.responses.close() + await self.redirects.close() # TODO: Support yarl.URL like aiohttp does? @@ -303,6 +305,9 @@ async def contains(self, key: str) -> bool: async def clear(self): """Delete all items from the cache""" + async def close(self): + """Close any active connections, if applicable""" + @abstractmethod async def delete(self, key: str): """Delete an item from the cache. Does not raise an error if the item is missing.""" diff --git a/aiohttp_client_cache/backends/sqlite.py b/aiohttp_client_cache/backends/sqlite.py index c2151fed..505d540e 100644 --- a/aiohttp_client_cache/backends/sqlite.py +++ b/aiohttp_client_cache/backends/sqlite.py @@ -45,9 +45,6 @@ def __init__( ) self.redirects = SQLiteCache(cache_name, 'redirects', use_temp=use_temp, **kwargs) - async def close(self): - await self.responses.close() - class SQLiteCache(BaseCache): """An async interface for caching objects in a SQLite database. diff --git a/aiohttp_client_cache/session.py b/aiohttp_client_cache/session.py index f69e1c65..18e6b10d 100644 --- a/aiohttp_client_cache/session.py +++ b/aiohttp_client_cache/session.py @@ -62,6 +62,11 @@ async def _request( await self.cache.save_response(new_response, actions.key, actions.expires) return set_response_defaults(new_response) + async def close(self): + """Close both aiohttp connector and any backend connection(s) on contextmanager exit""" + await super().close() + await self.cache.close() + @asynccontextmanager async def disabled(self): """Temporarily disable the cache diff --git a/pyproject.toml b/pyproject.toml index 43acbef6..fd0a4077 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "aiohttp-client-cache" -version = "0.8.0" +version = "0.8.1" description = "Persistent cache for aiohttp requests" authors = ["Jordan Cook"] license = "MIT License" From 9384f0065e97d9e21292d05dffe38763ad08e444 Mon Sep 17 00:00:00 2001 From: Jordan Cook Date: Wed, 4 Jan 2023 17:49:33 -0600 Subject: [PATCH 2/2] Update backend storage tests to explicitly close connections (not automatically done since not used with ClientSession) --- test/integration/base_backend_test.py | 1 - test/integration/base_storage_test.py | 107 +++++++++++++------------- test/integration/test_filesystem.py | 21 +++-- test/integration/test_mongodb.py | 10 +-- test/integration/test_sqlite.py | 76 +++++++++--------- 5 files changed, 111 insertions(+), 104 deletions(-) diff --git a/test/integration/base_backend_test.py b/test/integration/base_backend_test.py index 653ff2b5..23a31221 100644 --- a/test/integration/base_backend_test.py +++ b/test/integration/base_backend_test.py @@ -41,7 +41,6 @@ async def init_session(self, clear=True, **kwargs) -> AsyncIterator[CachedSessio async with CachedSession(cache=cache, **self.init_kwargs, **kwargs) as session: yield session - await session.cache.close() @pytest.mark.parametrize('method', HTTPBIN_METHODS) @pytest.mark.parametrize('field', ['params', 'data', 'json']) diff --git a/test/integration/base_storage_test.py b/test/integration/base_storage_test.py index b17fe798..f12ac985 100644 --- a/test/integration/base_storage_test.py +++ b/test/integration/base_storage_test.py @@ -1,5 +1,6 @@ +from contextlib import asynccontextmanager from datetime import datetime -from typing import Dict, Type +from typing import Any, AsyncIterator, Dict, Type import pytest @@ -17,82 +18,84 @@ class BaseStorageTest: init_kwargs: Dict = {} picklable: bool = False storage_class: Type[BaseCache] = None # type: ignore - test_data = picklable_test_data + test_data: Dict[str, Any] = picklable_test_data - async def init_cache(self, index=0, **kwargs): - self.test_data = picklable_test_data if self.picklable else str_test_data + @asynccontextmanager + async def init_cache(self, index=0, **kwargs) -> AsyncIterator[BaseCache]: + self.test_data = picklable_test_data if self.picklable else str_test_data # type: ignore cache = self.storage_class(CACHE_NAME, f'table_{index}', **self.init_kwargs, **kwargs) await cache.clear() - return cache + yield cache + await cache.close() async def test_write_read(self): - cache = await self.init_cache() - # Test write() and contains() - for k, v in self.test_data.items(): - await cache.write(k, v) - assert await cache.contains(k) is True + async with self.init_cache() as cache: + # Test write() and contains() + for k, v in self.test_data.items(): + await cache.write(k, v) + assert await cache.contains(k) is True - # Test read() - for k, v in self.test_data.items(): - assert await cache.read(k) == v + # Test read() + for k, v in self.test_data.items(): + assert await cache.read(k) == v async def test_missing_key(self): - cache = await self.init_cache() - assert await cache.contains('nonexistent_key') is False - assert await cache.read('nonexistent_key') is None + async with self.init_cache() as cache: + assert await cache.contains('nonexistent_key') is False + assert await cache.read('nonexistent_key') is None async def test_delete(self): - cache = await self.init_cache() - await cache.write('do_not_delete', 'value') - for k, v in self.test_data.items(): - await cache.write(k, v) + async with self.init_cache() as cache: + await cache.write('do_not_delete', 'value') + for k, v in self.test_data.items(): + await cache.write(k, v) - for k in self.test_data.keys(): - await cache.delete(k) - assert await cache.contains(k) is False + for k in self.test_data.keys(): + await cache.delete(k) + assert await cache.contains(k) is False - assert await cache.read('do_not_delete') == 'value' + assert await cache.read('do_not_delete') == 'value' async def test_bulk_delete(self): - cache = await self.init_cache() - await cache.write('do_not_delete', 'value') - for k, v in self.test_data.items(): - await cache.write(k, v) + async with self.init_cache() as cache: + await cache.write('do_not_delete', 'value') + for k, v in self.test_data.items(): + await cache.write(k, v) - await cache.bulk_delete(self.test_data.keys()) + await cache.bulk_delete(self.test_data.keys()) - for k in self.test_data.keys(): - assert await cache.contains(k) is False + for k in self.test_data.keys(): + assert await cache.contains(k) is False async def test_bulk_delete_ignores_nonexistent_keys(self): - cache = await self.init_cache() - await cache.bulk_delete(self.test_data.keys()) + async with self.init_cache() as cache: + await cache.bulk_delete(self.test_data.keys()) async def test_keys_values(self): - cache = await self.init_cache() - assert [k async for k in cache.keys()] == [] - assert [v async for v in cache.values()] == [] + async with self.init_cache() as cache: + assert [k async for k in cache.keys()] == [] + assert [v async for v in cache.values()] == [] - for k, v in self.test_data.items(): - await cache.write(k, v) + for k, v in self.test_data.items(): + await cache.write(k, v) - assert {k async for k in cache.keys()} == set(self.test_data.keys()) - assert {v async for v in cache.values()} == set(self.test_data.values()) + assert {k async for k in cache.keys()} == set(self.test_data.keys()) + assert {v async for v in cache.values()} == set(self.test_data.values()) async def test_size(self): - cache = await self.init_cache() - assert await cache.size() == 0 - for k, v in self.test_data.items(): - await cache.write(k, v) + async with self.init_cache() as cache: + assert await cache.size() == 0 + for k, v in self.test_data.items(): + await cache.write(k, v) assert await cache.size() == len(self.test_data) async def test_clear(self): - cache = await self.init_cache() - for k, v in self.test_data.items(): - await cache.write(k, v) - - await cache.clear() - assert await cache.size() == 0 - assert {k async for k in cache.keys()} == set() - assert {v async for v in cache.values()} == set() + async with self.init_cache() as cache: + for k, v in self.test_data.items(): + await cache.write(k, v) + + await cache.clear() + assert await cache.size() == 0 + assert {k async for k in cache.keys()} == set() + assert {v async for v in cache.values()} == set() diff --git a/test/integration/test_filesystem.py b/test/integration/test_filesystem.py index bc054f4b..3245059f 100644 --- a/test/integration/test_filesystem.py +++ b/test/integration/test_filesystem.py @@ -1,9 +1,12 @@ +from contextlib import asynccontextmanager from os.path import isfile from shutil import rmtree from tempfile import gettempdir +from typing import AsyncIterator import pytest +from aiohttp_client_cache.backends.base import BaseCache from aiohttp_client_cache.backends.filesystem import FileBackend, FileCache from test.conftest import CACHE_NAME from test.integration import BaseBackendTest, BaseStorageTest @@ -13,10 +16,12 @@ class TestFileCache(BaseStorageTest): storage_class = FileCache picklable = True - async def init_cache(self, index=0, **kwargs): + @asynccontextmanager + async def init_cache(self, index=0, **kwargs) -> AsyncIterator[BaseCache]: cache = self.storage_class(f'{CACHE_NAME}_{index}', use_temp=True, **kwargs) await cache.clear() - return cache + yield cache + await cache.close() @classmethod def teardown_class(cls): @@ -29,13 +34,13 @@ async def test_use_temp(self): assert temp_path.startswith(gettempdir()) async def test_paths(self): - cache = await self.init_cache() - for i in range(10): - await cache.write(f'key_{i}', f'value_{i}') + async with self.init_cache() as cache: + for i in range(10): + await cache.write(f'key_{i}', f'value_{i}') - assert len([p async for p in cache.paths()]) == 10 - async for path in cache.paths(): - assert isfile(path) + assert len([p async for p in cache.paths()]) == 10 + async for path in cache.paths(): + assert isfile(path) # TODO async def test_write_error(self): diff --git a/test/integration/test_mongodb.py b/test/integration/test_mongodb.py index a3fa2026..20ea1efe 100644 --- a/test/integration/test_mongodb.py +++ b/test/integration/test_mongodb.py @@ -28,11 +28,11 @@ class TestMongoDBCache(BaseStorageTest): async def test_values_many(self): # If some entries are missing the "data" field for some reason, they # should not be returned with the results. - cache = await self.init_cache() - await cache.collection.insert_many({"data": f'value_{i}'} for i in range(10)) - await cache.collection.insert_many({"not_data": f'value_{i}'} for i in range(10)) - actual_results = [v async for v in cache.values()] - assert actual_results == [f'value_{i}' for i in range(10)] + async with self.init_cache() as cache: + await cache.collection.insert_many({"data": f'value_{i}'} for i in range(10)) + await cache.collection.insert_many({"not_data": f'value_{i}'} for i in range(10)) + actual_results = [v async for v in cache.values()] + assert actual_results == [f'value_{i}' for i in range(10)] class TestMongoDBPickleCache(TestMongoDBCache): diff --git a/test/integration/test_sqlite.py b/test/integration/test_sqlite.py index 6890c3d6..8e6207ba 100644 --- a/test/integration/test_sqlite.py +++ b/test/integration/test_sqlite.py @@ -30,19 +30,19 @@ def test_use_temp(self): assert temp_path.startswith(gettempdir()) async def test_bulk_commit(self): - cache = await self.init_cache() - async with cache.bulk_commit(): - pass + async with self.init_cache() as cache: + async with cache.bulk_commit(): + pass - n_items = 1000 - async with cache.bulk_commit(): - for i in range(n_items): - await cache.write(f'key_{i}', f'value_{i}') + n_items = 1000 + async with cache.bulk_commit(): + for i in range(n_items): + await cache.write(f'key_{i}', f'value_{i}') - keys = {k async for k in cache.keys()} - values = {v async for v in cache.values()} - assert keys == {f'key_{i}' for i in range(n_items)} - assert values == {f'value_{i}' for i in range(n_items)} + keys = {k async for k in cache.keys()} + values = {v async for v in cache.values()} + assert keys == {f'key_{i}' for i in range(n_items)} + assert values == {f'value_{i}' for i in range(n_items)} @skip_37 @patch('aiohttp_client_cache.backends.sqlite.aiosqlite') @@ -52,12 +52,13 @@ async def test_concurrent_bulk_commit(self, mock_sqlite): mock_connection = AsyncMock() mock_sqlite.connect = AsyncMock(return_value=mock_connection) - cache = await self.init_cache() - async def bulk_commit_items(n_items): - async with cache.bulk_commit(): - for i in range(n_items): - await cache.write(f'key_{n_items}_{i}', f'value_{i}') + async with self.init_cache() as cache: + + async def bulk_commit_items(n_items): + async with cache.bulk_commit(): + for i in range(n_items): + await cache.write(f'key_{n_items}_{i}', f'value_{i}') assert mock_connection.commit.call_count == 1 tasks = [asyncio.create_task(bulk_commit_items(n)) for n in [10, 100, 1000, 10000]] @@ -65,20 +66,19 @@ async def bulk_commit_items(n_items): assert mock_connection.commit.call_count == 5 async def test_fast_save(self): - cache_1 = await self.init_cache(index=1, fast_save=True) - cache_2 = await self.init_cache(index=2, fast_save=True) - - n = 1000 - for i in range(n): - await cache_1.write(i, i) - await cache_2.write(i, i) - - keys_1 = {k async for k in cache_1.keys()} - keys_2 = {k async for k in cache_2.keys()} - values_1 = {v async for v in cache_1.values()} - values_2 = {v async for v in cache_2.values()} - assert keys_1 == keys_2 == set(range(n)) - assert values_1 == values_2 == set(range(n)) + async with self.init_cache(index=1, fast_save=True) as cache_1, self.init_cache( + index=2, fast_save=True + ) as cache_2: + for i in range(1000): + await cache_1.write(i, i) + await cache_2.write(i, i) + + keys_1 = {k async for k in cache_1.keys()} + keys_2 = {k async for k in cache_2.keys()} + values_1 = {v async for v in cache_1.values()} + values_2 = {v async for v in cache_2.values()} + assert keys_1 == keys_2 == set(range(1000)) + assert values_1 == values_2 == set(range(1000)) @skip_37 @patch('aiohttp_client_cache.backends.sqlite.aiosqlite') @@ -87,16 +87,16 @@ async def test_connection_kwargs(self, mock_sqlite): from unittest.mock import AsyncMock mock_sqlite.connect = AsyncMock() - cache = await self.init_cache(timeout=0.5, invalid_kwarg='???') - mock_sqlite.connect.assert_called_with(cache.filename, timeout=0.5) + async with self.init_cache(timeout=0.5, invalid_kwarg='???') as cache: + mock_sqlite.connect.assert_called_with(cache.filename, timeout=0.5) async def test_close(self): - cache = await self.init_cache() - async with cache.get_connection(): - pass - await cache.close() - await cache.close() # Closing again should be a no-op - assert cache._connection is None + async with self.init_cache() as cache: + async with cache.get_connection(): + pass + await cache.close() + await cache.close() # Closing again should be a no-op + assert cache._connection is None # TODO: Tests for unimplemented features # async def test_chunked_bulk_delete(self):