From a28f930c52d8c820d163a54efb6c9d578fcf71d6 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sun, 6 May 2012 23:24:05 -0700 Subject: [PATCH] transaction objects to control DB access In an attempt to finally address the longstanding SQLite locking issues, I'm introducing a way to explicitly, lexically scope transactions. The Transaction class is a context manager that always fully fetches after SELECTs and automatically commits on exit. No direct access to the library is allowed, so all changes will eventually be committed and all queries will be completed. This will also provide a debugging mechanism to show where concurrent transactions are beginning and ending. To support composition (transaction reentrancy), an internal, per-Library stack of transactions is maintained. Commits only happen when the outermost transaction exits. This means that, while it's possible to introduce atomicity bugs by invoking Library methods outside of a transaction, you can conveniently call them *without* a currently-active transaction to get a single atomic action. Note that this "transaction stack" concepts assumes a single Library object per thread. Because we need to duplicate Library objects for concurrent access due to sqlite3 limitation already, this is fine for now. Later, the interface should provide one transaction stack per thread for shared Library objects. --- beets/importer.py | 15 +- beets/library.py | 272 ++++++++++++++++++-------------- beets/ui/commands.py | 194 +++++++++++------------ beetsplug/bpd/__init__.py | 19 ++- beetsplug/lastgenre/__init__.py | 2 - beetsplug/lyrics.py | 1 - beetsplug/mpdupdate.py | 2 +- beetsplug/web/__init__.py | 10 +- docs/changelog.rst | 2 + docs/plugins/writing.rst | 3 - test/test_db.py | 121 +++++++------- test/test_importer.py | 4 +- test/test_query.py | 7 +- test/test_ui.py | 2 +- test/test_vfs.py | 1 - 15 files changed, 343 insertions(+), 312 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index d081f8492..4af83dcd8 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -718,7 +718,7 @@ def apply_choices(config): # Add items -- before path changes -- to the library. We add the # items now (rather than at the end) so that album structures # are in place before calls to destination(). - try: + with lib.transaction(): # Remove old items. for replaced in replaced_items.itervalues(): for item in replaced: @@ -735,8 +735,6 @@ def apply_choices(config): # Add tracks. for item in items: lib.add(item) - finally: - lib.save() # Move/copy files. task.old_paths = [item.path for item in items] # For deletion. @@ -764,11 +762,9 @@ def apply_choices(config): item.write() # Save new paths. - try: + with lib.transaction(): for item in items: lib.store(item) - finally: - lib.save() def fetch_art(config): """A coroutine that fetches and applies album art for albums where @@ -786,11 +782,8 @@ def fetch_art(config): # Save the art if any was found. if artpath: - try: - album = lib.get_album(task.album_id) - album.set_art(artpath, not (config.delete or config.move)) - finally: - lib.save(False) + album = lib.get_album(task.album_id) + album.set_art(artpath, not (config.delete or config.move)) if (config.delete or config.move) and task.toppath: task.prune(artpath) diff --git a/beets/library.py b/beets/library.py index f705c2a62..89b2a7e18 100644 --- a/beets/library.py +++ b/beets/library.py @@ -8,10 +8,11 @@ # distribute, sublicense, and/or sell copies of the Software, and to # permit persons to whom the Software is furnished to do so, subject to # the following conditions: -# +# # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. +from __future__ import with_statement import sqlite3 import os import re @@ -165,7 +166,7 @@ class Item(object): self.dirty = {} self._fill_record(values) self._clear_dirty() - + @classmethod def from_path(cls, path): """Creates a new item from the media file at the specified path. @@ -175,7 +176,7 @@ class Item(object): 'album_id': None, }) i.read(path) - i.mtime = i.current_mtime() # Initial mtime. + i.mtime = i.current_mtime() # Initial mtime. return i def _fill_record(self, values): @@ -211,7 +212,7 @@ class Item(object): sets the record entry for that key to value. Note that to change the attribute in the database or in the file's tags, one must call store() or write(). - + Otherwise, performs an ordinary setattr. """ # Encode unicode paths and read buffers. @@ -230,10 +231,10 @@ class Item(object): self.mtime = 0 # Reset mtime on dirty. else: super(Item, self).__setattr__(key, value) - - + + # Interaction with file metadata. - + def read(self, read_path=None): """Read the metadata from the associated file. If read_path is specified, read metadata from that file instead. @@ -251,7 +252,7 @@ class Item(object): # Database's mtime should now reflect the on-disk value. if read_path == self.path: self.mtime = self.current_mtime() - + def write(self): """Writes the item's metadata to the associated file. """ @@ -278,7 +279,7 @@ class Item(object): util.copy(self.path, dest) else: util.move(self.path, dest) - + # Either copying or moving succeeded, so update the stored path. self.path = dest @@ -366,16 +367,14 @@ class Query(object): clause, subvals = self.clause() return ('SELECT ' + columns + ' FROM items WHERE ' + clause, subvals) - def count(self, library): + def count(self, tx): """Returns `(num, length)` where `num` is the number of items in the library matching this query and `length` is their total length in seconds. """ clause, subvals = self.clause() statement = 'SELECT COUNT(id), SUM(length) FROM items WHERE ' + clause - c = library.conn.execute(statement, subvals) - result = c.fetchone() - c.close() + result = tx.query(statement, subvals)[0] return (result[0], result[1] or 0.0) class FieldQuery(Query): @@ -387,7 +386,7 @@ class FieldQuery(Query): raise InvalidFieldError(field + ' is not an item key') self.field = field self.pattern = pattern - + class MatchQuery(FieldQuery): """A query that looks for exact matches in an item field.""" def clause(self): @@ -457,7 +456,7 @@ class CollectionQuery(Query): """ def __init__(self, subqueries=()): self.subqueries = subqueries - + # is there a better way to do this? def __len__(self): return len(self.subqueries) def __getitem__(self, key): return self.subqueries[key] @@ -674,7 +673,7 @@ class PathQuery(Query): # Match the path as a single file. self.file_path = normpath(path) # As a directory (prefix). - self.dir_path = os.path.join(self.file_path, '') + self.dir_path = os.path.join(self.file_path, '') def match(self, item): return (item.path == self.file_path) or \ @@ -686,21 +685,18 @@ class PathQuery(Query): return '(path = ?) || (path LIKE ?)', (file_blob, dir_pat) class ResultIterator(object): - """An iterator into an item query result set. The iterator eagerly - fetches all of the results from the cursor but lazily constructs - Item objects that reflect them. + """An iterator into an item query result set. The iterator lazily + constructs Item objects that reflect database rows. """ - def __init__(self, cursor): - # Fetch all of the rows, closing the cursor (and unlocking the - # database). - self.rows = cursor.fetchall() + def __init__(self, rows): + self.rows = rows self.rowiter = iter(self.rows) - + def __iter__(self): return self - + def next(self): - row = self.rowiter.next() # May raise StopIteration. + row = self.rowiter.next() # May raise StopIteration. return Item(row) @@ -755,12 +751,6 @@ class BaseLibrary(object): """ raise NotImplementedError - def save(self): - """Ensure that the library is consistent on disk. A no-op by - default. - """ - pass - def load(self, item, load_id=None): """Refresh the item's metadata from the library database. If fetch_id is not specified, use the item's current id. @@ -887,6 +877,47 @@ class BaseAlbum(object): # Concrete DB-backed library. +class Transaction(object): + """A context manager for safe, concurrent access to the database. + All SQL commands should be executed through a transaction. + """ + def __init__(self, lib): + self.lib = lib + + def __enter__(self): + """Begin a transaction. This transaction may be created while + another is active. + """ + self.lib._tx_stack.append(self) + return self + + def __exit__(self, exc_type, exc_value, traceback): + """Complete a transaction. This must be the most recently + entered but not yet exited transaction. If it is the last active + transaction, the database updates are committed. + """ + assert self.lib._tx_stack.pop() is self + if not self.lib._tx_stack: + self.lib.conn.commit() + + def query(self, statement, subvals=()): + """Execute an SQL statement with substitution values and return + a list of rows from the database. + """ + cursor = self.lib.conn.execute(statement, subvals) + return cursor.fetchall() + + def mutate(self, statement, subvals=()): + """Execute an SQL statement with substitution values and return + the row ID of the last affected row. + """ + cursor = self.lib.conn.execute(statement, subvals) + return cursor.lastrowid + + def script(self, statements): + """Execute a string containing multiple SQL statements.""" + self.lib.conn.executescript(statements) + class Library(BaseLibrary): """A music library using an SQLite database as a metadata store.""" def __init__(self, path='library.blb', @@ -907,6 +938,9 @@ class Library(BaseLibrary): self.art_filename = bytestring_path(art_filename) self.replacements = replacements + self._memotable = {} # Used for template substitution performance. + self._tx_stack = [] + self.timeout = timeout self.conn = sqlite3.connect(self.path, timeout) # This way we can access our SELECT results like dictionaries. @@ -929,29 +963,27 @@ class Library(BaseLibrary): self._make_table('items', item_fields) self._make_table('albums', album_fields) - self._memotable = {} # Used for template substitution performance. - def _make_table(self, table, fields): """Set up the schema of the library file. fields is a list of all the fields that should be present in the indicated table. Columns are added if necessary. """ # Get current schema. - cur = self.conn.cursor() - cur.execute('PRAGMA table_info(%s)' % table) - current_fields = set([row[1] for row in cur]) - + with self.transaction() as tx: + rows = tx.query('PRAGMA table_info(%s)' % table) + current_fields = set([row[1] for row in rows]) + field_names = set([f[0] for f in fields]) if current_fields.issuperset(field_names): # Table exists and has all the required columns. return - + if not current_fields: - # No table exists. + # No table exists. setup_sql = 'CREATE TABLE %s (' % table setup_sql += ', '.join(['%s %s' % f[:2] for f in fields]) setup_sql += ');\n' - + else: # Table exists but is missing fields. setup_sql = '' @@ -971,8 +1003,14 @@ class Library(BaseLibrary): 'albumartist' not in current_fields: setup_sql += "UPDATE ALBUMS SET albumartist=artist;\n" - self.conn.executescript(setup_sql) - self.conn.commit() + with self.transaction() as tx: + tx.script(setup_sql) + + def transaction(self): + """Get a transaction object for interacting with the database. + This should *always* be used as a context manager. + """ + return Transaction(self) def destination(self, item, pathmod=None, fragment=False, basedir=None, platform=None): @@ -1031,7 +1069,7 @@ class Library(BaseLibrary): return subpath else: basedir = basedir or self.directory - return normpath(os.path.join(basedir, subpath)) + return normpath(os.path.join(basedir, subpath)) # Item manipulation. @@ -1041,7 +1079,7 @@ class Library(BaseLibrary): if copy: self.move(item, copy=True) - # build essential parts of query + # Build essential parts of query. columns = ','.join([key for key in ITEM_KEYS if key != 'id']) values = ','.join(['?'] * (len(ITEM_KEYS) - 1)) subvars = [] @@ -1052,41 +1090,30 @@ class Library(BaseLibrary): value = buffer(value) subvars.append(value) - # issue query - c = self.conn.cursor() + # Issue query. query = 'INSERT INTO items (' + columns + ') VALUES (' + values + ')' - c.execute(query, subvars) - new_id = c.lastrowid - c.close() + with self.transaction() as tx: + new_id = tx.mutate(query, subvars) item._clear_dirty() item.id = new_id self._memotable = {} return new_id - def save(self, event=True): - """Writes the library to disk (completing an sqlite - transaction). - """ - self.conn.commit() - if event: - plugins.send('save', lib=self) - def load(self, item, load_id=None): if load_id is None: load_id = item.id - c = self.conn.execute( - 'SELECT * FROM items WHERE id=?', (load_id,) ) - item._fill_record(c.fetchone()) + with self.transaction() as tx: + rows = tx.query('SELECT * FROM items WHERE id=?', (load_id,)) + item._fill_record(rows[0]) item._clear_dirty() - c.close() def store(self, item, store_id=None, store_all=False): if store_id is None: store_id = item.id - # build assignments for query + # Build assignments for query. assignments = '' subvars = [] for key in ITEM_KEYS: @@ -1102,14 +1129,15 @@ class Library(BaseLibrary): if not assignments: # nothing to store (i.e., nothing was dirty) return - - assignments = assignments[:-1] # knock off last , - # finish the query + assignments = assignments[:-1] # Knock off last , + + # Finish the query. query = 'UPDATE items SET ' + assignments + ' WHERE id=?' subvars.append(store_id) - self.conn.execute(query, subvars) + with self.transaction() as tx: + tx.mutate(query, subvars) item._clear_dirty() self._memotable = {} @@ -1121,7 +1149,8 @@ class Library(BaseLibrary): """ album = self.get_album(item) if with_album else None - self.conn.execute('DELETE FROM items WHERE id=?', (item.id,)) + with self.transaction() as tx: + tx.mutate('DELETE FROM items WHERE id=?', (item.id,)) if album: item_iter = album.items() @@ -1143,26 +1172,26 @@ class Library(BaseLibrary): directory (provided by destination()). Subdirectories are created as needed. If the operation succeeds, the item's path field is updated to reflect the new location. - + If copy is True, moving the file is copied rather than moved. - + basedir overrides the library base directory for the destination. If the item is in an album, the album is given an opportunity to move its art. (This can be disabled by passing with_album=False.) - + The item is stored to the database if it is in the database, so any dirty fields prior to the move() call will be written as a side effect. You probably want to call save() to commit the DB transaction. """ dest = self.destination(item, basedir=basedir) - + # Create necessary ancestry for the move. util.mkdirall(dest) - + # Perform the move and store the change. old_path = item.path item.move(dest, copy) @@ -1192,8 +1221,9 @@ class Library(BaseLibrary): "WHERE " + where + \ " ORDER BY %s, album" % \ _orelse("albumartist_sort", "albumartist") - c = self.conn.execute(sql, subvals) - return [Album(self, dict(res)) for res in c.fetchall()] + with self.transaction() as tx: + rows = tx.query(sql, subvals) + return [Album(self, dict(res)) for res in rows] def items(self, query=None, artist=None, album=None, title=None): queries = [self._get_query(query, False)] @@ -1211,8 +1241,9 @@ class Library(BaseLibrary): " ORDER BY %s, album, disc, track" % \ _orelse("artist_sort", "artist") log.debug('Getting items with SQL: %s' % sql) - c = self.conn.execute(sql, subvals) - return ResultIterator(c) + with self.transaction() as tx: + rows = tx.query(sql, subvals) + return ResultIterator(rows) # Convenience accessors. @@ -1220,13 +1251,14 @@ class Library(BaseLibrary): def get_item(self, id): """Fetch an Item by its ID. Returns None if no match is found. """ - c = self.conn.execute("SELECT * FROM items WHERE id=?", (id,)) - it = ResultIterator(c) + with self.transaction() as tx: + rows = tx.query("SELECT * FROM items WHERE id=?", (id,)) + it = ResultIterator(rows) try: return it.next() except StopIteration: return None - + def get_album(self, item_or_id): """Given an album ID or an item associated with an album, return an Album object for the album. If no such album exists, @@ -1239,16 +1271,13 @@ class Library(BaseLibrary): if album_id is None: return None - c = self.conn.execute( - 'SELECT * FROM albums WHERE id=?', - (album_id,) - ) - try: - record = c.fetchone() - finally: - c.close() - if record: - return Album(self, dict(record)) + with self.transaction() as tx: + rows = tx.query( + 'SELECT * FROM albums WHERE id=?', + (album_id,) + ) + if rows: + return Album(self, dict(rows[0])) def add_album(self, items): """Create a new album in the database with metadata derived @@ -1260,12 +1289,20 @@ class Library(BaseLibrary): item_values = dict( (key, getattr(items[0], key)) for key in ALBUM_KEYS_ITEM) - sql = 'INSERT INTO albums (%s) VALUES (%s)' % \ - (', '.join(ALBUM_KEYS_ITEM), - ', '.join(['?'] * len(ALBUM_KEYS_ITEM))) - subvals = [item_values[key] for key in ALBUM_KEYS_ITEM] - c = self.conn.execute(sql, subvals) - album_id = c.lastrowid + with self.transaction() as tx: + sql = 'INSERT INTO albums (%s) VALUES (%s)' % \ + (', '.join(ALBUM_KEYS_ITEM), + ', '.join(['?'] * len(ALBUM_KEYS_ITEM))) + subvals = [item_values[key] for key in ALBUM_KEYS_ITEM] + album_id = tx.mutate(sql, subvals) + + # Add the items to the library. + for item in items: + item.album_id = album_id + if item.id is None: + self.add(item) + else: + self.store(item) # Construct the new Album object. record = {} @@ -1278,14 +1315,6 @@ class Library(BaseLibrary): record['id'] = album_id album = Album(self, record) - # Add the items to the library. - for item in items: - item.album_id = album_id - if item.id is None: - self.add(item) - else: - self.store(item) - return album class Album(BaseAlbum): @@ -1318,7 +1347,8 @@ class Album(BaseAlbum): # Change album table. sql = 'UPDATE albums SET %s=? WHERE id=?' % key - self._library.conn.execute(sql, (value, self.id)) + with self._library.transaction() as tx: + tx.mutate(sql, (value, self.id)) # Possibly make modification on items as well. if key in ALBUM_KEYS_ITEM: @@ -1342,11 +1372,12 @@ class Album(BaseAlbum): """Returns an iterable over the items associated with this album. """ - c = self._library.conn.execute( - 'SELECT * FROM items WHERE album_id=?', - (self.id,) - ) - return ResultIterator(c) + with self._library.transaction() as tx: + rows = tx.query( + 'SELECT * FROM items WHERE album_id=?', + (self.id,) + ) + return ResultIterator(rows) def remove(self, delete=False, with_items=True): """Removes this album and all its associated items from the @@ -1355,22 +1386,23 @@ class Album(BaseAlbum): containing the album are also removed (recursively) if empty. Set with_items to False to avoid removing the album's items. """ - if with_items: - # Remove items. - for item in self.items(): - self._library.remove(item, delete, False) - if delete: # Delete art file. artpath = self.artpath if artpath: util.soft_remove(artpath) - # Remove album from database. - self._library.conn.execute( - 'DELETE FROM albums WHERE id=?', - (self.id,) - ) + with self._library.transaction() as tx: + if with_items: + # Remove items. + for item in self.items(): + self._library.remove(item, delete, False) + + # Remove album from database. + tx.mutate( + 'DELETE FROM albums WHERE id=?', + (self.id,) + ) def move_art(self, copy=False): """Move or copy any existing album art so that it remains in the diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 811ddf3fb..7436b8ed3 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -849,89 +849,89 @@ def update_items(lib, query, album, move, color, pretend): """For all the items matched by the query, update the library to reflect the item's embedded tags. """ - items, _ = _do_query(lib, query, album) + with lib.transaction(): + items, _ = _do_query(lib, query, album) - # Walk through the items and pick up their changes. - affected_albums = set() - for item in items: - # Item deleted? - if not os.path.exists(syspath(item.path)): - print_(u'X %s - %s' % (item.artist, item.title)) - if not pretend: - lib.remove(item, True) - affected_albums.add(item.album_id) - continue - - # Did the item change since last checked? - if item.current_mtime() <= item.mtime: - log.debug(u'skipping %s because mtime is up to date (%i)' % - (displayable_path(item.path), item.mtime)) - continue - - # Read new data. - old_data = dict(item.record) - item.read() - - # Special-case album artist when it matches track artist. (Hacky - # but necessary for preserving album-level metadata for non- - # autotagged imports.) - if not item.albumartist and \ - old_data['albumartist'] == old_data['artist'] == item.artist: - item.albumartist = old_data['albumartist'] - item.dirty['albumartist'] = False - - # Get and save metadata changes. - changes = {} - for key in library.ITEM_KEYS_META: - if item.dirty[key]: - changes[key] = old_data[key], getattr(item, key) - if changes: - # Something changed. - print_(u'* %s - %s' % (item.artist, item.title)) - for key, (oldval, newval) in changes.iteritems(): - _showdiff(key, oldval, newval, color) - - # If we're just pretending, then don't move or save. - if pretend: + # Walk through the items and pick up their changes. + affected_albums = set() + for item in items: + # Item deleted? + if not os.path.exists(syspath(item.path)): + print_(u'X %s - %s' % (item.artist, item.title)) + if not pretend: + lib.remove(item, True) + affected_albums.add(item.album_id) continue - # Move the item if it's in the library. - if move and lib.directory in ancestry(item.path): - lib.move(item) + # Did the item change since last checked? + if item.current_mtime() <= item.mtime: + log.debug(u'skipping %s because mtime is up to date (%i)' % + (displayable_path(item.path), item.mtime)) + continue - lib.store(item) - affected_albums.add(item.album_id) - elif not pretend: - # The file's mtime was different, but there were no changes - # to the metadata. Store the new mtime, which is set in the - # call to read(), so we don't check this again in the - # future. - lib.store(item) + # Read new data. + old_data = dict(item.record) + item.read() - # Skip album changes while pretending. - if pretend: - return + # Special-case album artist when it matches track artist. (Hacky + # but necessary for preserving album-level metadata for non- + # autotagged imports.) + if not item.albumartist and \ + old_data['albumartist'] == old_data['artist'] == \ + item.artist: + item.albumartist = old_data['albumartist'] + item.dirty['albumartist'] = False - # Modify affected albums to reflect changes in their items. - for album_id in affected_albums: - if album_id is None: # Singletons. - continue - album = lib.get_album(album_id) - if not album: # Empty albums have already been removed. - log.debug('emptied album %i' % album_id) - continue - al_items = list(album.items()) + # Get and save metadata changes. + changes = {} + for key in library.ITEM_KEYS_META: + if item.dirty[key]: + changes[key] = old_data[key], getattr(item, key) + if changes: + # Something changed. + print_(u'* %s - %s' % (item.artist, item.title)) + for key, (oldval, newval) in changes.iteritems(): + _showdiff(key, oldval, newval, color) - # Update album structure to reflect an item in it. - for key in library.ALBUM_KEYS_ITEM: - setattr(album, key, getattr(al_items[0], key)) + # If we're just pretending, then don't move or save. + if pretend: + continue - # Move album art (and any inconsistent items). - if move and lib.directory in ancestry(al_items[0].path): - log.debug('moving album %i' % album_id) - album.move() + # Move the item if it's in the library. + if move and lib.directory in ancestry(item.path): + lib.move(item) - lib.save() + lib.store(item) + affected_albums.add(item.album_id) + elif not pretend: + # The file's mtime was different, but there were no changes + # to the metadata. Store the new mtime, which is set in the + # call to read(), so we don't check this again in the + # future. + lib.store(item) + + # Skip album changes while pretending. + if pretend: + return + + # Modify affected albums to reflect changes in their items. + for album_id in affected_albums: + if album_id is None: # Singletons. + continue + album = lib.get_album(album_id) + if not album: # Empty albums have already been removed. + log.debug('emptied album %i' % album_id) + continue + al_items = list(album.items()) + + # Update album structure to reflect an item in it. + for key in library.ALBUM_KEYS_ITEM: + setattr(album, key, getattr(al_items[0], key)) + + # Move album art (and any inconsistent items). + if move and lib.directory in ancestry(al_items[0].path): + log.debug('moving album %i' % album_id) + album.move() update_cmd = ui.Subcommand('update', help='update the library', aliases=('upd','up',)) @@ -972,14 +972,13 @@ def remove_items(lib, query, album, delete=False): return # Remove (and possibly delete) items. - if album: - for al in albums: - al.remove(delete) - else: - for item in items: - lib.remove(item, delete) - - lib.save() + with lib.transaction(): + if album: + for al in albums: + al.remove(delete) + else: + for item in items: + lib.remove(item, delete) remove_cmd = ui.Subcommand('remove', help='remove matching items from the library', aliases=('rm',)) @@ -1091,23 +1090,23 @@ def modify_items(lib, mods, query, write, move, album, color, confirm): return # Apply changes to database. - for obj in objs: - for field, value in fsets.iteritems(): - setattr(obj, field, value) + with lib.transaction(): + for obj in objs: + for field, value in fsets.iteritems(): + setattr(obj, field, value) - if move: - cur_path = obj.item_dir() if album else obj.path - if lib.directory in ancestry(cur_path): # In library? - log.debug('moving object %s' % cur_path) - if album: - obj.move() - else: - lib.move(obj) + if move: + cur_path = obj.item_dir() if album else obj.path + if lib.directory in ancestry(cur_path): # In library? + log.debug('moving object %s' % cur_path) + if album: + obj.move() + else: + lib.move(obj) - # When modifying items, we have to store them to the database. - if not album: - lib.store(obj) - lib.save() + # When modifying items, we have to store them to the database. + if not album: + lib.store(obj) # Apply tags if requested. if write: @@ -1166,7 +1165,6 @@ def move_items(lib, dest, query, copy, album): else: lib.move(obj, copy, basedir=dest) lib.store(obj) - lib.save() move_cmd = ui.Subcommand('move', help='move or copy items', aliases=('mv',)) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index b9cb30ba2..2f73dfdc8 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -892,14 +892,13 @@ class Server(BaseServer): def cmd_stats(self, conn): """Sends some statistics about the library.""" - songs, totaltime = beets.library.TrueQuery().count(self.lib) + with self.lib.transaction() as tx: + songs, totaltime = beets.library.TrueQuery().count(tx) - statement = 'SELECT COUNT(DISTINCT artist), ' \ - 'COUNT(DISTINCT album) FROM items' - c = self.lib.conn.execute(statement) - result = c.fetchone() - c.close() - artists, albums = result[0], result[1] + statement = 'SELECT COUNT(DISTINCT artist), ' \ + 'COUNT(DISTINCT album) FROM items' + result = tx.query(statement)[0] + artists, albums = result[0], result[1] yield (u'artists: ' + unicode(artists), u'albums: ' + unicode(albums), @@ -996,10 +995,10 @@ class Server(BaseServer): statement = 'SELECT DISTINCT ' + show_key + \ ' FROM items WHERE ' + clause + \ ' ORDER BY ' + show_key - c = self.lib.conn.cursor() - c.execute(statement, subvals) + with self.lib.transaction() as tx: + rows = tx.query(statement, subvals) - for row in c: + for row in rows: yield show_tag_canon + u': ' + unicode(row[0]) def cmd_count(self, conn, tag, value): diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index 8fd3a7463..ccfb7ec74 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -166,7 +166,6 @@ def album_imported(lib, album, config): if genre: log.debug(u'adding last.fm album genre: %s' % genre) album.genre = genre - lib.save() if config.write: for item in album.items(): @@ -180,7 +179,6 @@ def item_imported(lib, item, config): log.debug(u'adding last.fm item genre: %s' % genre) item.genre = genre lib.store(item) - lib.save() if config.write: item.write() diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py index 6056ba83c..cccbca772 100644 --- a/beetsplug/lyrics.py +++ b/beetsplug/lyrics.py @@ -177,7 +177,6 @@ def fetch_item_lyrics(lib, loglevel, item, write): if write: item.write() lib.store(item) - lib.save() AUTOFETCH = True class LyricsPlugin(BeetsPlugin): diff --git a/beetsplug/mpdupdate.py b/beetsplug/mpdupdate.py index 7c4e5a8dd..c010d068b 100644 --- a/beetsplug/mpdupdate.py +++ b/beetsplug/mpdupdate.py @@ -101,6 +101,6 @@ class MPDUpdatePlugin(BeetsPlugin): options['password'] = \ ui.config_val(config, 'mpdupdate', 'password', '') -@MPDUpdatePlugin.listen('save') +@MPDUpdatePlugin.listen('import') def update(lib=None): update_mpd(options['host'], options['port'], options['password']) diff --git a/beetsplug/web/__init__.py b/beetsplug/web/__init__.py index 43ccd9742..d6194e14e 100644 --- a/beetsplug/web/__init__.py +++ b/beetsplug/web/__init__.py @@ -59,8 +59,9 @@ def single_item(item_id): @app.route('/item/') def all_items(): - c = g.lib.conn.execute("SELECT id FROM items") - all_ids = [row[0] for row in c] + with g.lib.transaction() as tx: + rows = tx.query("SELECT id FROM items") + all_ids = [row[0] for row in rows] return flask.jsonify(item_ids=all_ids) @app.route('/item//file') @@ -84,8 +85,9 @@ def single_album(album_id): @app.route('/album/') def all_albums(): - c = g.lib.conn.execute("SELECT id FROM albums") - all_ids = [row[0] for row in c] + with g.lib.transaction() as tx: + rows = tx.query("SELECT id FROM albums") + all_ids = [row[0] for row in rows] return flask.jsonify(album_ids=all_ids) @app.route('/album/query/') diff --git a/docs/changelog.rst b/docs/changelog.rst index 58f3f7a4d..a2e1e7b93 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -64,6 +64,8 @@ Changelog Mecucci). * Filenames are normalized with Unicode Normal Form D (NFD) on Mac OS X and NFC on all other platforms. +* Significant internal restructuring to avoid SQLite locking errors. As part of + these changes, the not-very-useful "save" plugin event has been removed. .. _pyacoustid: https://github.com/sampsyo/pyacoustid diff --git a/docs/plugins/writing.rst b/docs/plugins/writing.rst index ba11ed322..be0a4d437 100644 --- a/docs/plugins/writing.rst +++ b/docs/plugins/writing.rst @@ -116,9 +116,6 @@ currently available are: * *pluginload*: called after all the plugins have been loaded after the ``beet`` command starts -* *save*: called whenever the library is changed and written to disk (the - ``lib`` keyword argument is the Library object that was written) - * *import*: called after a ``beet import`` command fishes (the ``lib`` keyword argument is a Library object; ``paths`` is a list of paths (strings) that were imported) diff --git a/test/test_db.py b/test/test_db.py index b04ba2e15..e1c95aa46 100644 --- a/test/test_db.py +++ b/test/test_db.py @@ -8,7 +8,7 @@ # distribute, sublicense, and/or sell copies of the Software, and to # permit persons to whom the Software is furnished to do so, subject to # the following conditions: -# +# # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. @@ -29,10 +29,18 @@ import beets.library from beets import util from beets import plugins +TEMP_LIB = os.path.join(_common.RSRC, 'test_copy.blb') + def lib(): - return beets.library.Library(os.path.join(_common.RSRC, 'test.blb')) -def boracay(l): return beets.library.Item(l.conn.execute('select * from items ' - 'where id=3').fetchone()) + shutil.copy(os.path.join(_common.RSRC, 'test.blb'), TEMP_LIB) + return beets.library.Library(TEMP_LIB) +def remove_lib(): + if os.path.exists(TEMP_LIB): + os.unlink(TEMP_LIB) +def boracay(l): + return beets.library.Item( + l.conn.execute('select * from items where id=3').fetchone() + ) np = util.normpath class LoadTest(unittest.TestCase): @@ -41,13 +49,14 @@ class LoadTest(unittest.TestCase): self.i = boracay(self.lib) def tearDown(self): self.lib.conn.close() - + remove_lib() + def test_load_restores_data_from_db(self): original_title = self.i.title self.i.title = 'something' self.lib.load(self.i) self.assertEqual(original_title, self.i.title) - + def test_load_clears_dirty_flags(self): self.i.artist = 'something' self.lib.load(self.i) @@ -59,14 +68,15 @@ class StoreTest(unittest.TestCase): self.i = boracay(self.lib) def tearDown(self): self.lib.conn.close() - + remove_lib() + def test_store_changes_database_value(self): self.i.year = 1987 self.lib.store(self.i) new_year = self.lib.conn.execute('select year from items where ' 'title="Boracay"').fetchone()['year'] self.assertEqual(new_year, 1987) - + def test_store_only_writes_dirty_fields(self): original_genre = self.i.genre self.i.record['genre'] = 'beatboxing' # change value w/o dirtying @@ -74,7 +84,7 @@ class StoreTest(unittest.TestCase): new_genre = self.lib.conn.execute('select genre from items where ' 'title="Boracay"').fetchone()['genre'] self.assertEqual(new_genre, original_genre) - + def test_store_clears_dirty_flags(self): self.i.composer = 'tvp' self.lib.store(self.i) @@ -86,27 +96,28 @@ class AddTest(unittest.TestCase): self.i = item() def tearDown(self): self.lib.conn.close() - + def test_item_add_inserts_row(self): self.lib.add(self.i) new_grouping = self.lib.conn.execute('select grouping from items ' 'where composer="the composer"').fetchone()['grouping'] self.assertEqual(new_grouping, self.i.grouping) - + def test_library_add_path_inserts_row(self): i = beets.library.Item.from_path(os.path.join(_common.RSRC, 'full.mp3')) self.lib.add(i) new_grouping = self.lib.conn.execute('select grouping from items ' 'where composer="the composer"').fetchone()['grouping'] self.assertEqual(new_grouping, self.i.grouping) - + class RemoveTest(unittest.TestCase): def setUp(self): self.lib = lib() self.i = boracay(self.lib) def tearDown(self): self.lib.conn.close() - + remove_lib() + def test_remove_deletes_from_db(self): self.lib.remove(self.i) c = self.lib.conn.execute('select * from items where id=3') @@ -115,19 +126,19 @@ class RemoveTest(unittest.TestCase): class GetSetTest(unittest.TestCase): def setUp(self): self.i = item() - + def test_set_changes_value(self): self.i.bpm = 4915 self.assertEqual(self.i.bpm, 4915) - + def test_set_sets_dirty_flag(self): self.i.comp = not self.i.comp self.assertTrue(self.i.dirty['comp']) - + def test_set_does_not_dirty_if_value_unchanged(self): self.i.title = self.i.title self.assertTrue(not self.i.dirty['title']) - + def test_invalid_field_raises_attributeerror(self): self.assertRaises(AttributeError, getattr, self.i, 'xyzzy') @@ -137,17 +148,17 @@ class DestinationTest(unittest.TestCase): self.i = item() def tearDown(self): self.lib.conn.close() - + def test_directory_works_with_trailing_slash(self): self.lib.directory = 'one/' self.lib.path_formats = [('default', 'two')] self.assertEqual(self.lib.destination(self.i), np('one/two')) - + def test_directory_works_without_trailing_slash(self): self.lib.directory = 'one' self.lib.path_formats = [('default', 'two')] self.assertEqual(self.lib.destination(self.i), np('one/two')) - + def test_destination_substitues_metadata_values(self): self.lib.directory = 'base' self.lib.path_formats = [('default', '$album/$artist $title')] @@ -156,21 +167,21 @@ class DestinationTest(unittest.TestCase): self.i.album = 'one' self.assertEqual(self.lib.destination(self.i), np('base/one/two three')) - + def test_destination_preserves_extension(self): self.lib.directory = 'base' self.lib.path_formats = [('default', '$title')] self.i.path = 'hey.audioformat' self.assertEqual(self.lib.destination(self.i), np('base/the title.audioformat')) - + def test_lower_case_extension(self): self.lib.directory = 'base' self.lib.path_formats = [('default', '$title')] self.i.path = 'hey.MP3' self.assertEqual(self.lib.destination(self.i), np('base/the title.mp3')) - + def test_destination_pads_some_indices(self): self.lib.directory = 'base' self.lib.path_formats = [('default', '$track $tracktotal ' \ @@ -191,38 +202,38 @@ class DestinationTest(unittest.TestCase): self.i.day = 3 self.assertEqual(self.lib.destination(self.i), np('base/0001-02-03')) - + def test_destination_escapes_slashes(self): self.i.album = 'one/two' dest = self.lib.destination(self.i) self.assertTrue('one' in dest) self.assertTrue('two' in dest) self.assertFalse('one/two' in dest) - + def test_destination_escapes_leading_dot(self): self.i.album = '.something' dest = self.lib.destination(self.i) self.assertTrue('something' in dest) self.assertFalse('/.' in dest) - + def test_destination_preserves_legitimate_slashes(self): self.i.artist = 'one' self.i.album = 'two' dest = self.lib.destination(self.i) self.assertTrue(os.path.join('one', 'two') in dest) - + def test_destination_long_names_truncated(self): self.i.title = 'X'*300 self.i.artist = 'Y'*300 for c in self.lib.destination(self.i).split(os.path.sep): self.assertTrue(len(c) <= 255) - + def test_destination_long_names_keep_extension(self): self.i.title = 'X'*300 self.i.path = 'something.extn' dest = self.lib.destination(self.i) self.assertEqual(dest[-5:], '.extn') - + def test_distination_windows_removes_both_separators(self): self.i.title = 'one \\ two / three.mp3' p = self.lib.destination(self.i, ntpath) @@ -230,15 +241,15 @@ class DestinationTest(unittest.TestCase): self.assertFalse('one / two' in p) self.assertFalse('two \\ three' in p) self.assertFalse('two / three' in p) - + def test_sanitize_unix_replaces_leading_dot(self): p = util.sanitize_path(u'one/.two/three', posixpath) self.assertFalse('.' in p) - + def test_sanitize_windows_replaces_trailing_dot(self): p = util.sanitize_path(u'one/two./three', ntpath) self.assertFalse('.' in p) - + def test_sanitize_windows_replaces_illegal_chars(self): p = util.sanitize_path(u':*?"<>|', ntpath) self.assertFalse(':' in p) @@ -261,7 +272,7 @@ class DestinationTest(unittest.TestCase): self.lib.path_formats = [('default', '$album ($year)/$track $title')] dest1, dest2 = self.lib.destination(i1), self.lib.destination(i2) self.assertEqual(os.path.dirname(dest1), os.path.dirname(dest2)) - + def test_default_path_for_non_compilations(self): self.i.comp = False self.lib.add_album([self.i]) @@ -435,7 +446,7 @@ class DestinationFunctionTest(unittest.TestCase, PathFormattingMixin): def test_upper_case_literal(self): self._setf(u'%upper{foo}') self._assert_dest('/base/FOO') - + def test_upper_case_variable(self): self._setf(u'%upper{$title}') self._assert_dest('/base/THE TITLE') @@ -447,7 +458,7 @@ class DestinationFunctionTest(unittest.TestCase, PathFormattingMixin): def test_left_variable(self): self._setf(u'%left{$title, 3}') self._assert_dest('/base/the') - + def test_right_variable(self): self._setf(u'%right{$title,3}') self._assert_dest('/base/tle') @@ -455,11 +466,11 @@ class DestinationFunctionTest(unittest.TestCase, PathFormattingMixin): def test_if_false(self): self._setf(u'x%if{,foo}') self._assert_dest('/base/x') - + def test_if_true(self): self._setf(u'%if{bar,foo}') self._assert_dest('/base/foo') - + def test_if_else_false(self): self._setf(u'%if{,foo,baz}') self._assert_dest('/base/baz') @@ -484,7 +495,7 @@ class DisambiguationTest(unittest.TestCase, PathFormattingMixin): self.i2 = item() self.i2.year = 2002 self.lib.add_album([self.i2]) - self.lib.save() + self.lib.conn.commit() self._setf(u'foo%aunique{albumartist album,year}/$title') @@ -497,21 +508,21 @@ class DisambiguationTest(unittest.TestCase, PathFormattingMixin): def test_unique_with_default_arguments_uses_albumtype(self): album2 = self.lib.get_album(self.i1) album2.albumtype = 'bar' - self.lib.save() + self.lib.conn.commit() self._setf(u'foo%aunique{}/$title') self._assert_dest('/base/foo [bar]/the title', self.i1) def test_unique_expands_to_nothing_for_distinct_albums(self): album2 = self.lib.get_album(self.i2) album2.album = 'different album' - self.lib.save() + self.lib.conn.commit() self._assert_dest('/base/foo/the title', self.i1) def test_use_fallback_numbers_when_identical(self): album2 = self.lib.get_album(self.i2) album2.year = 2001 - self.lib.save() + self.lib.conn.commit() self._assert_dest('/base/foo 1/the title', self.i1) self._assert_dest('/base/foo 2/the title', self.i2) @@ -550,13 +561,13 @@ class PluginDestinationTest(unittest.TestCase): def test_undefined_value_not_substituted(self): self._assert_dest('the artist $foo') - + def test_plugin_value_not_substituted(self): self._tv_map = { 'foo': 'bar', } self._assert_dest('the artist bar') - + def test_plugin_value_overrides_attribute(self): self._tv_map = { 'artist': 'bar', @@ -568,7 +579,7 @@ class PluginDestinationTest(unittest.TestCase): 'foo': 'bar/baz', } self._assert_dest('the artist bar_baz') - + class MigrationTest(unittest.TestCase): """Tests the ability to change the database schema between versions. @@ -579,7 +590,7 @@ class MigrationTest(unittest.TestCase): self.old_fields = self.older_fields + [('field_two', 'int')] self.new_fields = self.old_fields + [('field_three', 'int')] self.newer_fields = self.new_fields + [('field_four', 'int')] - + # Set up a library with old_fields. self.libfile = os.path.join(_common.RSRC, 'templib.blb') old_lib = beets.library.Library(self.libfile, @@ -588,12 +599,12 @@ class MigrationTest(unittest.TestCase): old_lib.conn.execute( 'insert into items (field_one, field_two) values (4, 2)' ) - old_lib.save() + old_lib.conn.commit() del old_lib - + def tearDown(self): os.unlink(self.libfile) - + def test_open_with_same_fields_leaves_untouched(self): new_lib = beets.library.Library(self.libfile, item_fields=self.old_fields) @@ -601,7 +612,7 @@ class MigrationTest(unittest.TestCase): c.execute("select * from items") row = c.fetchone() self.assertEqual(len(row), len(self.old_fields)) - + def test_open_with_new_field_adds_column(self): new_lib = beets.library.Library(self.libfile, item_fields=self.new_fields) @@ -609,7 +620,7 @@ class MigrationTest(unittest.TestCase): c.execute("select * from items") row = c.fetchone() self.assertEqual(len(row), len(self.new_fields)) - + def test_open_with_fewer_fields_leaves_untouched(self): new_lib = beets.library.Library(self.libfile, item_fields=self.older_fields) @@ -617,7 +628,7 @@ class MigrationTest(unittest.TestCase): c.execute("select * from items") row = c.fetchone() self.assertEqual(len(row), len(self.old_fields)) - + def test_open_with_multiple_new_fields(self): new_lib = beets.library.Library(self.libfile, item_fields=self.newer_fields) @@ -689,13 +700,13 @@ class AlbumInfoTest(unittest.TestCase): ai.artpath = '/my/great/art' new_ai = self.lib.get_album(self.i) self.assertEqual(new_ai.artpath, '/my/great/art') - + def test_albuminfo_for_two_items_doesnt_duplicate_row(self): i2 = item() self.lib.add(i2) self.lib.get_album(self.i) self.lib.get_album(i2) - + c = self.lib.conn.cursor() c.execute('select * from albums where album=?', (self.i.album,)) # Cursor should only return one row. @@ -768,11 +779,11 @@ class ArtDestinationTest(unittest.TestCase): self.i.path = self.lib.destination(self.i) self.lib.art_filename = 'artimage' self.ai = self.lib.add_album((self.i,)) - + def test_art_filename_respects_setting(self): art = self.ai.art_destination('something.jpg') self.assert_('%sartimage.jpg' % os.path.sep in art) - + def test_art_path_in_item_dir(self): art = self.ai.art_destination('something.jpg') track = self.lib.destination(self.i) diff --git a/test/test_importer.py b/test/test_importer.py index 2eba76ce6..deff9413e 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -292,7 +292,7 @@ class ImportApplyTest(unittest.TestCase, _common.ExtraAsserts): # First, add the item to the library. temp_item = library.Item.from_path(self.srcpath) self.lib.add(temp_item) - self.lib.save() + self.lib.conn.commit() # Then, re-import the same file. coro = importer.apply_choices(_common.iconfig(self.lib)) @@ -748,7 +748,7 @@ class ArtFetchTest(unittest.TestCase, _common.ExtraAsserts): self.i = _common.item() self.i.path = itempath self.album = self.lib.add_album([self.i]) - self.lib.save() + self.lib.conn.commit() # Set up an art-fetching coroutine. self.config = _common.iconfig(self.lib) diff --git a/test/test_query.py b/test/test_query.py index bcb932cc1..1f6c2f4ae 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -296,7 +296,6 @@ class MemoryGetTest(unittest.TestCase, AssertsMixin): def test_unicode_query(self): self.single_item.title = u'caf\xe9' self.lib.store(self.single_item) - self.lib.save() q = u'title:caf\xe9' results = self.lib.items(q) @@ -410,13 +409,15 @@ class CountTest(unittest.TestCase): self.lib.add(self.item) def test_count_gets_single_item(self): - songs, totaltime = beets.library.TrueQuery().count(self.lib) + with self.lib.transaction() as tx: + songs, totaltime = beets.library.TrueQuery().count(tx) self.assertEqual(songs, 1) self.assertEqual(totaltime, self.item.length) def test_count_works_for_empty_library(self): self.lib.remove(self.item) - songs, totaltime = beets.library.TrueQuery().count(self.lib) + with self.lib.transaction() as tx: + songs, totaltime = beets.library.TrueQuery().count(tx) self.assertEqual(songs, 0) self.assertEqual(totaltime, 0.0) diff --git a/test/test_ui.py b/test/test_ui.py index 970c27e56..8ee2b1143 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -54,7 +54,7 @@ class ListTest(unittest.TestCase): def test_list_unicode_query(self): self.item.title = u'na\xefve' self.lib.store(self.item) - self.lib.save() + self.lib.conn.commit() commands.list_items(self.lib, [u'na\xefve'], False, False, None) out = self.io.getoutput() diff --git a/test/test_vfs.py b/test/test_vfs.py index c789d1842..773ede420 100644 --- a/test/test_vfs.py +++ b/test/test_vfs.py @@ -26,7 +26,6 @@ class VFSTest(unittest.TestCase): ]) self.lib.add(_common.item()) self.lib.add_album([_common.item()]) - self.lib.save() self.tree = vfs.libtree(self.lib) def test_singleton_item(self):