From 88d0d900715b10ff24dcae216e95cb6125166c66 Mon Sep 17 00:00:00 2001 From: Andreas Gabriel Date: Sun, 15 Apr 2018 22:45:32 +0200 Subject: [PATCH] Fixed inconsistent cache read of concurrent threads --- .../PluginIndexes/BooleanIndex/tests.py | 10 +- src/Products/PluginIndexes/DateIndex/tests.py | 10 +- .../PluginIndexes/DateRangeIndex/tests.py | 10 +- .../PluginIndexes/KeywordIndex/tests.py | 10 +- .../PluginIndexes/PathIndex/PathIndex.py | 6 +- src/Products/PluginIndexes/PathIndex/tests.py | 10 +- .../PluginIndexes/TopicIndex/TopicIndex.py | 6 +- .../PluginIndexes/TopicIndex/tests.py | 10 +- src/Products/PluginIndexes/UUIDIndex/tests.py | 10 +- .../PluginIndexes/tests/test_unindex.py | 13 +- src/Products/PluginIndexes/unindex.py | 6 +- src/Products/ZCatalog/cache.py | 6 +- src/Products/ZCatalog/tests/test_cache.py | 118 ++++++++++++++++-- 13 files changed, 171 insertions(+), 54 deletions(-) diff --git a/src/Products/PluginIndexes/BooleanIndex/tests.py b/src/Products/PluginIndexes/BooleanIndex/tests.py index c5208190..0a50834a 100644 --- a/src/Products/PluginIndexes/BooleanIndex/tests.py +++ b/src/Products/PluginIndexes/BooleanIndex/tests.py @@ -257,19 +257,19 @@ def test_reindexation_when_index_reversed(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy(1, True) index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/DateIndex/tests.py b/src/Products/PluginIndexes/DateIndex/tests.py index 274b8b88..9c434fe3 100644 --- a/src/Products/PluginIndexes/DateIndex/tests.py +++ b/src/Products/PluginIndexes/DateIndex/tests.py @@ -337,21 +337,21 @@ def test_getCounter(self): from DateTime import DateTime index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) index.index_object(1, Dummy('b', DateTime(0))) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) def test_precision(self): from DateTime import DateTime diff --git a/src/Products/PluginIndexes/DateRangeIndex/tests.py b/src/Products/PluginIndexes/DateRangeIndex/tests.py index d9d94be2..faf96136 100644 --- a/src/Products/PluginIndexes/DateRangeIndex/tests.py +++ b/src/Products/PluginIndexes/DateRangeIndex/tests.py @@ -284,22 +284,22 @@ def test_resultset(self): def test_getCounter(self): index = self._makeOne('work', 'start', 'stop') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) k, obj = dummies[0] index.index_object(k, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(k) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) def test_precision(self): precision = 5 diff --git a/src/Products/PluginIndexes/KeywordIndex/tests.py b/src/Products/PluginIndexes/KeywordIndex/tests.py index 44216913..898c2a8e 100644 --- a/src/Products/PluginIndexes/KeywordIndex/tests.py +++ b/src/Products/PluginIndexes/KeywordIndex/tests.py @@ -275,19 +275,19 @@ def test_value_removes(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy(['hello']) index.index_object(10, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(10) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/PathIndex/PathIndex.py b/src/Products/PluginIndexes/PathIndex/PathIndex.py index 1216542e..a64ee91e 100644 --- a/src/Products/PluginIndexes/PathIndex/PathIndex.py +++ b/src/Products/PluginIndexes/PathIndex/PathIndex.py @@ -198,7 +198,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """ See IPluggableIndex. diff --git a/src/Products/PluginIndexes/PathIndex/tests.py b/src/Products/PluginIndexes/PathIndex/tests.py index 75cb809c..f6025417 100644 --- a/src/Products/PluginIndexes/PathIndex/tests.py +++ b/src/Products/PluginIndexes/PathIndex/tests.py @@ -537,19 +537,19 @@ def test__search_w_level_0(self): def test_getCounter(self): index = self._makeOne() - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) doc = Dummy('/aa/bb') index.index_object(1, doc) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py index 21982031..29ba7a79 100644 --- a/src/Products/PluginIndexes/TopicIndex/TopicIndex.py +++ b/src/Products/PluginIndexes/TopicIndex/TopicIndex.py @@ -101,7 +101,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/PluginIndexes/TopicIndex/tests.py b/src/Products/PluginIndexes/TopicIndex/tests.py index a6dedf8b..b2a7fcfb 100644 --- a/src/Products/PluginIndexes/TopicIndex/tests.py +++ b/src/Products/PluginIndexes/TopicIndex/tests.py @@ -97,18 +97,18 @@ def test_getCounter(self): index = self.TI index._counter.set(0) - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) index.index_object(1, Obj(1, 'doc1')) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/UUIDIndex/tests.py b/src/Products/PluginIndexes/UUIDIndex/tests.py index e7dee9c5..c2089469 100644 --- a/src/Products/PluginIndexes/UUIDIndex/tests.py +++ b/src/Products/PluginIndexes/UUIDIndex/tests.py @@ -179,19 +179,19 @@ def test_non_unique(self): def test_getCounter(self): index = self._makeOne('foo') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) obj = Dummy('a') index.index_object(10, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(10) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear is a change index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) diff --git a/src/Products/PluginIndexes/tests/test_unindex.py b/src/Products/PluginIndexes/tests/test_unindex.py index cfc3a364..f88a77ae 100644 --- a/src/Products/PluginIndexes/tests/test_unindex.py +++ b/src/Products/PluginIndexes/tests/test_unindex.py @@ -25,6 +25,7 @@ def _getTargetClass(self): return UnIndex def _makeOne(self, *args, **kw): + index = self._getTargetClass()(*args, **kw) class DummyZCatalog(SimpleItem): @@ -142,9 +143,10 @@ def testQuery(record, expect=1): testQuery(record) def test_getCounter(self): + index = self._makeOne('counter') - self.assertEqual(index.getCounter(), 0) + self.assertEqual(index.getCounter(), (0, False)) class Dummy(object): id = 1 @@ -152,15 +154,16 @@ class Dummy(object): obj = Dummy() index.index_object(obj.id, obj) - self.assertEqual(index.getCounter(), 1) + self.assertEqual(index.getCounter(), (1, False)) index.unindex_object(obj.id) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # unknown id index.unindex_object(1234) - self.assertEqual(index.getCounter(), 2) + self.assertEqual(index.getCounter(), (2, False)) # clear changes the index index.clear() - self.assertEqual(index.getCounter(), 3) + self.assertEqual(index.getCounter(), (3, False)) + diff --git a/src/Products/PluginIndexes/unindex.py b/src/Products/PluginIndexes/unindex.py index 6ce6b1af..b16d8871 100644 --- a/src/Products/PluginIndexes/unindex.py +++ b/src/Products/PluginIndexes/unindex.py @@ -297,7 +297,11 @@ def _increment_counter(self): def getCounter(self): """Return a counter which is increased on index changes""" - return self._counter is not None and self._counter() or 0 + counter = self._counter is not None and self._counter() or 0 + changed = (self._counter is not None and + self._counter._p_changed or False) + + return (counter, changed) def numObjects(self): """Return the number of indexed objects.""" diff --git a/src/Products/ZCatalog/cache.py b/src/Products/ZCatalog/cache.py index f39a3995..f44988b4 100644 --- a/src/Products/ZCatalog/cache.py +++ b/src/Products/ZCatalog/cache.py @@ -63,7 +63,11 @@ def skip(name, value): if name in catalog.indexes: index = catalog.getIndex(name) if IIndexCounter.providedBy(index): - counter = index.getCounter() + counter, changed = index.getCounter() + + # cannot cache if index was updated + if changed: + return None else: # cache key invalidation cannot be supported if # any index of query cannot be tested for changes diff --git a/src/Products/ZCatalog/tests/test_cache.py b/src/Products/ZCatalog/tests/test_cache.py index 2b10685e..48ca5ec9 100644 --- a/src/Products/ZCatalog/tests/test_cache.py +++ b/src/Products/ZCatalog/tests/test_cache.py @@ -12,8 +12,12 @@ ############################################################################## import unittest +import transaction from zope.testing import cleanup +from ZODB.tests.test_storage import MinimalMemoryStorage +from ZODB import DB +from ZODB.POSException import ConflictError from Products.PluginIndexes.BooleanIndex.BooleanIndex import BooleanIndex from Products.PluginIndexes.DateRangeIndex.DateRangeIndex import DateRangeIndex @@ -31,6 +35,7 @@ class Dummy(object): def __init__(self, num): + self.id = str(num) self.num = num def big(self): @@ -74,7 +79,12 @@ class TestCatalogQueryKey(cleanup.CleanUp, unittest.TestCase): def setUp(self): cleanup.CleanUp.setUp(self) + self.zcat = self._build_zcatalog() + + def _build_zcatalog(self): + zcat = ZCatalog('catalog') + zcat.addColumn('id') zcat._catalog.addIndex('big', BooleanIndex('big')) zcat._catalog.addIndex('start', DateIndex('start')) zcat._catalog.addIndex('date', DateRangeIndex('date', 'start', 'end')) @@ -87,8 +97,8 @@ def setUp(self): obj = Dummy(i) zcat.catalog_object(obj, str(i)) - self.zcat = zcat - + return zcat + def _apply_query(self, query): cache = _get_cache() @@ -110,28 +120,28 @@ def _apply_query(self, query): rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) - def _get_cache_key(self, query=None): - catalog = self.zcat._catalog - query = catalog.make_query(query) - return CatalogCacheKey(catalog, query=query).key + def _get_cache_key(self, zcatalog, query): + catalog = zcatalog._catalog + q = catalog.make_query(query) + return CatalogCacheKey(catalog, query=q).key def test_make_key(self): query = {'big': True} expect = (('catalog',), frozenset([('big', (True,), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) query = {'start': '2013-07-01'} expect = (('catalog',), frozenset([('start', ('2013-07-01',), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) query = {'path': '/1', 'date': '2013-07-05', 'numbers': [1, 3]} expect = (('catalog',), frozenset([('date', ('2013-07-05',), self.length), ('numbers', (1, 3), self.length), ('path', ('/1',), self.length)])) - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) queries = [{'big': True, 'b_start': 0}, {'big': True, 'b_start': 0, 'b_size': 5}, @@ -142,7 +152,7 @@ def test_make_key(self): expect = (('catalog',), frozenset([('big', (True,), self.length)])) for query in queries: - self.assertEquals(self._get_cache_key(query), expect) + self.assertEquals(self._get_cache_key(self.zcat, query), expect) def test_cache(self): query = {'big': True} @@ -179,3 +189,91 @@ def test_cache_invalidate(self): rset1 = list(map(lambda x: x.getRID(), res1)) rset2 = list(map(lambda x: x.getRID(), res2)) self.assertEqual(rset1, rset2) + + def test_cache_mvcc_aware(self): + st = MinimalMemoryStorage() + db = DB(st) + + query = {'big': True} + cache = _get_cache() + cache.invalidateAll() + + # init catalog + cn = db.open() + r = cn.root() + r['zcat'] = self._build_zcatalog() + obj = Dummy(20) + r['zcat'].catalog_object(obj, obj.id) + transaction.get().commit() + + # 1st thread + tm1 = transaction.TransactionManager() + cn1 = db.open(transaction_manager=tm1) + + # 2nd thread + tm2 = transaction.TransactionManager() + cn2 = db.open(transaction_manager=tm2) + + r1 = cn1.root() + # catalog new object + obj = Dummy(21) + r1['zcat'].catalog_object(obj, obj.id) + + # dont cache if catalog has changed + self.assertEqual(self._get_cache_key(r1['zcat'], query), None) + tm1.get().commit() + + r2 = cn2.root() + # catalog new object + obj = Dummy(22) + r2['zcat'].catalog_object(obj, obj.id) + + # dont cache if catalog has changed + self.assertEqual(self._get_cache_key(r2['zcat'], query), None) + + res2 = r2['zcat'].search(query) + indexed_ids = {rec.id for rec in res2} + self.assertTrue(obj.id in indexed_ids) + + # raise conflict error because catalog was changed in tm1 + self.assertRaises(ConflictError, tm2.get().commit) + + tm2.get().abort() + + # try it again + r2 = cn2.root() + obj = Dummy(22) + r2['zcat'].catalog_object(obj, obj.id) + + res2 = r2['zcat'].search(query) + indexed_ids = {rec.id for rec in res2} + self.assertTrue(obj.id in indexed_ids) + + tm2.get().commit() + + cn1.sync() + # query without changing catalog + r1 = cn1.root() + qkey = self._get_cache_key(r1['zcat'], query) + + # query key is not None, results will be cached + self.assertEqual(qkey, (('catalog',), + frozenset({('big', (True,), 12)}))) + + res1 = r1['zcat'].search(query) + tm1.get().commit() + + r2 = cn2.root() + res2 = r2['zcat'].search(query) + tm2.get().commit() + + # compare result + rset1 = list(map(lambda x: x.getRID(), res1)) + rset2 = list(map(lambda x: x.getRID(), res2)) + self.assertEqual(rset1, rset2) + + stats = cache.getStatistics() + hits = stats[0]['hits'] + misses = stats[0]['misses'] + self.assertEqual((hits, misses), (1, 1)) +