From 312e44dc3b54d242321e6dd840f916e99da1d9d8 Mon Sep 17 00:00:00 2001 From: Charles Haley Date: Wed, 5 May 2021 18:59:45 +0100 Subject: [PATCH] Ensure that updating the vl_book cache is both thread safe and recursion safe. --- src/calibre/db/cache.py | 53 ++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/src/calibre/db/cache.py b/src/calibre/db/cache.py index e7aac6c04b..e4f1f86051 100644 --- a/src/calibre/db/cache.py +++ b/src/calibre/db/cache.py @@ -15,6 +15,7 @@ import traceback from collections import MutableSet, Set, defaultdict from functools import partial, wraps from io import BytesIO +from threading import Lock from time import time from calibre import as_unicode, isbytestring @@ -142,7 +143,7 @@ class Cache(object): self.formatter_template_cache = {} self.dirtied_cache = {} self.vls_for_books_cache = None - self.vls_for_books_cache_is_loading = False + self.vls_cache_lock = Lock() self.dirtied_sequence = 0 self.cover_caches = set() self.clear_search_cache_count = 0 @@ -252,7 +253,6 @@ class Cache(object): self.clear_search_cache_count += 1 self._search_api.update_or_clear(self, book_ids) self.vls_for_books_cache = None - self.vls_for_books_cache_is_loading = False @read_api def last_modified(self): @@ -2232,26 +2232,35 @@ class Cache(object): if self.vls_for_books_cache is None: # Using a list is slightly faster than a set. c = defaultdict(list) - if self.vls_for_books_cache_is_loading: - # We get here if resolving the books in a VL triggers another VL - # calculation. This can be 'real' recursion, in which case the - # eventual answer will be wrong. It can also be a search using - # a location of 'all' that causes evaluation of a composite that - # references virtual libraries. If the composite isn't used in a - # VL then the eventual answer will be correct because get_metadata - # will clear the caches. - return c - self.vls_for_books_cache_is_loading = True - libraries = self._pref('virtual_libraries', {}) - for lib, expr in libraries.items(): - book = None - try: - for book in self._search(expr, virtual_fields=virtual_fields): - c[book].append(lib) - except Exception as e: - if book: - c[book].append(_('[Error in Virtual library {0}: {1}]').format(lib, str(e))) - self.vls_for_books_cache = {b:tuple(sorted(libs, key=sort_key)) for b, libs in c.items()} + got_lock = False + try: + # use a primitive lock to ensure that only one thread is updating + # the cache and that recursive calls don't do the update. This + # method can recurse via self._search() + got_lock = self.vls_cache_lock.acquire(blocking=False) + if not got_lock: + # We get here if resolving the books in a VL triggers another VL + # calculation. This can be 'real' recursion, in which case the + # eventual answer will be wrong. It can also be a search using + # a location of 'all' that causes evaluation of a composite that + # references virtual_libraries(). If the composite isn't used in a + # VL then the eventual answer will be correct because get_metadata + # will clear the caches. + return c + self.vls_for_books_cache_is_loading = True + libraries = self._pref('virtual_libraries', {}) + for lib, expr in libraries.items(): + book = None + try: + for book in self._search(expr, virtual_fields=virtual_fields): + c[book].append(lib) + except Exception as e: + if book: + c[book].append(_('[Error in Virtual library {0}: {1}]').format(lib, str(e))) + self.vls_for_books_cache = {b:tuple(sorted(libs, key=sort_key)) for b, libs in c.items()} + finally: + if got_lock: + self.vls_cache_lock.release() if not book_ids: book_ids = self._all_book_ids() # book_ids is usually 1 long. The loop will be faster than a comprehension