From dba9397a5e457598f9f4d903b3e3447adf83dea8 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Thu, 17 Oct 2013 15:25:26 +0530 Subject: [PATCH] Fix a few remaining downgrade locking errors Fix a few remaining call sites that could generate downgrade lock errors. Also speed up locking a little by aliasing the __enter__ and __exit__ methods on the wrapper lock object instead of wrapping them. --- src/calibre/db/cache.py | 30 +++++++++++++++++++----------- src/calibre/db/lazy.py | 2 +- src/calibre/db/legacy.py | 8 ++++---- src/calibre/db/locking.py | 39 +++++++++++++++++++++++++++++++-------- src/calibre/db/view.py | 4 ++-- 5 files changed, 57 insertions(+), 26 deletions(-) diff --git a/src/calibre/db/cache.py b/src/calibre/db/cache.py index 6a4ed4708e..fcb8b7c407 100644 --- a/src/calibre/db/cache.py +++ b/src/calibre/db/cache.py @@ -18,7 +18,7 @@ from calibre.constants import iswindows, preferred_encoding from calibre.customize.ui import run_plugins_on_import, run_plugins_on_postimport from calibre.db import SPOOL_SIZE, _get_next_series_num_for_list from calibre.db.categories import get_categories -from calibre.db.locking import create_locks, DowngradeLockError +from calibre.db.locking import create_locks, DowngradeLockError, SafeReadLock from calibre.db.errors import NoSuchFormat from calibre.db.fields import create_field, IDENTITY, InvalidLinkTable from calibre.db.search import Search @@ -57,11 +57,8 @@ def wrap_simple(lock, func): return func(*args, **kwargs) except DowngradeLockError: # We already have an exclusive lock, no need to acquire a shared - # lock. This can happen when updating the search cache in the - # presence of composite columns. Updating the search cache holds an - # exclusive lock, but searching a composite column involves - # reading field values via ProxyMetadata which tries to get a - # shared lock. + # lock. See the safe_read_lock properties documentation for why + # this is necessary. return func(*args, **kwargs) return call_func_with_lock @@ -118,6 +115,17 @@ class Cache(object): self._search_api = Search(self, 'saved_searches', self.field_metadata.get_search_terms()) self.initialize_dynamic() + @property + def safe_read_lock(self): + ''' A safe read lock is a lock that does nothing if the thread already + has a write lock, otherwise it acquires a read lock. This is necessary + to prevent DowngradeLockErrors, which can happen when updating the + search cache in the presence of composite columns. Updating the search + cache holds an exclusive lock, but searching a composite column + involves reading field values via ProxyMetadata which tries to get a + shared lock. There may be other scenarios that trigger this as well. ''' + return SafeReadLock(self.read_lock) + @write_api def initialize_dynamic(self): # Reconstruct the user categories, putting them into field_metadata @@ -501,7 +509,7 @@ class Cache(object): x = self.format_metadata_cache[book_id].get(fmt, None) if x is not None: return x - with self.read_lock: + with self.safe_read_lock: try: name = self.fields['formats'].format_fname(book_id, fmt) path = self._field_for('path', book_id).replace('/', os.sep) @@ -545,7 +553,7 @@ class Cache(object): cover_as_data is True then as mi.cover_data. ''' - with self.read_lock: + with self.safe_read_lock: mi = self._get_metadata(book_id, get_user_categories=get_user_categories) if get_cover: @@ -751,7 +759,7 @@ class Cache(object): ext = ('.'+fmt.lower()) if fmt else '' if as_path: if preserve_filename: - with self.read_lock: + with self.safe_read_lock: try: fname = self.fields['formats'].format_fname(book_id, fmt) except: @@ -777,7 +785,7 @@ class Cache(object): return None ret = pt.name elif as_file: - with self.read_lock: + with self.safe_read_lock: try: fname = self.fields['formats'].format_fname(book_id, fmt) except: @@ -878,7 +886,7 @@ class Cache(object): @api def get_categories(self, sort='name', book_ids=None, icon_map=None, already_fixed=None): try: - with self.read_lock: + with self.safe_read_lock: return get_categories(self, sort=sort, book_ids=book_ids, icon_map=icon_map) except InvalidLinkTable as err: bad_field = err.field_name diff --git a/src/calibre/db/lazy.py b/src/calibre/db/lazy.py index 8935110b65..fbe37713d5 100644 --- a/src/calibre/db/lazy.py +++ b/src/calibre/db/lazy.py @@ -132,7 +132,7 @@ def adata_getter(field): author_ids, adata = cache['adata'] except KeyError: db = dbref() - with db.read_lock: + with db.safe_read_lock: author_ids = db._field_ids_for('authors', book_id) adata = db._author_data(author_ids) cache['adata'] = (author_ids, adata) diff --git a/src/calibre/db/legacy.py b/src/calibre/db/legacy.py index 64205173e8..cc69ca963e 100644 --- a/src/calibre/db/legacy.py +++ b/src/calibre/db/legacy.py @@ -154,7 +154,7 @@ class LibraryDatabase(object): return tuple(self.new_api.all_book_ids()) def is_empty(self): - with self.new_api.read_lock: + with self.new_api.safe_read_lock: return not bool(self.new_api.fields['title'].table.book_col_map) def get_usage_count_by_id(self, field): @@ -363,7 +363,7 @@ class LibraryDatabase(object): def authors_with_sort_strings(self, index, index_is_id=False): book_id = index if index_is_id else self.id(index) - with self.new_api.read_lock: + with self.new_api.safe_read_lock: authors = self.new_api._field_ids_for('authors', book_id) adata = self.new_api._author_data(authors) return [(aid, adata[aid]['name'], adata[aid]['sort'], adata[aid]['link']) for aid in authors] @@ -379,7 +379,7 @@ class LibraryDatabase(object): self.notify('metadata', list(changed_books)) def book_on_device(self, book_id): - with self.new_api.read_lock: + with self.new_api.safe_read_lock: return self.new_api.fields['ondevice'].book_on_device(book_id) def book_on_device_string(self, book_id): @@ -393,7 +393,7 @@ class LibraryDatabase(object): return self.new_api.fields['ondevice'].book_on_device_func def books_in_series(self, series_id): - with self.new_api.read_lock: + with self.new_api.safe_read_lock: book_ids = self.new_api._books_for_field('series', series_id) ff = self.new_api._field_for return sorted(book_ids, key=lambda x:ff('series_index', x)) diff --git a/src/calibre/db/locking.py b/src/calibre/db/locking.py index cc006f65f5..7d9bd21712 100644 --- a/src/calibre/db/locking.py +++ b/src/calibre/db/locking.py @@ -213,16 +213,15 @@ class RWLockWrapper(object): self._shlock = shlock self._is_shared = is_shared - def __enter__(self): + def acquire(self): self._shlock.acquire(shared=self._is_shared) - return self - def __exit__(self, *args): - self.release() - - def release(self): + def release(self, *args): self._shlock.release() + __enter__ = acquire + __exit__ = release + def owns_lock(self): return self._shlock.owns_lock() @@ -231,11 +230,11 @@ class DebugRWLockWrapper(RWLockWrapper): def __init__(self, *args, **kwargs): RWLockWrapper.__init__(self, *args, **kwargs) - def __enter__(self): + def acquire(self): print ('#' * 120, file=sys.stderr) print ('acquire called: thread id:', current_thread(), 'shared:', self._is_shared, file=sys.stderr) traceback.print_stack() - RWLockWrapper.__enter__(self) + RWLockWrapper.acquire(self) print ('acquire done: thread id:', current_thread(), file=sys.stderr) print ('_' * 120, file=sys.stderr) @@ -247,4 +246,28 @@ class DebugRWLockWrapper(RWLockWrapper): print ('release done: thread id:', current_thread(), 'is_shared:', self._shlock.is_shared, 'is_exclusive:', self._shlock.is_exclusive, file=sys.stderr) print ('_' * 120, file=sys.stderr) + __enter__ = acquire + __exit__ = release +class SafeReadLock(object): + + def __init__(self, read_lock): + self.read_lock = read_lock + self.acquired = False + + def acquire(self): + try: + self.read_lock.acquire() + except DowngradeLockError: + pass + else: + self.acquired = True + return self + + def release(self, *args): + if self.acquired: + self.read_lock.release() + self.acquired = False + + __enter__ = acquire + __exit__ = release diff --git a/src/calibre/db/view.py b/src/calibre/db/view.py index 1a3d195e66..ff6b1cd1b0 100644 --- a/src/calibre/db/view.py +++ b/src/calibre/db/view.py @@ -205,7 +205,7 @@ class View(object): def get_series_sort(self, idx, index_is_id=True, default_value=''): book_id = idx if index_is_id else self.index_to_id(idx) - with self.cache.read_lock: + with self.cache.safe_read_lock: lang_map = self.cache.fields['languages'].book_value_map lang = lang_map.get(book_id, None) or None if lang: @@ -223,7 +223,7 @@ class View(object): def get_author_data(self, idx, index_is_id=True, default_value=None): id_ = idx if index_is_id else self.index_to_id(idx) - with self.cache.read_lock: + with self.cache.safe_read_lock: ids = self.cache._field_ids_for('authors', id_) adata = self.cache._author_data(ids) ans = [':::'.join((adata[aid]['name'], adata[aid]['sort'], adata[aid]['link'])) for aid in ids if aid in adata]