Add Disk.with_options(on_error='miss') so unreadable entries are deleted and treated as missing instead of raising

CHANGELOG.md

@@ -17,6 +17,7 @@ diskcache-community forked from DiskCache at upstream commit ebfa37c (3 March 20
 
 ### Features
 
+- `Disk.with_options(on_error='miss')` returns a disk type, on any disk class including `SafeDisk` and the classes `with_types` returns, whose caches delete an entry they find but cannot read back (a corrupt or stale pickle, a refused type, a path outside the cache, an undecodable text file) and treat it as missing: `get` returns its default, `cache[key]` raises `KeyError`, `pop`, `pull`, `peek` and `peekitem` move on, and iteration skips unreadable keys. `Index.items()` and `values()` skip such entries too. The first such entry per cache is logged as a warning on the `diskcache_community` logger; a read-only cache does not delete it, and its `peek` and `peekitem` raise. A missing value file, `MemoryError` and `RecursionError` are never treated as unreadable. The default, `on_error='raise'`, keeps raising as before, and the option is a class attribute that is never persisted (PR #25).
 - `configure(disk)` sets the disk type of every `Cache`, `FanoutCache`, `Deque`, `Index` and `DjangoCache` created afterwards without a disk, and `default_disk()` returns the type in effect. Without `configure`, the environment decides: `DISKCACHE_DISK=safe` selects `SafeDisk`, with `DISKCACHE_SAFE_TYPES` and `DISKCACHE_SAFE_MODULES` passed to `SafeDisk.with_types`, and `DISKCACHE_DISK` also takes `default` or the import path of a disk class. A disk passed explicitly always wins, so libraries that choose their own disk keep it; caches that already exist keep theirs, and nothing is persisted. The `disk` arguments now default to `None`, meaning that type (PR #24).
 
 ### Packaging

diskcache_community/core.py

@@ -13,6 +13,7 @@
 import inspect
 import io
 import json
+import logging
 import os
 import os.path as op
 import pathlib
@@ -194,6 +195,8 @@ def _makedirs_private(directory):
 class Disk:
     """Cache key and value serialization for SQLite database and files."""
 
+    on_error = 'raise'
+
     def __init__(self, directory, min_file_size=0, pickle_protocol=0):
         """Initialize disk instance.
 
@@ -206,6 +209,40 @@ def __init__(self, directory, min_file_size=0, pickle_protocol=0):
         self.min_file_size = min_file_size
         self.pickle_protocol = pickle_protocol
 
+    @classmethod
+    def with_options(cls, *, on_error):
+        """Return a subclass of this disk type with the given options.
+
+        `on_error` says what a cache does with an entry that it finds but
+        cannot read back: a pickle that is corrupt, refers to a class that no
+        longer exists, or is refused by :class:`SafeDisk`, or a value file
+        that is not valid text. ``'raise'``, the default, raises the error.
+        ``'miss'`` deletes the entry and treats it as missing, so ``get``
+        returns its default and ``cache[key]`` raises :exc:`KeyError`; the
+        first such entry of each cache is logged as a warning on the
+        ``diskcache_community`` logger. A read-only cache returns the default
+        from ``get`` without deleting the entry, while its ``peek`` and
+        ``peekitem`` raise, since they cannot move past it. ``len`` and
+        ``in`` count an unreadable entry until a read deletes it. A missing
+        value file, :exc:`MemoryError` and :exc:`RecursionError` are never
+        treated as unreadable entries. The option is a class attribute and
+        is never persisted.
+
+        >>> MissDisk = Disk.with_options(on_error='miss')
+        >>> MissDisk.on_error
+        'miss'
+
+        :param str on_error: ``'raise'`` or ``'miss'``
+        :return: subclass to pass as ``disk=``
+        :raises ValueError: if `on_error` is not one of the two
+
+        """
+        if on_error not in ('raise', 'miss'):
+            raise ValueError(
+                "on_error must be 'raise' or 'miss', not %r" % (on_error,)
+            )
+        return _derive_disk(cls, {'on_error': on_error})
+
     def hash(self, key):
         """Compute portable hash for `key`.
 
@@ -990,6 +1027,8 @@ def remove(self, file_path):
         super().remove(file_path)
 
 
+_LOGGER = logging.getLogger('diskcache_community')
+
 ENV_DISK = 'DISKCACHE_DISK'
 ENV_SAFE_TYPES = 'DISKCACHE_SAFE_TYPES'
 ENV_SAFE_MODULES = 'DISKCACHE_SAFE_MODULES'
@@ -1425,6 +1464,13 @@ def after_fork_child(self):
 # Every live Cache, so a child process can forget inherited connections.
 _CACHES: 'weakref.WeakSet[Cache]' = weakref.WeakSet()
 
+# Caches that have logged an unreadable entry as a warning already.
+_UNREADABLE_LOGGED: 'weakref.WeakSet[Cache]' = weakref.WeakSet()
+
+# Errors that on_error='miss' never treats as an unreadable entry: a missing
+# value file is already a miss, and the others say nothing about the entry.
+_NOT_UNREADABLE = (OSError, MemoryError, RecursionError)
+
 # Connections inherited from a parent process, kept open on purpose.
 _INHERITED_CONNECTIONS: list[sqlite3.Connection] = []
 
@@ -2331,6 +2377,66 @@ def decr(self, key, delta=1, default=0, retry=False):
         """
         return self.incr(key, -delta, default, retry)
 
+    def _unreadable(self, error):
+        """Return True when `error`, raised reading an entry, means a miss.
+
+        That is when the disk's `on_error` is ``'miss'``. The first such
+        entry of the cache is logged as a warning and later ones at debug
+        level.
+
+        """
+        if getattr(self._disk, 'on_error', 'raise') != 'miss' or isinstance(
+            error, _NOT_UNREADABLE
+        ):
+            return False
+
+        logged = self in _UNREADABLE_LOGGED
+        _UNREADABLE_LOGGED.add(self)
+        _LOGGER.log(
+            logging.DEBUG if logged else logging.WARNING,
+            'treating an unreadable entry in %r as missing: %s: %s',
+            self._directory,
+            type(error).__name__,
+            error,
+        )
+        return True
+
+    def _remove_unreadable_file(self, filename):
+        """Remove the value file of an unreadable entry.
+
+        A disk may refuse to remove the path, as SafeDisk does for a path
+        outside the cache directory, and that path is then left alone.
+
+        """
+        if filename is not None:
+            with cl.suppress(OSError, ValueError):
+                self._disk.remove(filename)
+
+    def _drop_unreadable(self, rowid, mode, filename, db_value, retry):
+        """Delete the unreadable entry at `rowid` unless it has changed."""
+        if self._read_only:
+            return
+
+        with self._transact(retry) as (sql, _):
+            cursor = sql(
+                'DELETE FROM Cache WHERE rowid = ? AND mode = ?'
+                ' AND filename IS ? AND value IS ?',
+                (rowid, mode, filename, db_value),
+            )
+            deleted = cursor.rowcount
+
+        if deleted:
+            self._remove_unreadable_file(filename)
+
+    def _read_key(self, key, raw):
+        """Return the key of a row, or ENOVAL for one read as a miss."""
+        try:
+            return self._disk.get(key, raw)
+        except Exception as error:  # pylint: disable=broad-except
+            if not self._unreadable(error):
+                raise
+            return ENOVAL
+
     def get(
         self,
         key,
@@ -2407,6 +2513,11 @@ def get(
             except IOError:
                 # Key was deleted before we could retrieve result.
                 return default
+            except Exception as error:  # pylint: disable=broad-except
+                if not self._unreadable(error):
+                    raise
+                self._drop_unreadable(rowid, mode, filename, db_value, retry)
+                return default
 
         else:  # Slow path, transaction required.
             cache_hit = (
@@ -2443,6 +2554,15 @@ def get(
                     if statistics:
                         sql(cache_miss)
                     return default
+                except Exception as error:  # pylint: disable=broad-except
+                    if not self._unreadable(error):
+                        raise
+                    self._drop_unreadable(
+                        rowid, mode, filename, db_value, retry
+                    )
+                    if statistics:
+                        sql(cache_miss)
+                    return default
 
                 if statistics:
                     sql(cache_hit)
@@ -2556,6 +2676,12 @@ def pop(
         except IOError:
             # Key was deleted before we could retrieve result.
             return default
+        except Exception as error:  # pylint: disable=broad-except
+            if not self._unreadable(error):
+                raise
+            self._remove_unreadable_file(filename)
+            filename = None
+            return default
         finally:
             if filename is not None:
                 self._disk.remove(filename)
@@ -2828,6 +2954,12 @@ def pull(
             except IOError:
                 # Key was deleted before we could retrieve result.
                 continue
+            except Exception as error:  # pylint: disable=broad-except
+                if not self._unreadable(error):
+                    raise
+                self._remove_unreadable_file(name)
+                name = None
+                continue
             finally:
                 if name is not None:
                     self._disk.remove(name)
@@ -2945,6 +3077,11 @@ def peek(
             except IOError:
                 # Key was deleted before we could retrieve result.
                 continue
+            except Exception as error:  # pylint: disable=broad-except
+                if self._read_only or not self._unreadable(error):
+                    raise
+                self._drop_unreadable(rowid, mode, name, db_value, retry)
+                continue
             break
 
         if expire_time and tag:
@@ -3019,13 +3156,17 @@ def peekitem(self, last=True, expire_time=False, tag=False, retry=False):
                     else:
                         break
 
-            key = self._disk.get(db_key, raw)
-
             try:
+                key = self._disk.get(db_key, raw)
                 value = self._disk.fetch(mode, name, db_value, False)
             except IOError:
                 # Key was deleted before we could retrieve result.
                 continue
+            except Exception as error:  # pylint: disable=broad-except
+                if self._read_only or not self._unreadable(error):
+                    raise
+                self._drop_unreadable(rowid, mode, name, db_value, retry)
+                continue
             break
 
         if expire_time and tag:
@@ -3485,7 +3626,7 @@ def iterkeys(self, reverse=False, tag=ENOVAL):
         """
         sql = self._sql
         limit = 100
-        _disk_get = self._disk.get
+        _read_key = self._read_key
         select = 'SELECT key, raw FROM Cache'
         args = ()
 
@@ -3513,7 +3654,9 @@ def iterkeys(self, reverse=False, tag=ENOVAL):
         else:
             return
 
-        yield _disk_get(key, raw)
+        result = _read_key(key, raw)
+        if result is not ENOVAL:
+            yield result
 
         while True:
             rows = sql(iterate, args + (key, raw, key, limit)).fetchall()
@@ -3522,7 +3665,9 @@ def iterkeys(self, reverse=False, tag=ENOVAL):
                 break
 
             for key, raw in rows:
-                yield _disk_get(key, raw)
+                result = _read_key(key, raw)
+                if result is not ENOVAL:
+                    yield result
 
     def _iter(self, ascending=True):
         sql = self._sql
@@ -3535,7 +3680,7 @@ def _iter(self, ascending=True):
 
         bound = max_rowid + 1
         limit = 100
-        _disk_get = self._disk.get
+        _read_key = self._read_key
         rowid = 0 if ascending else bound
         select = (
             'SELECT rowid, key, raw FROM Cache'
@@ -3555,7 +3700,9 @@ def _iter(self, ascending=True):
                 break
 
             for rowid, key, raw in rows:
-                yield _disk_get(key, raw)
+                result = _read_key(key, raw)
+                if result is not ENOVAL:
+                    yield result
 
     def __iter__(self):
         """Iterate keys in cache including expired items."""

diskcache_community/persistent.py

@@ -16,6 +16,43 @@
 from .core import ENOVAL, Cache
 
 
+class _IndexValuesView(ValuesView):
+    """Values view of an Index that skips items read as missing.
+
+    An item can disappear between listing its key and reading its value:
+    another process deleted it, or the disk treats an unreadable value as
+    missing.
+
+    """
+
+    def __iter__(self):
+        _cache = self._mapping._cache  # pylint: disable=protected-access
+        for key in self._mapping:
+            value = _cache.get(key, ENOVAL, retry=True)
+            if value is not ENOVAL:
+                yield value
+
+    def __contains__(self, value):
+        return any(item is value or item == value for item in self)
+
+
+class _IndexItemsView(ItemsView):
+    """Items view of an Index that skips items read as missing."""
+
+    def __iter__(self):
+        _cache = self._mapping._cache  # pylint: disable=protected-access
+        for key in self._mapping:
+            value = _cache.get(key, ENOVAL, retry=True)
+            if value is not ENOVAL:
+                yield key, value
+
+    def __contains__(self, item):
+        key, value = item
+        _cache = self._mapping._cache  # pylint: disable=protected-access
+        stored = _cache.get(key, ENOVAL, retry=True)
+        return stored is not ENOVAL and (stored is value or stored == value)
+
+
 def _make_compare(seq_op, doc):
     """Make compare method with Sequence semantics."""
 
@@ -1082,7 +1119,7 @@ def values(self):
         :return: values view
 
         """
-        return ValuesView(self)
+        return _IndexValuesView(self)
 
     def items(self):
         """Set-like object providing a view of index items.
@@ -1096,7 +1133,7 @@ def items(self):
         :return: items view
 
         """
-        return ItemsView(self)
+        return _IndexItemsView(self)
 
     __hash__ = None  # type: ignore
 

docs/tutorial.rst

@@ -1052,6 +1052,41 @@ wins, then :func:`configure <diskcache_community.configure>`, then the
 environment, then :class:`Disk <diskcache_community.Disk>`. Caches that already
 exist keep their disk, and the choice is never stored in the cache directory.
 
+.. _tutorial-unreadable-entries:
+
+Unreadable entries
+..................
+
+An entry can exist and still not be readable: its pickle is corrupt, refers to
+a class that a library upgrade removed, or is refused by :class:`SafeDisk
+<diskcache_community.SafeDisk>`. By default reading it raises that error.
+:meth:`Disk.with_options <diskcache_community.Disk.with_options>` returns a
+disk type that deletes such an entry and treats it as missing instead, which
+suits a cache whose entries can always be computed again:
+
+.. code-block:: python
+
+    from diskcache_community import Cache, SafeDisk
+
+    AppDisk = SafeDisk.with_types(Point).with_options(on_error='miss')
+    cache = Cache('/tmp/mycache', disk=AppDisk)
+
+``get`` then returns its default, ``cache[key]`` raises :exc:`KeyError`,
+``pop``, ``pull``, ``peek`` and ``peekitem`` move on to the next item, and
+iteration, including the items and values of an :class:`Index
+<diskcache_community.Index>`, skips entries that cannot be read. ``len`` and
+``in`` still count an unreadable entry until a read deletes it. The first
+unreadable entry of each cache is logged as a warning on the
+``diskcache_community`` logger. A read-only cache returns the default from
+``get`` without deleting the entry, and its ``peek`` and ``peekitem`` raise,
+since they cannot move past it. A value file that has disappeared is a miss
+in either mode, as before.
+
+The entry is deleted for every process that shares the directory, so give
+every reader and writer of a shared cache the same disk: a reader whose
+allowlist is narrower than a writer's would delete entries the writer can
+still read.
+
 .. _tutorial-read-only:
 
 Read-Only Caches