From 18fbb72eecf1b294af66915db67ed780dde90186 Mon Sep 17 00:00:00 2001 From: lemon24 Date: Sat, 7 Dec 2024 12:14:04 +0200 Subject: [PATCH] Fix foreign keys being disabled in threads other than the creating one. --- CHANGES.rst | 29 +++++++++++++++++++++ src/reader/_storage/_base.py | 16 +++++------- src/reader/_storage/_sqlite_utils.py | 11 +++++++- tests/test_reader.py | 39 ++++++++++++++++++++++++++++ tests/test_storage.py | 7 +---- 5 files changed, 85 insertions(+), 17 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 08d4461b..2d025fc6 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -20,11 +20,40 @@ Unreleased * Add :meth:`~Reader.copy_entry`. (:issue:`290`) +* Fix bug causing :class:`Reader` operations + from a thread other than the one that created the instance + to happen with foreign key constraint enforcement disabled + (e.g. deleting a feed from another thread would not delete its entries). + + This bug exists since using :class:`Reader` instances from other threads + became allowed in `2.15 `_. + + Serving the web application with ``python -m reader serve`` + is known to be affected. + Serving it with uWSGI without threads (the default) + should not be affected. + + .. attention:: + + **Your database may be in an inconsistent state because of this bug.** + + It is recommended you run `PRAGMA foreign_key_check`_ on your database. + + If you are upgrading from a version prior to 3.16 + (i.e. were not using a pre-release version of *reader*), + the migration will do so for you. + If there are inconsistencies, you will get this error:: + + StorageError: integrity error: after migrating to version 43: + integrity error: FOREIGN KEY constraint failed + * Fix :meth:`~Reader.enable_search` / :meth:`~Reader.update_search` not working when the search database is missing but change tracking is enabled (e.g. when restoring the main database from backup). (:issue:`362`) +.. _PRAGMA foreign_key_check: https://www.sqlite.org/pragma.html#pragma_foreign_key_check + Version 3.15 ------------ diff --git a/src/reader/_storage/_base.py b/src/reader/_storage/_base.py index 78941cf8..efb25789 100644 --- a/src/reader/_storage/_base.py +++ b/src/reader/_storage/_base.py @@ -57,16 +57,12 @@ def __init__(self, path: str, timeout: float | None = None): if timeout is not None: kwargs['timeout'] = timeout - self.factory = _sqlite_utils.LocalConnectionFactory(path, **kwargs) - db = self.factory() - try: - self.setup_db(db) - except BaseException: - db.close() - raise - - self.path = path - self.timeout = timeout + # at least the "PRAGMA foreign_keys = ON" part of setup_db + # has to run for every connection (in every thread), + # since it's not persisted across connections + self.factory = _sqlite_utils.LocalConnectionFactory( + path, self.setup_db, **kwargs + ) def get_db(self) -> sqlite3.Connection: return self.factory() diff --git a/src/reader/_storage/_sqlite_utils.py b/src/reader/_storage/_sqlite_utils.py index ce8dd11d..12d198a6 100644 --- a/src/reader/_storage/_sqlite_utils.py +++ b/src/reader/_storage/_sqlite_utils.py @@ -426,8 +426,11 @@ class LocalConnectionFactory: INLINE_OPTIMIZE_TIMEOUT = 0.1 - def __init__(self, path: str, **kwargs: Any): + def __init__( + self, path: str, setup_db: _DBFunction = lambda _: None, **kwargs: Any + ): self.path = path + self.setup_db = setup_db self.kwargs = kwargs if kwargs.get('uri'): # pragma: no cover raise NotImplementedError("is_private() does not work for uri=True") @@ -463,6 +466,12 @@ def __call__(self) -> sqlite3.Connection: assert db is not None, "for mypy" self._local.call_count = 0 + try: + self.setup_db(db) + except BaseException: + db.close() + raise + # http://threebean.org/blog/atexit-for-threads/ # works on cpython (finalizer runs in thread), # but not on pypy (finalizer runs in main thread); diff --git a/tests/test_reader.py b/tests/test_reader.py index 06aeeff2..2fc726b6 100644 --- a/tests/test_reader.py +++ b/tests/test_reader.py @@ -2911,3 +2911,42 @@ def get_tags(entry): reader.copy_entry(src_id, dst_id) dst = reader.get_entry(dst_id) assert dst.source.title == 'user' if not src.source else src.source.title + + +@pytest.mark.parametrize('same_thread', [True, False]) +def test_delete_feed(make_reader, db_path, same_thread): + reader = make_reader(db_path) + reader._parser = parser = Parser() + feed = parser.feed(1) + entry = parser.entry(1, 'feed') + + reader.add_feed(feed) + reader.update_feeds() + reader.add_entry(dict(feed_url='1', id='user')) + reader.copy_entry(entry, ('1', 'copy')) + reader.set_tag(feed, 'tag', 'feed') + reader.set_tag(entry, 'tag', 'entry') + + assert {e.resource_id for e in reader.get_entries()} == { + ('1', 'feed'), + ('1', 'user'), + ('1', 'copy'), + } + assert dict(reader.get_tags(feed)) == {'tag': 'feed'} + assert dict(reader.get_tags(entry)) == {'tag': 'entry'} + + if same_thread: + reader.delete_feed(feed) + else: + # would cause the test to fail if foreign key enforcement + # was not enabled in other threads (like prior to 3.16) + thread = threading.Thread(target=reader.delete_feed, args=(feed,)) + thread.start() + thread.join() + + reader.add_feed(feed) + assert {e.resource_id for e in reader.get_entries()} == set() + assert dict(reader.get_tags(feed)) == {} + + reader.update_feeds() + assert dict(reader.get_tags(entry)) == {} diff --git a/tests/test_storage.py b/tests/test_storage.py index 7c4fe951..d278f81c 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -111,11 +111,6 @@ def test_database_error_permissions(db_path): Storage(db_path) -def test_path(db_path): - storage = Storage(db_path) - assert storage.path == db_path - - def test_timeout(db_path, monkeypatch): """Storage.__init__ must pass timeout= to connect.""" @@ -170,7 +165,7 @@ def execute(*args): def init(storage, _, __): - Storage(storage.path, timeout=0) + Storage(storage.factory.path, timeout=0) def add_feed(storage, feed, __):