Merge pull request #16 from navytux/y/loadBefore-vs-gc

Fix loadBefore vs GC

/reviewed-by @mauritsvanrees @dataflake @d-maurer
/reviewed-on https://github.com/zopefoundation/tempstorage/pull/16

CHANGES.rst

@@ -4,7 +4,9 @@ Changelog
 5.2 (unreleased)
 ----------------
 
-- Nothing changed yet.
+- Package is now officially undeprecated because the data corruption issue -
+  that was the reason for its deprecation - has been understood and fixed. See
+  (`#16 <https://github.com/zopefoundation/tempstorage/issues/16>`_).
 
 
 5.1 (2019-08-15)

README.rst

@@ -5,12 +5,3 @@ A storage implementation which uses RAM to persist objects, much like
 MappingStorage. Unlike MappingStorage, it needs not be packed to get rid of
 non-cyclic garbage and it does rudimentary conflict resolution. This is a
 ripoff of Jim's Packless bsddb3 storage.
-
-**Please note: Usage of this package is deprecated, as it is known to randomly lose data, especially with Zope 4.**
-
-For a detailed discussion see `#8 <https://github.com/zopefoundation/tempstorage/issues/8>`_ as well as `#12 <https://github.com/zopefoundation/tempstorage/issues/12>`_
-
-To replace server-side sessions, cookies are probably your best bet, as these also get rid of any denial of service problems that server side sessions are vulnerable to.
-
-If you need server side storage of sessions, consider using a normal store rather than tempstorage for your session data.
-For details and suggestions see `this discussion in the pull request <https://github.com/zopefoundation/tempstorage/pull/14#issuecomment-520318459>`_ as well as the discussion in the aforementioned bug reports as well as `the discussion in Zope about the removal of the generated configuration <https://github.com/zopefoundation/Zope/pull/684>`_.

src/tempstorage/TemporaryStorage.py

@@ -19,7 +19,6 @@ resolution.
 This is a ripoff of Jim's Packless bsddb3 storage.
 """
 import bisect
-from logging import getLogger
 import warnings
 import time
 
@@ -38,8 +37,6 @@ CONFLICT_CACHE_GCEVERY = 60
 # keep history of recently gc'ed oids of length RECENTLY_GC_OIDS_LEN
 RECENTLY_GC_OIDS_LEN = 200
 
-LOG = getLogger('TemporaryStorage')
-
 
 class ReferenceCountError(POSException.POSError):
     """ Error while decrementing a reference to an object in the commit phase.
@@ -85,13 +82,6 @@ class TemporaryStorage(BaseStorage, ConflictResolvingStorage):
 
         _conflict_cache_maxage -- age at whic conflict cache items are GC'ed
         """
-        deprecation_warning = """\
-DEPRECATED: Usage of the package tempstorage is deprecated, as it is known to randomly lose data.
-Especially on Zope 4. For details see https://github.com/zopefoundation/tempstorage/issues/8
-and https://github.com/zopefoundation/tempstorage
-"""
-        LOG.warning(deprecation_warning)
-        warnings.warn(deprecation_warning, DeprecationWarning)
 
         BaseStorage.__init__(self, name)
 
@@ -124,11 +114,20 @@ and https://github.com/zopefoundation/tempstorage
     def _clear_temp(self):
         now = time.time()
         if now > (self._last_cache_gc + self._conflict_cache_gcevery):
-            temp_cc = self._conflict_cache.copy()
-            for k, v in temp_cc.items():
-                data, t = v
-                if now > (t + self._conflict_cache_maxage):
-                    del self._conflict_cache[k]
+            # build {} oid -> [](serial, data, t)
+            byoid = {}
+            for ((oid,serial), (data,t)) in self._conflict_cache.items():
+                hist = byoid.setdefault(oid, [])
+                hist.append((serial, data, t))
+
+            # gc entries but keep latest record for each oid
+            for oid, hist in byoid.items():
+                hist.sort(key=lambda _: _[0]) # by serial
+                hist = hist[:-1] # without latest record
+                for serial, data, t in hist:
+                    if now > (t + self._conflict_cache_maxage):
+                        del self._conflict_cache[(oid,serial)]
+
             self._last_cache_gc = now
         self._tmp = []
 

src/tempstorage/tests/testTemporaryStorage.py

@@ -19,6 +19,7 @@ from ZODB.tests import BasicStorage
 from ZODB.tests import Synchronization
 from ZODB.tests import ConflictResolution
 from ZODB.tests import MTStorage
+from ZODB.utils import p64, u64
 
 
 def handle_all_serials(oid, *args):
@@ -180,25 +181,50 @@ class TemporaryStorageTests(unittest.TestCase):
         storage._conflict_cache_gcevery = 1  # second
         storage._conflict_cache_maxage = 1  # second
 
-        oid = storage.new_oid()
-        self._dostore(storage, oid, data=MinPO(5))
+        # assertCacheKeys asserts that set(storage._conflict_cache.keys()) == oidrevSet
+        # storage._conflict_cache is organized as {} (oid,rev) -> (data,t) and
+        # so is used by loadBefore as data storage. It is important that latest
+        # revision of an object is not garbage-collected so that loadBefore
+        # does not loose what was last committed.
+        def assertCacheKeys(*voidrevOK):
+            oidrevOK = set(voidrevOK)
+            self.assertEqual(set(storage._conflict_cache.keys()), oidrevOK)
+            # make sure that loadBefore actually uses ._conflict_cache data
+            for (oid, rev) in voidrevOK:
+                load_data, load_serial, _ = storage.loadBefore(oid, p64(u64(rev)+1))
+                data, t = storage._conflict_cache[(oid, rev)]
+                self.assertEqual((load_data, load_serial), (data, rev))
+
+        oid1 = storage.new_oid()
+        self._dostore(storage, oid1, data=MinPO(5))
+        rev11 = storage.lastTransaction()
+        self._dostore(storage, oid1, revid=rev11, data=MinPO(7))
+        rev12 = storage.lastTransaction()
 
         time.sleep(2)
 
         oid2 = storage.new_oid()
         self._dostore(storage, oid2, data=MinPO(10))
+        rev21 = storage.lastTransaction()
 
         oid3 = storage.new_oid()
         self._dostore(storage, oid3, data=MinPO(9))
+        rev31 = storage.lastTransaction()
+
+        # (oid1, rev11) garbage-collected
+        assertCacheKeys((oid1, rev12), (oid2, rev21), (oid3, rev31))
 
-        self.assertEqual(len(storage._conflict_cache), 2)
+        self._dostore(storage, oid2, revid=rev21, data=MinPO(11))
+        rev22 = storage.lastTransaction()
 
         time.sleep(2)
 
         oid4 = storage.new_oid()
         self._dostore(storage, oid4, data=MinPO(11))
+        rev41 = storage.lastTransaction()
 
-        self.assertEqual(len(storage._conflict_cache), 1)
+        # (oid2, rev21) garbage-collected
+        assertCacheKeys((oid1, rev12), (oid2, rev22), (oid3, rev31), (oid4, rev41))
 
     def test_have_MVCC_ergo_no_ReadConflict(self):
         from ZODB.DB import DB