launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/cachedproperty into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Make invalidation of cached properties on SQLBase derived classes automatic. This addresses many of the risks associated with using them in the test suite (the server already discards all such objects between transactions), leaving just read-write-readback cases to handle as and when they occur.
-- 
https://code.launchpad.net/~lifeless/launchpad/cachedproperty/+merge/32728
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/cachedproperty into lp:launchpad/devel.

=== modified file 'lib/canonical/cachedproperty.py'
--- lib/canonical/cachedproperty.py	2010-01-11 21:29:33 +0000
+++ lib/canonical/cachedproperty.py	2010-08-16 00:06:48 +0000
@@ -3,10 +3,15 @@
 
 """Cached properties for situations where a property is computed once and
 then returned each time it is asked for.
+
+The clear_cachedproperties function can be used to wipe the cache of properties
+from an instance.
 """
 
 __metaclass__ = type
 
+__all__ = ['cachedproperty', 'clear_cachedproperties']
+
 # XXX: JonathanLange 2010-01-11 bug=505731: Move this to lp.services.
 
 def cachedproperty(attrname_or_fn):
@@ -17,6 +22,10 @@
 
     If you don't provide a name, the mangled name of the property is used.
 
+    cachedproperty is not threadsafe - it should not be used on objects which
+    are shared across threads / external locking should be used on those
+    objects.
+
     >>> class CachedPropertyTest(object):
     ...
     ...     @cachedproperty('_foo_cache')
@@ -44,6 +53,10 @@
     69
     >>> cpt._bar_cached_value
     69
+    
+    Cached properties are listed on instances.
+    >>> sorted(cpt._cached_properties)
+    ['_bar_cached_value', '_foo_cache']
 
     """
     if isinstance(attrname_or_fn, basestring):
@@ -55,7 +68,34 @@
         return CachedProperty(attrname, fn)
 
 
+def clear_cachedproperties(instance):
+    """Clear cached properties from an object.
+    
+    >>> class CachedPropertyTest(object):
+    ...
+    ...     @cachedproperty('_foo_cache')
+    ...     def foo(self):
+    ...         return 23
+    ...
+    >>> instance = CachedPropertyTest()
+    >>> instance.foo
+    23
+    >>> instance._cached_properties
+    ['_foo_cache']
+    >>> clear_cachedproperties(instance)
+    >>> instance._cached_properties
+    []
+    >>> hasattr(instance, '_foo_cache')
+    False
+    """
+    cached_properties = getattr(instance, '_cached_properties', [])
+    for property_name in cached_properties:
+        delattr(instance, property_name)
+    instance._cached_properties = []
+
+
 class CachedPropertyForAttr:
+    """Curry a decorator to provide arguments to the CachedProperty."""
 
     def __init__(self, attrname):
         self.attrname = attrname
@@ -66,18 +106,23 @@
 
 class CachedProperty:
 
+    # Used to detect not-yet-cached properties.
+    sentinel = object()
+
     def __init__(self, attrname, fn):
         self.fn = fn
         self.attrname = attrname
-        self.marker = object()
 
     def __get__(self, inst, cls=None):
         if inst is None:
             return self
-        cachedresult = getattr(inst, self.attrname, self.marker)
-        if cachedresult is self.marker:
+        cachedresult = getattr(inst, self.attrname, CachedProperty.sentinel)
+        if cachedresult is CachedProperty.sentinel:
             result = self.fn(inst)
             setattr(inst, self.attrname, result)
+            cached_properties = getattr(inst, '_cached_properties', [])
+            cached_properties.append(self.attrname)
+            inst._cached_properties = cached_properties
             return result
         else:
             return cachedresult

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2010-05-21 13:08:18 +0000
+++ lib/canonical/database/sqlbase.py	2010-08-16 00:06:48 +0000
@@ -30,6 +30,7 @@
 
 from lazr.restful.interfaces import IRepresentationCache
 
+from canonical.cachedproperty import clear_cachedproperties
 from canonical.config import config, dbconfig
 from canonical.database.interfaces import ISQLBase
 
@@ -175,6 +176,9 @@
         correct master Store.
         """
         from canonical.launchpad.interfaces import IMasterStore
+        # Make it simple to write dumb-invalidators - initialised
+        # _cached_properties to a valid list rather than just-in-time creation.
+        self._cached_properties = []
         store = IMasterStore(self.__class__)
 
         # The constructor will fail if objects from a different Store
@@ -253,6 +257,15 @@
         cache = getUtility(IRepresentationCache)
         cache.delete(self)
 
+    def __storm_invalidated__(self):
+        """Flush cached properties."""
+        # Note this is not directly tested; but the entire test suite blows up
+        # awesomely if its broken : its entirely unclear where tests for this
+        # should be -- RBC 20100816.
+        super(SQLBase, self).__storm_invalidated__()
+        clear_cachedproperties(self)
+
+
 alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
 "already installed.  This is probably caused by calling initZopeless twice.")
 

=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py	2010-07-06 16:12:46 +0000
+++ lib/canonical/launchpad/database/librarian.py	2010-08-16 00:06:48 +0000
@@ -194,6 +194,7 @@
 
     def __storm_invalidated__(self):
         """Make sure that the file is closed across transaction boundary."""
+        super(LibraryFileAlias, self).__storm_invalidated__()
         self.close()
 
 

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2010-08-06 14:49:35 +0000
+++ lib/lp/registry/model/product.py	2010-08-16 00:06:48 +0000
@@ -509,9 +509,8 @@
 
     def __storm_invalidated__(self):
         """Clear cached non-storm attributes when the transaction ends."""
+        super(Product, self).__storm_invalidated__()
         self._cached_licenses = None
-        if safe_hasattr(self, '_commercial_subscription_cached'):
-            del self._commercial_subscription_cached
 
     def _getLicenses(self):
         """Get the licenses as a tuple."""

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2010-08-06 09:32:02 +0000
+++ lib/lp/translations/model/potemplate.py	2010-08-16 00:06:48 +0000
@@ -185,6 +185,7 @@
     _uses_english_msgids = None
 
     def __storm_invalidated__(self):
+        super(POTemplate, self).__storm_invalidated__()
         self.clearPOFileCache()
         self._uses_english_msgids = None
 

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-06 06:51:37 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-16 00:06:48 +0000
@@ -103,6 +103,7 @@
     credits_message_ids = credits_message_info.keys()
 
     def __storm_invalidated__(self):
+        super(POTMsgSet, self).__storm_invalidated__()
         self._cached_singular_text = None
         self._cached_uses_english_msgids = None
 
@@ -157,6 +158,7 @@
     @property
     def uses_english_msgids(self):
         """See `IPOTMsgSet`."""
+        # TODO: convert to cachedproperty, it will be simpler.
         if self._cached_uses_english_msgids is not None:
             return self._cached_uses_english_msgids
 
@@ -181,6 +183,7 @@
     @property
     def singular_text(self):
         """See `IPOTMsgSet`."""
+        # TODO: convert to cachedproperty, it will be simpler.
         if self._cached_singular_text is not None:
             return self._cached_singular_text