From 8ef7b800f5b2905917732a114fb9c4b25c93bc42 Mon Sep 17 00:00:00 2001 From: Jordan Cook Date: Tue, 3 Oct 2023 14:50:34 -0500 Subject: [PATCH 1/3] Add SQLiteCache.__del__() method to force connection close if used without a contextmanager --- aiohttp_client_cache/backends/sqlite.py | 14 ++++++++++++++ test/integration/base_backend_test.py | 20 ++++++++++++++++++-- test/integration/test_memory.py | 8 ++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/aiohttp_client_cache/backends/sqlite.py b/aiohttp_client_cache/backends/sqlite.py index 95f696e..ff4ee3b 100644 --- a/aiohttp_client_cache/backends/sqlite.py +++ b/aiohttp_client_cache/backends/sqlite.py @@ -103,6 +103,20 @@ async def _init_db(self): ) return self._connection + def __del__(self): + """If the aiosqlite connection is still open when this object is deleted, force its thread + to close by emptying its internal queue and setting its ``_running`` flag to ``False``. + This is basically a last resort to avoid hanging the application if this backend is used + without the CachedSession contextmanager. + + Note: Since this uses internal attributes, it has the potential to break in future versions + of aiosqlite. + """ + if self._connection is not None: + self._connection._tx.queue.clear() + self._connection._running = False + self._connection = None + @asynccontextmanager async def bulk_commit(self): """Contextmanager to more efficiently write a large number of records at once diff --git a/test/integration/base_backend_test.py b/test/integration/base_backend_test.py index 7e0fd36..1b09dae 100644 --- a/test/integration/base_backend_test.py +++ b/test/integration/base_backend_test.py @@ -35,13 +35,17 @@ class BaseBackendTest: @asynccontextmanager async def init_session(self, clear=True, **kwargs) -> AsyncIterator[CachedSession]: + session = await self._init_session(clear=clear, **kwargs) + async with session: + yield session + + async def _init_session(self, clear=True, **kwargs) -> CachedSession: kwargs.setdefault('allowed_methods', ALL_METHODS) cache = self.backend_class(CACHE_NAME, **self.init_kwargs, **kwargs) if clear: await cache.clear() - async with CachedSession(cache=cache, **self.init_kwargs, **kwargs) as session: - yield session + return CachedSession(cache=cache, **self.init_kwargs, **kwargs) @pytest.mark.parametrize('method', HTTPBIN_METHODS) @pytest.mark.parametrize('field', ['params', 'data', 'json']) @@ -100,6 +104,18 @@ async def get_url(mysession, url): responses = await asyncio.gather(*tasks) assert all([r.from_cache is True for r in responses]) + async def test_without_contextmanager(self): + """Test that the cache backend can be safely used without the CachedSession contextmanager. + An "unclosed ClientSession" warning is expected here, however. + """ + session = await self._init_session() + await session.get(httpbin('get')) + del session + + session = await self._init_session(clear=False) + r = await session.get(httpbin('get')) + assert r.from_cache is True + async def test_request__expire_after(self): async with self.init_session() as session: await session.get(httpbin('get'), expire_after=1) diff --git a/test/integration/test_memory.py b/test/integration/test_memory.py index 3b89def..c45d843 100644 --- a/test/integration/test_memory.py +++ b/test/integration/test_memory.py @@ -23,6 +23,14 @@ async def test_content_reset(self): content_2 = await cached_response_2.read() assert content_1 == content_2 == original_content + async def test_without_contextmanager(self): + """Test that the cache backend can be safely used without the CachedSession contextmanager. + An "unclosed ClientSession" warning is expected here, however. + """ + session = await self._init_session() + await session.get(httpbin('get')) + del session + # Serialization tests don't apply to in-memory cache async def test_serializer__pickle(self): pass From 0e535c5659fc8d47e0b53c507b6e0dabc770f949 Mon Sep 17 00:00:00 2001 From: Jordan Cook Date: Tue, 3 Oct 2023 16:10:24 -0500 Subject: [PATCH 2/3] Add timeout to avoid hanging if the test fails --- noxfile.py | 2 +- poetry.lock | 11 +++++++---- pyproject.toml | 1 + test/integration/base_backend_test.py | 15 +++++++++------ test/unit/test_base_backend.py | 21 ++++++++------------- 5 files changed, 26 insertions(+), 24 deletions(-) diff --git a/noxfile.py b/noxfile.py index f648154..395a6f6 100644 --- a/noxfile.py +++ b/noxfile.py @@ -32,7 +32,7 @@ def test(session): """Run tests for a specific python version""" test_paths = session.posargs or [UNIT_TESTS] - session.install('.', 'pytest', 'pytest-xdist', 'requests-mock', 'timeout-decorator') + session.install('.', 'pytest', 'pytest-aiohttp', 'pytest-asyncio', 'pytest-xdist') cmd = f'pytest -rs {XDIST_ARGS}' session.run(*cmd.split(' '), *test_paths) diff --git a/poetry.lock b/poetry.lock index c905421..63d7f35 100644 --- a/poetry.lock +++ b/poetry.lock @@ -283,18 +283,21 @@ tests-no-zope = ["cloudpickle", "hypothesis", "mypy (>=1.1.1)", "pympler", "pyte [[package]] name = "babel" -version = "2.12.1" +version = "2.13.0" description = "Internationalization utilities" optional = false python-versions = ">=3.7" files = [ - {file = "Babel-2.12.1-py3-none-any.whl", hash = "sha256:b4246fb7677d3b98f501a39d43396d3cafdc8eadb045f4a31be01863f655c610"}, - {file = "Babel-2.12.1.tar.gz", hash = "sha256:cc2d99999cd01d44420ae725a21c9e3711b3aadc7976d6147f622d8581963455"}, + {file = "Babel-2.13.0-py3-none-any.whl", hash = "sha256:fbfcae1575ff78e26c7449136f1abbefc3c13ce542eeb13d43d50d8b047216ec"}, + {file = "Babel-2.13.0.tar.gz", hash = "sha256:04c3e2d28d2b7681644508f836be388ae49e0cfe91465095340395b60d00f210"}, ] [package.dependencies] pytz = {version = ">=2015.7", markers = "python_version < \"3.9\""} +[package.extras] +dev = ["freezegun (>=1.0,<2.0)", "pytest (>=6.0)", "pytest-cov"] + [[package]] name = "beautifulsoup4" version = "4.12.2" @@ -2428,4 +2431,4 @@ sqlite = ["aiosqlite"] [metadata] lock-version = "2.0" python-versions = "^3.7" -content-hash = "1fe630ffa0c485b5f63f406aa40fba8355ed503147802b2c59546af2027b7146" +content-hash = "578e8d2a0e539e0fd7a4acbad630db9a8444048460aa61eac4b25df8c58ca685" diff --git a/pyproject.toml b/pyproject.toml index 2ec6796..3354087 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -70,6 +70,7 @@ docs = ["furo", "linkify-it-py", "markdown-it-py", "myst-parser", "python [tool.poetry.dev-dependencies] # For unit + integration tests +async-timeout = ">=4.0" brotli = ">=1.0" pytest = ">=6.2" pytest-aiohttp = "^0.3" diff --git a/test/integration/base_backend_test.py b/test/integration/base_backend_test.py index 1b09dae..b7a4df7 100644 --- a/test/integration/base_backend_test.py +++ b/test/integration/base_backend_test.py @@ -8,6 +8,7 @@ from uuid import uuid4 import pytest +from async_timeout import timeout from itsdangerous.exc import BadSignature from itsdangerous.serializer import Serializer @@ -108,13 +109,15 @@ async def test_without_contextmanager(self): """Test that the cache backend can be safely used without the CachedSession contextmanager. An "unclosed ClientSession" warning is expected here, however. """ - session = await self._init_session() - await session.get(httpbin('get')) - del session + # Timeout to avoid hanging if the test fails + async with timeout(5.0): + session = await self._init_session() + await session.get(httpbin('get')) + del session - session = await self._init_session(clear=False) - r = await session.get(httpbin('get')) - assert r.from_cache is True + session = await self._init_session(clear=False) + r = await session.get(httpbin('get')) + assert r.from_cache is True async def test_request__expire_after(self): async with self.init_session() as session: diff --git a/test/unit/test_base_backend.py b/test/unit/test_base_backend.py index a364da8..7dd2b2b 100644 --- a/test/unit/test_base_backend.py +++ b/test/unit/test_base_backend.py @@ -1,19 +1,14 @@ import pickle -from sys import version_info from unittest.mock import MagicMock, patch import pytest from aiohttp_client_cache import CachedResponse from aiohttp_client_cache.backends import CacheBackend, DictCache, get_placeholder_backend +from test.conftest import skip_37 TEST_URL = 'https://test.com' -pytestmark = pytest.mark.asyncio -skip_py37 = pytest.mark.skipif( - version_info < (3, 8), reason='Test requires AsyncMock from python 3.8+' -) - def get_mock_response(**kwargs): response_kwargs = { @@ -71,7 +66,7 @@ async def test_get_response__cache_miss(mock_delete): mock_delete.assert_not_called() -@skip_py37 +@skip_37 @patch.object(CacheBackend, 'delete') @patch.object(CacheBackend, 'is_cacheable', return_value=False) async def test_get_response__cache_expired(mock_is_cacheable, mock_delete): @@ -84,7 +79,7 @@ async def test_get_response__cache_expired(mock_is_cacheable, mock_delete): mock_delete.assert_called_with('request-key') -@skip_py37 +@skip_37 @pytest.mark.parametrize('error_type', [AttributeError, KeyError, TypeError, pickle.PickleError]) @patch.object(CacheBackend, 'delete') @patch.object(DictCache, 'read') @@ -99,7 +94,7 @@ async def test_get_response__cache_invalid(mock_read, mock_delete, error_type): mock_delete.assert_not_called() -@skip_py37 +@skip_37 @patch.object(DictCache, 'read', return_value=object()) async def test_get_response__quiet_serde_error(mock_read): """Test for a quiet deserialization error in which no errors are raised but attributes are @@ -113,7 +108,7 @@ async def test_get_response__quiet_serde_error(mock_read): assert response is None -@skip_py37 +@skip_37 async def test_save_response(): cache = CacheBackend() mock_response = get_mock_response() @@ -126,7 +121,7 @@ async def test_save_response(): assert await cache.redirects.read(redirect_key) == 'key' -@skip_py37 +@skip_37 async def test_save_response__manual_save(): """Manually save a response with no cache key provided""" cache = CacheBackend() @@ -193,7 +188,7 @@ async def test_has_url(): assert not await cache.has_url('https://test.com/some_other_path') -@skip_py37 +@skip_37 @patch('aiohttp_client_cache.backends.base.create_key') async def test_create_key(mock_create_key): """Actual logic is in cache_keys module; just test to make sure it gets called correctly""" @@ -244,7 +239,7 @@ async def test_is_cacheable(method, status, disabled, expired, filter_return, ex assert await cache.is_cacheable(mock_response) is expected_result -@skip_py37 +@skip_37 @pytest.mark.parametrize( 'method, status, disabled, expired, body, expected_result', [ From 52f344213bc870350150615c38fb3e355d123334 Mon Sep 17 00:00:00 2001 From: Austin Raney Date: Thu, 12 Oct 2023 13:07:26 -0500 Subject: [PATCH 3/3] Fix possible RuntimeWarning due to capturing async contextmanager in test_concurrent_bulk_commit() --- test/integration/test_sqlite.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/integration/test_sqlite.py b/test/integration/test_sqlite.py index 4badcb1..d86315d 100644 --- a/test/integration/test_sqlite.py +++ b/test/integration/test_sqlite.py @@ -1,5 +1,6 @@ import asyncio import os +from contextlib import asynccontextmanager from tempfile import gettempdir from unittest.mock import MagicMock, patch @@ -53,17 +54,22 @@ async def test_concurrent_bulk_commit(self, mock_sqlite): mock_connection = AsyncMock() mock_sqlite.connect = AsyncMock(return_value=mock_connection) - async with self.init_cache() as cache: + @asynccontextmanager + async def bulk_commit_ctx(): + 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}') - 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}') + yield bulk_commit_items - assert mock_connection.commit.call_count == 1 - tasks = [asyncio.create_task(bulk_commit_items(n)) for n in [10, 100, 1000, 10000]] - await asyncio.gather(*tasks) - assert mock_connection.commit.call_count == 5 + async with bulk_commit_ctx() as bulk_commit_items: + assert mock_connection.commit.call_count == 1 + tasks = [asyncio.create_task(bulk_commit_items(n)) for n in [10, 100, 1000, 10000]] + await asyncio.gather(*tasks) + assert mock_connection.commit.call_count == 5 async def test_fast_save(self): async with self.init_cache(index=1, fast_save=True) as cache_1, self.init_cache(