From 32c9a7afde5c796cb5c024464e64ff3abfd1204d Mon Sep 17 00:00:00 2001 From: Kirill Smelkov Date: Wed, 29 Jul 2020 17:13:23 +0300 Subject: [PATCH 1/2] pack: Clear all non-current entries after pack Else those non-current entries can be used to serve a loadBefore request with data, while, after pack that loadBefore request must return "data deleted" if requested object has current revision >= packtime. Fixes checkPackVSConnectionGet from ZODB from https://github.com/zopefoundation/ZODB/pull/322 which, without this patch fails as e.g. Failure in test checkPackVSConnectionGet (ZEO.tests.testZEO.MappingStorageTests) Traceback (most recent call last): File "/usr/lib/python2.7/unittest/case.py", line 329, in run testMethod() File "/home/kirr/src/wendelin/z/ZODB/src/ZODB/tests/PackableStorage.py", line 636, in checkPackVSConnectionGet raises(ReadConflictError, conn1.get, oid) File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises callableObj(*args, **kwargs) File "/usr/lib/python2.7/unittest/case.py", line 116, in __exit__ "{0} not raised".format(exc_name)) AssertionError: ReadConflictError not raised --- src/ZEO/ClientStorage.py | 11 ++++++++++- src/ZEO/cache.py | 15 +++++++++++++++ src/ZEO/tests/test_cache.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/ZEO/ClientStorage.py b/src/ZEO/ClientStorage.py index 14f18f650..014676936 100644 --- a/src/ZEO/ClientStorage.py +++ b/src/ZEO/ClientStorage.py @@ -559,7 +559,16 @@ def pack(self, t=None, referencesf=None, wait=1, days=0): if t is None: t = time.time() t = t - (days * 86400) - return self._call('pack', t, wait) + ret = self._call('pack', t, wait) + # remove all non-current entries from the cache. + # This way we make sure that loadBefore with before < packtime, won't + # return data from the cache, instead of returning "no data" if requested object + # has current revision >= packtime. + # By clearing all noncurrent entries we might remove more data from the + # cache than is strictly necessary, but since access to noncurrent data + # is seldom, that should not cause problems in practice. + self._cache.clearAllNonCurrent() + return ret def store(self, oid, serial, data, version, txn): """Storage API: store data for an object.""" diff --git a/src/ZEO/cache.py b/src/ZEO/cache.py index 553a8c24d..71d68f323 100644 --- a/src/ZEO/cache.py +++ b/src/ZEO/cache.py @@ -246,6 +246,21 @@ def clear(self): self.f.truncate() self._initfile(ZEC_HEADER_SIZE) + # clearAllNonCurrent removes all non-current entries from the cache. + def clearAllNonCurrent(self): + with self._lock: + f = self.f + for oid, tidofs in self.noncurrent.items(): + for (tid, ofs) in tidofs.items(): + f.seek(ofs) + status = f.read(1) + assert status == b'a', (ofs, f.tell(), oid, tid) + f.seek(ofs) + f.write(b'f') + self._len -= 1 + + self.noncurrent.clear() + ## # Scan the current contents of the cache file, calling `install` # for each object found in the cache. This method should only diff --git a/src/ZEO/tests/test_cache.py b/src/ZEO/tests/test_cache.py index 922f214b3..8b06eb50b 100644 --- a/src/ZEO/tests/test_cache.py +++ b/src/ZEO/tests/test_cache.py @@ -253,6 +253,34 @@ def test_clear_zeo_cache(self): self.assertEqual(cache.load(n3), None) self.assertEqual(cache.loadBefore(n3, n2), None) + def testClearAllNonCurrent(self): + cache = self.cache + cache.store(p64(1), n1, n2, b'1@1') + cache.store(p64(1), n2, n3, b'1@2') + cache.store(p64(1), n3, None, b'1') + cache.store(p64(2), n2, n3, b'2@2') + cache.store(p64(2), n3, None, b'2') + + eq = self.assertEqual + eq(len(cache), 5) + eq(cache.load(p64(1)), (b'1', n3)) + eq(cache.loadBefore(p64(1), n3), (b'1@2', n2, n3)) + eq(cache.loadBefore(p64(1), n2), (b'1@1', n1, n2)) + eq(cache.loadBefore(p64(1), n1), None) + eq(cache.load(p64(2)), (b'2', n3)) + eq(cache.loadBefore(p64(2), n3), (b'2@2', n2, n3)) + eq(cache.loadBefore(p64(2), n2), None) + + cache.clearAllNonCurrent() + eq(len(cache), 2) + eq(cache.load(p64(1)), (b'1', n3)) + eq(cache.loadBefore(p64(1), n3), None) + eq(cache.loadBefore(p64(1), n2), None) + eq(cache.loadBefore(p64(1), n1), None) + eq(cache.load(p64(2)), (b'2', n3)) + eq(cache.loadBefore(p64(2), n3), None) + eq(cache.loadBefore(p64(2), n2), None) + def testChangingCacheSize(self): # start with a small cache data = b'x' From 69c5609d4b7943daea4b75d341dd6ae36a20c0c7 Mon Sep 17 00:00:00 2001 From: Kirill Smelkov Date: Fri, 31 Jul 2020 15:53:56 +0300 Subject: [PATCH 2/2] fixup! pack: Clear all non-current entries after pack Take review feedback by @jamadden into account - ret -> result - use () for tuples uniformly - no vertical alignment --- src/ZEO/ClientStorage.py | 4 ++-- src/ZEO/cache.py | 2 +- src/ZEO/tests/test_cache.py | 34 +++++++++++++++++----------------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/ZEO/ClientStorage.py b/src/ZEO/ClientStorage.py index 014676936..de032dcc2 100644 --- a/src/ZEO/ClientStorage.py +++ b/src/ZEO/ClientStorage.py @@ -559,7 +559,7 @@ def pack(self, t=None, referencesf=None, wait=1, days=0): if t is None: t = time.time() t = t - (days * 86400) - ret = self._call('pack', t, wait) + result = self._call('pack', t, wait) # remove all non-current entries from the cache. # This way we make sure that loadBefore with before < packtime, won't # return data from the cache, instead of returning "no data" if requested object @@ -568,7 +568,7 @@ def pack(self, t=None, referencesf=None, wait=1, days=0): # cache than is strictly necessary, but since access to noncurrent data # is seldom, that should not cause problems in practice. self._cache.clearAllNonCurrent() - return ret + return result def store(self, oid, serial, data, version, txn): """Storage API: store data for an object.""" diff --git a/src/ZEO/cache.py b/src/ZEO/cache.py index 71d68f323..4861a20af 100644 --- a/src/ZEO/cache.py +++ b/src/ZEO/cache.py @@ -250,7 +250,7 @@ def clear(self): def clearAllNonCurrent(self): with self._lock: f = self.f - for oid, tidofs in self.noncurrent.items(): + for (oid, tidofs) in self.noncurrent.items(): for (tid, ofs) in tidofs.items(): f.seek(ofs) status = f.read(1) diff --git a/src/ZEO/tests/test_cache.py b/src/ZEO/tests/test_cache.py index 8b06eb50b..ec1338daa 100644 --- a/src/ZEO/tests/test_cache.py +++ b/src/ZEO/tests/test_cache.py @@ -255,31 +255,31 @@ def test_clear_zeo_cache(self): def testClearAllNonCurrent(self): cache = self.cache - cache.store(p64(1), n1, n2, b'1@1') - cache.store(p64(1), n2, n3, b'1@2') + cache.store(p64(1), n1, n2, b'1@1') + cache.store(p64(1), n2, n3, b'1@2') cache.store(p64(1), n3, None, b'1') - cache.store(p64(2), n2, n3, b'2@2') + cache.store(p64(2), n2, n3, b'2@2') cache.store(p64(2), n3, None, b'2') eq = self.assertEqual eq(len(cache), 5) - eq(cache.load(p64(1)), (b'1', n3)) - eq(cache.loadBefore(p64(1), n3), (b'1@2', n2, n3)) - eq(cache.loadBefore(p64(1), n2), (b'1@1', n1, n2)) - eq(cache.loadBefore(p64(1), n1), None) - eq(cache.load(p64(2)), (b'2', n3)) - eq(cache.loadBefore(p64(2), n3), (b'2@2', n2, n3)) - eq(cache.loadBefore(p64(2), n2), None) + eq(cache.load(p64(1)), (b'1', n3)) + eq(cache.loadBefore(p64(1), n3), (b'1@2', n2, n3)) + eq(cache.loadBefore(p64(1), n2), (b'1@1', n1, n2)) + eq(cache.loadBefore(p64(1), n1), None) + eq(cache.load(p64(2)), (b'2', n3)) + eq(cache.loadBefore(p64(2), n3), (b'2@2', n2, n3)) + eq(cache.loadBefore(p64(2), n2), None) cache.clearAllNonCurrent() eq(len(cache), 2) - eq(cache.load(p64(1)), (b'1', n3)) - eq(cache.loadBefore(p64(1), n3), None) - eq(cache.loadBefore(p64(1), n2), None) - eq(cache.loadBefore(p64(1), n1), None) - eq(cache.load(p64(2)), (b'2', n3)) - eq(cache.loadBefore(p64(2), n3), None) - eq(cache.loadBefore(p64(2), n2), None) + eq(cache.load(p64(1)), (b'1', n3)) + eq(cache.loadBefore(p64(1), n3), None) + eq(cache.loadBefore(p64(1), n2), None) + eq(cache.loadBefore(p64(1), n1), None) + eq(cache.load(p64(2)), (b'2', n3)) + eq(cache.loadBefore(p64(2), n3), None) + eq(cache.loadBefore(p64(2), n2), None) def testChangingCacheSize(self): # start with a small cache