Author: manik.surtani(a)jboss.com Date: 2009-03-18 10:45:39 -0400 (Wed, 18 Mar 2009) New Revision: 7911 Added: core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntryForRemoval.java Modified: core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntry.java core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java core/branches/flat/src/test/java/org/horizon/api/mvcc/repeatable_read/RepeatableReadLockTest.java Log: Fixed bug in repeatable read when removing a null entry Modified: core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntry.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntry.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntry.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -28,31 +28,16 @@ * @author Manik Surtani (<a href="mailto:manik@jboss.org">manik@jboss.org</a>) * @since 1.0 */ -public class NullMarkerEntry extends ReadCommittedEntry { - /** - * @return always returns true - */ - @Override - public boolean isNullEntry() { - return true; - } +public class NullMarkerEntry extends NullMarkerEntryForRemoval { - /** - * @return always returns true so that any get commands, upon getting this entry, will ignore the entry as though it - * were removed. - */ - @Override - public boolean isDeleted() { - return true; + private static final NullMarkerEntry INSTANCE = new NullMarkerEntry(); + + private NullMarkerEntry() { + super(null); } - /** - * @return always returns true so that any get commands, upon getting this entry, will ignore the entry as though it - * were invalid. - */ - @Override - public boolean isValid() { - return false; + public static NullMarkerEntry getInstance() { + return INSTANCE; } /** Added: core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntryForRemoval.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntryForRemoval.java (rev 0) +++ core/branches/flat/src/main/java/org/horizon/container/NullMarkerEntryForRemoval.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -0,0 +1,41 @@ +package org.horizon.container; + +/** + * A null entry that is read in for removal + * + * @author Manik Surtani + * @since 1.0 + */ +public class NullMarkerEntryForRemoval extends RepeatableReadEntry { + + public NullMarkerEntryForRemoval(Object key) { + super(key, null, -1); + } + + /** + * @return always returns true + */ + @Override + public boolean isNullEntry() { + return true; + } + + /** + * @return always returns true so that any get commands, upon getting this entry, will ignore the entry as though it + * were removed. + */ + @Override + public boolean isDeleted() { + return true; + } + + /** + * @return always returns true so that any get commands, upon getting this entry, will ignore the entry as though it + * were invalid. + */ + @Override + public boolean isValid() { + return false; + } + +} Modified: core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/main/java/org/horizon/factories/EntryFactory.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -48,7 +48,7 @@ */ boolean acquireLock(InvocationContext ctx, Object key) throws InterruptedException, TimeoutException; - MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked) throws InterruptedException; + MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked, boolean forRemoval) throws InterruptedException; MVCCEntry wrapEntryForReading(InvocationContext ctx, Object key) throws InterruptedException; } Modified: core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/main/java/org/horizon/factories/EntryFactoryImpl.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -26,6 +26,7 @@ import org.horizon.container.DataContainer; import org.horizon.container.MVCCEntry; import org.horizon.container.NullMarkerEntry; +import org.horizon.container.NullMarkerEntryForRemoval; import org.horizon.container.ReadCommittedEntry; import org.horizon.container.RepeatableReadEntry; import org.horizon.container.UpdateableEntry; @@ -42,7 +43,6 @@ public class EntryFactoryImpl implements EntryFactory { private boolean useRepeatableRead; - private static final NullMarkerEntry NULL_MARKER = new NullMarkerEntry(); DataContainer container; boolean writeSkewCheck; LockManager lockManager; @@ -66,8 +66,10 @@ writeSkewCheck = configuration.isWriteSkewCheck(); } - public final UpdateableEntry createWrappedEntry(Object key, Object value, boolean isForInsert, long lifespan) { - if (value == null && !isForInsert) return useRepeatableRead ? NULL_MARKER : null; + private UpdateableEntry createWrappedEntry(Object key, Object value, boolean isForInsert, boolean forRemoval, long lifespan) { + if (value == null && !isForInsert) return useRepeatableRead ? + forRemoval ? new NullMarkerEntryForRemoval(key) : NullMarkerEntry.getInstance() + : null; return useRepeatableRead ? new RepeatableReadEntry(key, value, lifespan) : new ReadCommittedEntry(key, value, lifespan); } @@ -76,7 +78,7 @@ MVCCEntry mvccEntry; if (ctx.hasOption(Options.FORCE_WRITE_LOCK)) { if (trace) log.trace("Forcing lock on reading"); - return wrapEntryForWriting(ctx, key, false, false, false); + return wrapEntryForWriting(ctx, key, false, false, false, false); } else if ((mvccEntry = ctx.lookupEntry(key)) == null) { if (trace) log.trace("Key " + key + " is not in context, fetching from container."); // simple implementation. Peek the entry, wrap it, put wrapped entry in the context. @@ -88,8 +90,8 @@ if (se != null) ctx.putLookedUpEntry(key, se); } else { mvccEntry = se == null ? - createWrappedEntry(key, null, false, -1) : - createWrappedEntry(key, se.getValue(), false, se.getLifespan()); + createWrappedEntry(key, null, false, false, -1) : + createWrappedEntry(key, se.getValue(), false, false, se.getLifespan()); if (mvccEntry != null) ctx.putLookedUpEntry(key, mvccEntry); } @@ -100,7 +102,7 @@ } } - public final MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked) throws InterruptedException { + public final MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked, boolean forRemoval) throws InterruptedException { MVCCEntry mvccEntry = ctx.lookupEntry(key); if (createIfAbsent && mvccEntry != null && mvccEntry.isNullEntry()) mvccEntry = null; if (mvccEntry != null) // exists in context! Just acquire lock if needed, and wrap. @@ -108,11 +110,12 @@ // acquire lock if needed if (alreadyLocked || acquireLock(ctx, key)) { UpdateableEntry ue; - if (mvccEntry instanceof UpdateableEntry) { + + if (mvccEntry instanceof UpdateableEntry && (!forRemoval || !(mvccEntry instanceof NullMarkerEntry))) { ue = (UpdateableEntry) mvccEntry; } else { // this is a read-only entry that needs to be copied to a proper read-write entry!! - ue = createWrappedEntry(key, mvccEntry.getValue(), false, mvccEntry.getLifespan()); + ue = createWrappedEntry(key, mvccEntry.getValue(), false, forRemoval, mvccEntry.getLifespan()); mvccEntry = ue; ctx.putLookedUpEntry(key, mvccEntry); } @@ -134,7 +137,7 @@ // exists in cache! Just acquire lock if needed, and wrap. // do we need a lock? boolean needToCopy = alreadyLocked || acquireLock(ctx, key) || ctx.hasOption(Options.SKIP_LOCKING); // even if we do not acquire a lock, if skip-locking is enabled we should copy - UpdateableEntry ue = createWrappedEntry(key, cachedValue.getValue(), false, cachedValue.getLifespan()); + UpdateableEntry ue = createWrappedEntry(key, cachedValue.getValue(), false, false, cachedValue.getLifespan()); ctx.putLookedUpEntry(key, ue); if (needToCopy) ue.copyForUpdate(container, writeSkewCheck); mvccEntry = ue; @@ -144,7 +147,7 @@ // now to lock and create the entry. Lock first to prevent concurrent creation! if (!alreadyLocked) acquireLock(ctx, key); notifier.notifyCacheEntryCreated(key, true, ctx); - UpdateableEntry ue = createWrappedEntry(key, null, true, -1); + UpdateableEntry ue = createWrappedEntry(key, null, true, false, -1); ue.setCreated(true); ctx.putLookedUpEntry(key, ue); ue.copyForUpdate(container, writeSkewCheck); Modified: core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/main/java/org/horizon/interceptors/CacheLoaderInterceptor.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -126,7 +126,7 @@ } // Reuse the lock and create a new entry for loading - MVCCEntry n = entryFactory.wrapEntryForWriting(ctx, key, true, false, keyLocked); + MVCCEntry n = entryFactory.wrapEntryForWriting(ctx, key, true, false, keyLocked, false); n = loadEntry(ctx, key, n); ctx.setContainsModifications(true); } Modified: core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java =================================================================== --- core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/main/java/org/horizon/interceptors/LockingInterceptor.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -129,7 +129,8 @@ public Object visitClearCommand(InvocationContext ctx, ClearCommand command) throws Throwable { try { // get a snapshot of all keys in the data container - for (Object key : dataContainer.keySet()) entryFactory.wrapEntryForWriting(ctx, key, false, false, false); + for (Object key : dataContainer.keySet()) + entryFactory.wrapEntryForWriting(ctx, key, false, false, false, false); ctx.setContainsModifications(true); return invokeNextInterceptor(ctx, command); } finally { @@ -147,7 +148,7 @@ public Object visitInvalidateCommand(InvocationContext ctx, InvalidateCommand command) throws Throwable { try { if (command.getKeys() != null) { - for (Object key : command.getKeys()) entryFactory.wrapEntryForWriting(ctx, key, false, true, false); + for (Object key : command.getKeys()) entryFactory.wrapEntryForWriting(ctx, key, false, true, false, false); } Object o = invokeNextInterceptor(ctx, command); if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful()); @@ -160,7 +161,7 @@ @Override public Object visitPutKeyValueCommand(InvocationContext ctx, PutKeyValueCommand command) throws Throwable { try { - entryFactory.wrapEntryForWriting(ctx, command.getKey(), true, false, false); + entryFactory.wrapEntryForWriting(ctx, command.getKey(), true, false, false, false); Object o = invokeNextInterceptor(ctx, command); if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful()); return o; @@ -173,7 +174,7 @@ public Object visitPutMapCommand(InvocationContext ctx, PutMapCommand command) throws Throwable { try { for (Object key : command.getMap().keySet()) { - entryFactory.wrapEntryForWriting(ctx, key, true, false, false); + entryFactory.wrapEntryForWriting(ctx, key, true, false, false, false); } Object o = invokeNextInterceptor(ctx, command); if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful()); @@ -187,7 +188,7 @@ @Override public Object visitRemoveCommand(InvocationContext ctx, RemoveCommand command) throws Throwable { try { - entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false); + entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false, true); Object o = invokeNextInterceptor(ctx, command); if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful()); return o; @@ -200,7 +201,7 @@ @Override public Object visitReplaceCommand(InvocationContext ctx, ReplaceCommand command) throws Throwable { try { - entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false); + entryFactory.wrapEntryForWriting(ctx, command.getKey(), false, true, false, false); Object o = invokeNextInterceptor(ctx, command); if (!ctx.isContainsModifications()) ctx.setContainsModifications(command.isSuccessful()); return o; Modified: core/branches/flat/src/test/java/org/horizon/api/mvcc/repeatable_read/RepeatableReadLockTest.java =================================================================== --- core/branches/flat/src/test/java/org/horizon/api/mvcc/repeatable_read/RepeatableReadLockTest.java 2009-03-18 14:02:03 UTC (rev 7910) +++ core/branches/flat/src/test/java/org/horizon/api/mvcc/repeatable_read/RepeatableReadLockTest.java 2009-03-18 14:45:39 UTC (rev 7911) @@ -95,4 +95,21 @@ assert "v".equals(cache.get("k")); assertNoLocks(); } + + public void testRepeatableReadWithNullRemoval() throws Exception { + LockTestBaseTL tl = threadLocal.get(); + Cache<String, String> cache = tl.cache; + TransactionManager tm = tl.tm; + + // start with an empty cache + tm.begin(); + cache.get("a"); + Transaction tx = tm.suspend(); + cache.put("a", "v2"); + assert cache.get("a").equals("v2"); + tm.resume(tx); + cache.remove("a"); + tx.commit(); + assert cache.get("a") == null; + } }