From 3d712b0a6e7c3834bb5b50abcaf4fbffd802b009 Mon Sep 17 00:00:00 2001 From: Radim Vansa Date: Wed, 11 Jan 2017 14:43:05 +0100 Subject: [PATCH] HHH-11381 In nonstrict mode, putFromLoad after evict can behave incorrectly * piggybacking minor improvements for size command, too --- .../access/NonStrictAccessDelegate.java | 9 +++----- .../access/TombstoneCallInterceptor.java | 11 +++++---- .../access/VersionedCallInterceptor.java | 23 +++++++++---------- .../infinispan/functional/VersionedTest.java | 22 ++++++++++++++---- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/NonStrictAccessDelegate.java b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/NonStrictAccessDelegate.java index cf5ff9a4e7..63908b3413 100644 --- a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/NonStrictAccessDelegate.java +++ b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/NonStrictAccessDelegate.java @@ -109,12 +109,9 @@ public class NonStrictAccessDelegate implements AccessDelegate { } // we can't use putForExternalRead since the PFER flag means that entry is not wrapped into context // when it is present in the container. TombstoneCallInterceptor will deal with this. - if (!(value instanceof CacheEntry)) { - value = new VersionedEntry(value, version, txTimestamp); - } - // Apply the update locally first - if we're the backup owner, async propagation wouldn't change the value - // for the subsequent operation soon enough as it goes through primary owner - putFromLoadCache.put(key, value); + // Even if value is instanceof CacheEntry, we have to wrap it in VersionedEntry and add transaction timestamp. + // Otherwise, old eviction record wouldn't be overwritten. + putFromLoadCache.put(key, new VersionedEntry(value, version, txTimestamp)); return true; } diff --git a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/TombstoneCallInterceptor.java b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/TombstoneCallInterceptor.java index 40e86039df..8aadf4d757 100644 --- a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/TombstoneCallInterceptor.java +++ b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/TombstoneCallInterceptor.java @@ -23,6 +23,7 @@ import org.infinispan.context.Flag; import org.infinispan.context.InvocationContext; import org.infinispan.factories.annotations.Inject; import org.infinispan.factories.annotations.Start; +import org.infinispan.filter.NullValueConverter; import org.infinispan.interceptors.CallInterceptor; import org.infinispan.metadata.EmbeddedMetadata; import org.infinispan.metadata.Metadata; @@ -190,14 +191,16 @@ public class TombstoneCallInterceptor extends CallInterceptor { public Object visitSizeCommand(InvocationContext ctx, SizeCommand command) throws Throwable { Set flags = command.getFlags(); int size = 0; - Map contextEntries = ctx.getLookedUpEntries(); - AdvancedCache decoratedCache = cache.getAdvancedCache().withFlags(flags != null ? flags.toArray(new Flag[flags.size()]) : null); + AdvancedCache decoratedCache = cache.getAdvancedCache(); + if (flags != null) { + decoratedCache = decoratedCache.withFlags(flags.toArray(new Flag[flags.size()])); + } // In non-transactional caches we don't care about context CloseableIterable> iterable = decoratedCache - .filterEntries(Tombstone.EXCLUDE_TOMBSTONES); + .filterEntries(Tombstone.EXCLUDE_TOMBSTONES).converter(NullValueConverter.getInstance()); try { for (CacheEntry entry : iterable) { - if (entry.getValue() != null && size++ == Integer.MAX_VALUE) { + if (size++ == Integer.MAX_VALUE) { return Integer.MAX_VALUE; } } diff --git a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/VersionedCallInterceptor.java b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/VersionedCallInterceptor.java index 3aa2b1c0d8..91c654c999 100644 --- a/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/VersionedCallInterceptor.java +++ b/hibernate-infinispan/src/main/java/org/hibernate/cache/infinispan/access/VersionedCallInterceptor.java @@ -78,8 +78,8 @@ public class VersionedCallInterceptor extends CallInterceptor { } Object newValue = command.getValue(); - Object newVersion = null; - long newTimestamp = Long.MIN_VALUE; + Object newVersion; + long newTimestamp; Object actualNewValue = newValue; boolean isRemoval = false; if (newValue instanceof VersionedEntry) { @@ -93,8 +93,8 @@ public class VersionedCallInterceptor extends CallInterceptor { actualNewValue = ve.getValue(); } } - else if (newValue instanceof org.hibernate.cache.spi.entry.CacheEntry) { - newVersion = ((org.hibernate.cache.spi.entry.CacheEntry) newValue).getVersion(); + else { + throw new IllegalArgumentException(String.valueOf(newValue)); } if (newVersion == null) { @@ -104,24 +104,20 @@ public class VersionedCallInterceptor extends CallInterceptor { } if (oldVersion == null) { assert oldValue == null || oldTimestamp != Long.MIN_VALUE; - if (newTimestamp == Long.MIN_VALUE) { - // remove, knowing the version - setValue(e, newValue, expiringMetadata); - } - else if (newTimestamp <= oldTimestamp) { + if (newTimestamp <= oldTimestamp) { // either putFromLoad or regular update/insert - in either case this update might come // when it was evicted/region-invalidated. In both cases, with old timestamp we'll leave // the invalid value assert oldValue == null; } else { - setValue(e, newValue, defaultMetadata); + setValue(e, actualNewValue, defaultMetadata); } return null; } int compareResult = versionComparator.compare(newVersion, oldVersion); if (isRemoval && compareResult >= 0) { - setValue(e, newValue, expiringMetadata); + setValue(e, actualNewValue, expiringMetadata); } else if (compareResult > 0) { setValue(e, actualNewValue, defaultMetadata); @@ -146,7 +142,10 @@ public class VersionedCallInterceptor extends CallInterceptor { public Object visitSizeCommand(InvocationContext ctx, SizeCommand command) throws Throwable { Set flags = command.getFlags(); int size = 0; - AdvancedCache decoratedCache = cache.getAdvancedCache().withFlags(flags != null ? flags.toArray(new Flag[flags.size()]) : null); + AdvancedCache decoratedCache = cache.getAdvancedCache(); + if (flags != null) { + decoratedCache = decoratedCache.withFlags(flags.toArray(new Flag[flags.size()])); + } // In non-transactional caches we don't care about context CloseableIterable> iterable = decoratedCache .filterEntries(VersionedEntry.EXCLUDE_EMPTY_EXTRACT_VALUE).converter(NullValueConverter.getInstance()); diff --git a/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/VersionedTest.java b/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/VersionedTest.java index 648b8d5527..21c8e949e2 100644 --- a/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/VersionedTest.java +++ b/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/VersionedTest.java @@ -220,11 +220,7 @@ public class VersionedTest extends AbstractNonInvalidationTest { public void testEvictUpdateExpiration() throws Exception { // since the timestamp for update is based on session open/tx begin time, we have to do this sequentially sessionFactory().getCache().evictEntity(Item.class, itemId); - - Map contents = Caches.entrySet(entityCache).toMap(); - assertEquals(1, contents.size()); - assertEquals(VersionedEntry.class, contents.get(itemId).getClass()); - + assertSingleEmpty(); TIME_SERVICE.advance(1); withTxSession(s -> { @@ -238,6 +234,22 @@ public class VersionedTest extends AbstractNonInvalidationTest { assertSingleCacheEntry(); } + @Test + public void testEvictAndPutFromLoad() throws Exception { + sessionFactory().getCache().evictEntity(Item.class, itemId); + assertSingleEmpty(); + TIME_SERVICE.advance(1); + + withTxSession(s -> { + Item item = s.load(Item.class, itemId); + assertEquals("Original item", item.getDescription()); + }); + + assertSingleCacheEntry(); + TIME_SERVICE.advance(TIMEOUT + 1); + assertSingleCacheEntry(); + } + @Test public void testCollectionUpdate() throws Exception { // the first insert puts VersionedEntry(null, null, timestamp), so we have to wait a while to cache the entry