-
Notifications
You must be signed in to change notification settings - Fork 0
Claude/fix persistent sqlite hwb9 q #17
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
Changes from all commits
a83a7f8
9c7862c
e2e0a30
d06f339
254be38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## 2025-03-20 - Database Connection Pooling | ||
| **Learning:** `streamrip/db.py` creates a new `sqlite3.connect` for *every single database operation* (`contains`, `add`, `all`). In bulk operations like resolving a playlist with hundreds of tracks, this introduces significant overhead. | ||
| **Action:** Use a single persistent connection per database instance instead of creating a new one on every method call. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,21 +68,39 @@ def __init__(self, path: str): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.path = path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not os.path.exists(self.path): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # sqlite3.connect creates the file if it does not exist, so we check | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # existence beforehand to know if we need to call create() later | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exists = os.path.exists(self.path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # ⚡ Bolt: Cache persistent SQLite connection to avoid recreating it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # on every db check/add. This gives ~10x speedup for database operations | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # like downloading a playlist where it does hundreds of ID checks sequentially. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.conn = sqlite3.connect(self.path, check_same_thread=False) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not exists or not self._table_exists(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.create() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __del__(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Ensure connection is closed on exit.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if hasattr(self, 'conn') and self.conn: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.conn.close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+83
to
+86
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __del__(self): | |
| """Ensure connection is closed on exit.""" | |
| if hasattr(self, 'conn') and self.conn: | |
| self.conn.close() | |
| def close(self) -> None: | |
| """Close the underlying SQLite connection, if open.""" | |
| conn = getattr(self, "conn", None) | |
| if conn is not None: | |
| try: | |
| conn.close() | |
| except Exception: | |
| # Best-effort close; failures are not fatal. | |
| logger.debug("Error while closing database connection", exc_info=True) | |
| finally: | |
| self.conn = None | |
| def __enter__(self): | |
| """Support use as a context manager.""" | |
| return self | |
| def __exit__(self, exc_type, exc_val, exc_tb): | |
| """Ensure the connection is closed when leaving a context manager.""" | |
| self.close() | |
| def __del__(self): | |
| """Best-effort attempt to close the connection on garbage collection.""" | |
| try: | |
| self.close() | |
| except Exception: | |
| # Avoid raising during interpreter shutdown | |
| pass |
Copilot
AI
Mar 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repo formatting config enforces double quotes; these single-quoted string literals will be reformatted (and may fail CI if formatting checks run). Please use double quotes for consistency.
Copilot
AI
Mar 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add() catches sqlite3.IntegrityError but does not ROLLBACK the transaction. With a long-lived connection this can leave the connection in an open transaction and hold locks, causing subsequent operations (or other processes) to hit database is locked. Roll back on errors (or wrap DML in with self.conn: so commit/rollback is handled consistently).
| # tried to insert an item that was already there | |
| # tried to insert an item that was already there | |
| self.conn.rollback() |
Copilot
AI
Mar 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repo formatting config enforces double quotes; use double quotes here as well (this currently deviates from tool.ruff.format.quote-style = "double").
| if hasattr(self, 'conn') and self.conn: | |
| if hasattr(self, "conn") and self.conn: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_same_thread=Falsedisables sqlite3's built-in guardrails, but there is no locking/serialization aroundself.connaccess. If this instance is ever used across threads, this can lead to unsafe concurrent access. If cross-thread use isn't required, keep the default (True); if it is required, add a lock around all DB operations (or use one connection per thread).