From 1f973601d6b823d71defb08cf0ff843ac84596aa Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 10:52:43 +0200 Subject: [PATCH 1/7] Set all files to LF --- .gitattributes | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.gitattributes b/.gitattributes index 4271c7b2..d01653a8 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,2 +1 @@ -Pipfile text eol=lf -Pipfile.lock text eol=lf \ No newline at end of file +text eol=lf From 8beee48df9ad0863d07432e6a0b14ecf8cd23fbf Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 10:53:27 +0200 Subject: [PATCH 2/7] Fix progress dialog not closing Occurred when an error was encountered before it was shown. --- src/usdb_syncer/gui/progress.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/usdb_syncer/gui/progress.py b/src/usdb_syncer/gui/progress.py index 51b22981..449ff6d2 100644 --- a/src/usdb_syncer/gui/progress.py +++ b/src/usdb_syncer/gui/progress.py @@ -81,7 +81,7 @@ def wrapped_task() -> None: def wrapped_on_done() -> None: assert result - dialog.close() + dialog.deleteLater() on_done(result) signal.result.connect(wrapped_on_done) From 7cb32bd727b0935525c7e2a4d73e5069e647f127 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 16:49:46 +0200 Subject: [PATCH 3/7] Attempt to fix sqlite3.InterfaceError Presumably, #277 is caused by a race condition due to concurrent reads during a write. This commit introduces thread-local connections, so all operations on a given connection are serialized. Locking is not required anymore. Threads are responsible for opening and closing their own connection. Also use WAL journal mode which should only have upsides. --- src/usdb_syncer/db/__init__.py | 64 +++++++++++-------- .../db/sql/setup_session_script.sql | 4 +- src/usdb_syncer/gui/__init__.py | 2 +- src/usdb_syncer/gui/progress.py | 5 +- src/usdb_syncer/song_loader.py | 53 +++++++-------- src/usdb_syncer/usdb_song.py | 6 +- tests/unit/test_db.py | 36 +++++------ 7 files changed, 94 insertions(+), 76 deletions(-) diff --git a/src/usdb_syncer/db/__init__.py b/src/usdb_syncer/db/__init__.py index b32ea803..c7cbf5f2 100644 --- a/src/usdb_syncer/db/__init__.py +++ b/src/usdb_syncer/db/__init__.py @@ -4,8 +4,8 @@ import contextlib import enum -import itertools import json +import os import sqlite3 import threading import time @@ -40,45 +40,50 @@ def get(cls, name: str, cache: bool = True) -> str: return stmt +class _LocalConnection(threading.local): + """A thread-local database connection.""" + + connection: sqlite3.Connection | None = None + + class _DbState: """Singleton for managing the global database connection.""" - lock = threading.Lock() - _connection: sqlite3.Connection | None = None + _local: _LocalConnection = _LocalConnection() @classmethod def connect(cls, db_path: Path | str, trace: bool = False) -> None: - with cls.lock: - cls._connection = sqlite3.connect( - db_path, check_same_thread=False, isolation_level=None - ) - if trace: - cls._connection.set_trace_callback(_logger.debug) - _validate_schema(cls._connection) + if cls._local.connection: + raise errors.DatabaseError("Already connected to database!") + cls._local.connection = sqlite3.connect( + db_path, check_same_thread=False, isolation_level=None + ) + if trace: + cls._local.connection.set_trace_callback(_logger.debug) + _validate_schema(cls._local.connection) @classmethod def connection(cls) -> sqlite3.Connection: - if cls._connection is None: + if cls._local.connection is None: raise errors.DatabaseError("Not connected to database!") - return cls._connection + return cls._local.connection @classmethod def close(cls) -> None: - if _DbState._connection is not None: - _DbState._connection.close() - _DbState._connection = None + if _DbState._local.connection is not None: + _DbState._local.connection.close() + _DbState._local.connection = None @contextlib.contextmanager def transaction() -> Generator[None, None, None]: - with _DbState.lock: - try: - _DbState.connection().execute("BEGIN") - yield None - except Exception: # pylint: disable=broad-except - _DbState.connection().rollback() - raise - _DbState.connection().commit() + try: + _DbState.connection().execute("BEGIN IMMEDIATE") + yield None + except Exception: # pylint: disable=broad-except + _DbState.connection().rollback() + raise + _DbState.connection().commit() def _validate_schema(connection: sqlite3.Connection) -> None: @@ -104,14 +109,23 @@ def _validate_schema(connection: sqlite3.Connection) -> None: connection.executescript(_SqlCache.get("setup_session_script.sql", cache=False)) -def connect(db_path: Path | str, trace: bool = False) -> None: - _DbState.connect(db_path, trace=trace) +def connect(db_path: Path | str) -> None: + _DbState.connect(db_path, trace=bool(os.environ.get("TRACESQL"))) def close() -> None: _DbState.close() +@contextlib.contextmanager +def managed_connection(db_path: Path | str) -> Generator[None, None, None]: + try: + _DbState.connect(db_path) + yield None + finally: + _DbState.close() + + class DownloadStatus(enum.IntEnum): """Status of song in download queue.""" diff --git a/src/usdb_syncer/db/sql/setup_session_script.sql b/src/usdb_syncer/db/sql/setup_session_script.sql index ee8832f1..818fab6e 100644 --- a/src/usdb_syncer/db/sql/setup_session_script.sql +++ b/src/usdb_syncer/db/sql/setup_session_script.sql @@ -1,6 +1,6 @@ BEGIN; -CREATE TEMPORARY TABLE session_usdb_song ( +CREATE TEMPORARY TABLE IF NOT EXISTS session_usdb_song ( song_id INTEGER NOT NULL, status INTEGER NOT NULL DEFAULT 0, is_playing BOOLEAN NOT NULL DEFAULT false, @@ -10,4 +10,6 @@ CREATE TEMPORARY TABLE session_usdb_song ( PRAGMA foreign_keys = ON; +PRAGMA journal_mode = WAL; + COMMIT; \ No newline at end of file diff --git a/src/usdb_syncer/gui/__init__.py b/src/usdb_syncer/gui/__init__.py index 0a29ff04..445be7f5 100644 --- a/src/usdb_syncer/gui/__init__.py +++ b/src/usdb_syncer/gui/__init__.py @@ -87,7 +87,7 @@ def _load_main_window(mw: MainWindow) -> None: QtWidgets.QApplication.processEvents() splash.showMessage("Loading song database ...", color=Qt.GlobalColor.gray) folder = settings.get_song_dir() - db.connect(utils.AppPaths.db, trace=bool(os.environ.get("TRACESQL"))) + db.connect(utils.AppPaths.db) with db.transaction(): song_routines.load_available_songs(force_reload=False) song_routines.synchronize_sync_meta_folder(folder) diff --git a/src/usdb_syncer/gui/progress.py b/src/usdb_syncer/gui/progress.py index 449ff6d2..f41156a7 100644 --- a/src/usdb_syncer/gui/progress.py +++ b/src/usdb_syncer/gui/progress.py @@ -7,7 +7,7 @@ import attrs from PySide6 import QtCore, QtGui, QtWidgets -from usdb_syncer import logger +from usdb_syncer import db, logger, utils _logger = logger.get_logger(__file__) T = TypeVar("T") @@ -74,7 +74,8 @@ def run_with_progress( def wrapped_task() -> None: nonlocal result try: - result = Result(task()) + with db.managed_connection(utils.AppPaths.db): + result = Result(task()) except Exception as exc: # pylint: disable=broad-exception-caught result = Result(_Error(exc)) signal.result.emit() diff --git a/src/usdb_syncer/song_loader.py b/src/usdb_syncer/song_loader.py index 7b0eb55e..d339ab98 100644 --- a/src/usdb_syncer/song_loader.py +++ b/src/usdb_syncer/song_loader.py @@ -335,33 +335,34 @@ def __init__(self, song: UsdbSong, options: download_options.Options) -> None: self.logger = get_logger(__file__, self.song_id) def run(self) -> None: - try: - self.song = self._run_inner() - except errors.AbortError: - self.logger.info("Download aborted by user request.") - self.song.status = DownloadStatus.NONE - except errors.UsdbLoginError: - self.logger.error("Aborted; download requires login.") - self.song.status = DownloadStatus.FAILED - except errors.UsdbNotFoundError: - self.logger.error("Song has been deleted from USDB.") + with db.managed_connection(utils.AppPaths.db): + try: + self.song = self._run_inner() + except errors.AbortError: + self.logger.info("Download aborted by user request.") + self.song.status = DownloadStatus.NONE + except errors.UsdbLoginError: + self.logger.error("Aborted; download requires login.") + self.song.status = DownloadStatus.FAILED + except errors.UsdbNotFoundError: + self.logger.error("Song has been deleted from USDB.") + with db.transaction(): + self.song.delete() + events.SongDeleted(self.song_id).post() + events.DownloadFinished(self.song_id).post() + return + except Exception: # pylint: disable=broad-except + self.logger.debug(traceback.format_exc()) + self.logger.error( + "Failed to finish download due to an unexpected error. " + "See debug log for more information." + ) + self.song.status = DownloadStatus.FAILED + else: + self.song.status = DownloadStatus.NONE + self.logger.info("All done!") with db.transaction(): - self.song.delete() - events.SongDeleted(self.song_id).post() - events.DownloadFinished(self.song_id).post() - return - except Exception: # pylint: disable=broad-except - self.logger.debug(traceback.format_exc()) - self.logger.error( - "Failed to finish download due to an unexpected error. " - "See debug log for more information." - ) - self.song.status = DownloadStatus.FAILED - else: - self.song.status = DownloadStatus.NONE - self.logger.info("All done!") - with db.transaction(): - self.song.upsert() + self.song.upsert() events.SongChanged(self.song_id).post() events.DownloadFinished(self.song_id).post() diff --git a/src/usdb_syncer/usdb_song.py b/src/usdb_syncer/usdb_song.py index 29dcbc2c..014acfcc 100644 --- a/src/usdb_syncer/usdb_song.py +++ b/src/usdb_syncer/usdb_song.py @@ -114,7 +114,7 @@ def remove_sync_meta(self) -> None: if self.sync_meta: self.sync_meta.delete() self.sync_meta = None - _UsdbSongCache.remove(self.song_id) + _UsdbSongCache.update(self) @classmethod def delete_all(cls) -> None: @@ -128,7 +128,7 @@ def upsert(self) -> None: db.upsert_usdb_songs_creators([(self.song_id, self.creators())]) if self.sync_meta: self.sync_meta.upsert() - _UsdbSongCache.remove(self.song_id) + _UsdbSongCache.update(self) @classmethod def upsert_many(cls, songs: list[UsdbSong]) -> None: @@ -138,7 +138,7 @@ def upsert_many(cls, songs: list[UsdbSong]) -> None: db.upsert_usdb_songs_creators([(s.song_id, s.creators()) for s in songs]) SyncMeta.upsert_many([song.sync_meta for song in songs if song.sync_meta]) for song in songs: - _UsdbSongCache.remove(song.song_id) + _UsdbSongCache.update(song) def db_params(self) -> db.UsdbSongParams: return db.UsdbSongParams( diff --git a/tests/unit/test_db.py b/tests/unit/test_db.py index c34dfdfc..855cf66a 100644 --- a/tests/unit/test_db.py +++ b/tests/unit/test_db.py @@ -9,10 +9,10 @@ def test_persisting_usdb_song(song: UsdbSong) -> None: - db.connect(":memory:") - song.upsert() - db.reset_active_sync_metas(Path("C:")) - db_song = UsdbSong.get(song.song_id) + with db.managed_connection(":memory:"): + song.upsert() + db.reset_active_sync_metas(Path("C:")) + db_song = UsdbSong.get(song.song_id) assert db_song assert attrs.asdict(song) == attrs.asdict(db_song) @@ -29,17 +29,17 @@ def test_persisting_saved_search() -> None: years=[1990, 2000, 2010], ), ) - db.connect(":memory:") - search.insert() - saved = list(db.SavedSearch.load_saved_searches()) - assert len(saved) == 1 - assert search.name == "name" - assert saved[0] == search - - search.insert() - assert search.name == "name (1)" - assert len(list(db.SavedSearch.load_saved_searches())) == 2 - - search.update(new_name="name") - assert search.name == "name (1)" - assert len(list(db.SavedSearch.load_saved_searches())) == 2 + with db.managed_connection(":memory:"): + search.insert() + saved = list(db.SavedSearch.load_saved_searches()) + assert len(saved) == 1 + assert search.name == "name" + assert saved[0] == search + + search.insert() + assert search.name == "name (1)" + assert len(list(db.SavedSearch.load_saved_searches())) == 2 + + search.update(new_name="name") + assert search.name == "name (1)" + assert len(list(db.SavedSearch.load_saved_searches())) == 2 From df8d2a7649583de61a0e7dc5c25474e79eb5f878 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 21:15:00 +0200 Subject: [PATCH 4/7] Make sure runnable still exists when aborting Was causing "RuntimeError: Internal C++ object (_SongLoader) already deleted." --- src/usdb_syncer/song_loader.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/usdb_syncer/song_loader.py b/src/usdb_syncer/song_loader.py index d339ab98..1d884e64 100644 --- a/src/usdb_syncer/song_loader.py +++ b/src/usdb_syncer/song_loader.py @@ -18,6 +18,7 @@ import mutagen.oggopus import mutagen.oggvorbis import send2trash +import shiboken6 from mutagen import id3 from mutagen.flac import Picture from PIL import Image @@ -63,7 +64,7 @@ def download(cls, songs: Iterable[UsdbSong]) -> None: @classmethod def abort(cls, songs: Iterable[SongId]) -> None: for song in songs: - if job := cls._jobs.get(song): + if (job := cls._jobs.get(song)) and shiboken6.isValid(job): if cls._threadpool().tryTake(job): job.logger.info("Download aborted by user request.") job.song.status = DownloadStatus.NONE From 17261d9bc42d075a46dedc249dfeaea19f2fa563 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 21:39:11 +0200 Subject: [PATCH 5/7] Fix setting journal mode --- src/usdb_syncer/db/sql/setup_session_script.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/usdb_syncer/db/sql/setup_session_script.sql b/src/usdb_syncer/db/sql/setup_session_script.sql index 818fab6e..5c05875c 100644 --- a/src/usdb_syncer/db/sql/setup_session_script.sql +++ b/src/usdb_syncer/db/sql/setup_session_script.sql @@ -1,3 +1,5 @@ +PRAGMA journal_mode = WAL; + BEGIN; CREATE TEMPORARY TABLE IF NOT EXISTS session_usdb_song ( @@ -10,6 +12,4 @@ CREATE TEMPORARY TABLE IF NOT EXISTS session_usdb_song ( PRAGMA foreign_keys = ON; -PRAGMA journal_mode = WAL; - COMMIT; \ No newline at end of file From dea706192d9c77de62b974c460a97cbdfbdd2e04 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 24 Aug 2024 21:51:03 +0200 Subject: [PATCH 6/7] Add shiboken6 to pylint allow list --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 8bf4aafe..b578854d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,7 +81,7 @@ load-plugins = "pylint.extensions.mccabe" good-names = ["mw", "p1", "p2", "closeEvent", "customEvent"] [tool.pylint.messages_control] -extension-pkg-whitelist = ["PySide6"] +extension-pkg-whitelist = ["PySide6", "shiboken6"] disable = [ "too-few-public-methods", "logging-fstring-interpolation", From 0deb5946073a57675ab5662c88e68174a616b6a8 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sun, 25 Aug 2024 11:25:39 +0200 Subject: [PATCH 7/7] Fix outdated cache after loading sync metas --- src/usdb_syncer/gui/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/usdb_syncer/gui/__init__.py b/src/usdb_syncer/gui/__init__.py index 445be7f5..103bc132 100644 --- a/src/usdb_syncer/gui/__init__.py +++ b/src/usdb_syncer/gui/__init__.py @@ -23,6 +23,7 @@ settings, song_routines, sync_meta, + usdb_song, utils, ) @@ -92,6 +93,7 @@ def _load_main_window(mw: MainWindow) -> None: song_routines.load_available_songs(force_reload=False) song_routines.synchronize_sync_meta_folder(folder) sync_meta.SyncMeta.reset_active(folder) + usdb_song.UsdbSong.clear_cache() default_search = db.SavedSearch.get_default() mw.tree.populate() if default_search: