From 77457e36700d291bffc43a2cc36f14561051fe31 Mon Sep 17 00:00:00 2001 From: Derek Hulley Date: Wed, 30 Sep 2009 15:41:37 +0000 Subject: [PATCH] Fixed unreported error: New transactions started in the post-commit phase *might* see stale cache data - Transactional caches were flushing last in the post-commit phase - New transactions in the post-commit phase would see cache data out of synch with the DB-committed data - Affects DOD5015 post-commit audit actions, which were not generating full datasets for the first-pass data import git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@16627 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../org/alfresco/repo/cache/CacheTest.java | 65 +++++-- .../repo/cache/TransactionalCache.java | 174 ++++++++++-------- .../AlfrescoTransactionSupport.java | 40 ++-- 3 files changed, 172 insertions(+), 107 deletions(-) diff --git a/source/java/org/alfresco/repo/cache/CacheTest.java b/source/java/org/alfresco/repo/cache/CacheTest.java index 99075a9f3c..cb6139d0bd 100644 --- a/source/java/org/alfresco/repo/cache/CacheTest.java +++ b/source/java/org/alfresco/repo/cache/CacheTest.java @@ -169,6 +169,7 @@ public class CacheTest extends TestCase String newGlobalTwo = "new_global_two"; String newGlobalThree = "new_global_three"; String updatedTxnThree = "updated_txn_three"; + String updatedTxnFour = "updated_txn_four"; // add item to global cache backingCache.put(newGlobalOne, newGlobalOne); @@ -198,17 +199,25 @@ public class CacheTest extends TestCase assertFalse("Transactionally removed item found in keys", transactionalKeys.contains(newGlobalOne)); assertTrue("Transactionally added item not found in keys", transactionalKeys.contains(updatedTxnThree)); - // Register a post-commit stresser. We do this here so that it is registered after the transactional cache - PostCommitCacheUser listener = new PostCommitCacheUser(transactionalCache, updatedTxnThree); - AlfrescoTransactionSupport.bindListener(listener); + // Register a post-commit cache reader to make sure that nothing blows up if the cache is hit in post-commit + PostCommitCacheReader listenerReader = new PostCommitCacheReader(transactionalCache, updatedTxnThree); + AlfrescoTransactionSupport.bindListener(listenerReader); + + // Register a post-commit cache reader to make sure that nothing blows up if the cache is hit in post-commit + PostCommitCacheWriter listenerWriter = new PostCommitCacheWriter(transactionalCache, updatedTxnFour, "FOUR"); + AlfrescoTransactionSupport.bindListener(listenerWriter); // commit the transaction txn.commit(); - // Check the post-commit stresser - if (listener.e != null) + // Check the post-commit stressers + if (listenerReader.e != null) { - throw listener.e; + throw listenerReader.e; + } + if (listenerWriter.e != null) + { + throw listenerWriter.e; } // check that backing cache was updated with the in-transaction changes @@ -231,18 +240,18 @@ public class CacheTest extends TestCase } /** - * This transaction listener attempts to use the cache in the afterCommit phase. Technically the + * This transaction listener attempts to read from the cache in the afterCommit phase. Technically the * transaction has finished, but the transaction resources are still available. * * @author Derek Hulley * @since 2.1 */ - private class PostCommitCacheUser extends TransactionListenerAdapter + private class PostCommitCacheReader extends TransactionListenerAdapter { private final SimpleCache transactionalCache; private final String key; private Throwable e; - private PostCommitCacheUser(SimpleCache transactionalCache, String key) + private PostCommitCacheReader(SimpleCache transactionalCache, String key) { this.transactionalCache = transactionalCache; this.key = key; @@ -260,12 +269,42 @@ public class CacheTest extends TestCase return; } } - @Override - public int hashCode() + } + + /** + * This transaction listener attempts to write to the cache in the afterCommit phase. Technically the + * transaction has finished, but the transaction resources are still available. + * + * @author Derek Hulley + * @since 2.1 + */ + private class PostCommitCacheWriter extends TransactionListenerAdapter + { + private final SimpleCache transactionalCache; + private final String key; + private final Object value; + private Throwable e; + private PostCommitCacheWriter(SimpleCache transactionalCache, String key, Object value) { - return -100000; + this.transactionalCache = transactionalCache; + this.key = key; + this.value = value; + } + @Override + public void afterCommit() + { + try + { + transactionalCache.put(key, value); + transactionalCache.remove(key); + transactionalCache.clear(); + } + catch (Throwable e) + { + this.e = e; + return; + } } - } /** diff --git a/source/java/org/alfresco/repo/cache/TransactionalCache.java b/source/java/org/alfresco/repo/cache/TransactionalCache.java index 8425980e14..544a8f640f 100644 --- a/source/java/org/alfresco/repo/cache/TransactionalCache.java +++ b/source/java/org/alfresco/repo/cache/TransactionalCache.java @@ -123,7 +123,7 @@ public class TransactionalCache { return false; } - if (!(obj instanceof TransactionalCache)) + if (!(obj instanceof TransactionalCache)) { return false; } @@ -396,40 +396,50 @@ public class TransactionalCache // Ensure that the cache isn't being modified if (txnData.isClosed) { - throw new AlfrescoRuntimeException("onCommit cache modifications are not allowed."); - } - // we have a transaction - add the item into the updated cache for this transaction - // are we in an overflow condition? - if (txnData.updatedItemsCache.getMemoryStoreSize() >= maxCacheSize) - { - // overflow about to occur or has occured - we can only guarantee non-stale - // data by clearing the shared cache after the transaction. Also, the - // shared cache needs to be ignored for the rest of the transaction. - txnData.isClearOn = true; - } - CacheBucket bucket = null; - if (sharedCache.contains(key)) - { - V existingValue = sharedCache.get(key); - // The value needs to be kept for later checks - bucket = new UpdateCacheBucket(existingValue, value); + if (isDebugEnabled) + { + logger.debug( + "In post-commit add: \n" + + " cache: " + this + "\n" + + " key: " + key + "\n" + + " value: " + value); + } } else { - // The value didn't exist before - bucket = new NewCacheBucket(value); - } - Element element = new Element(key, bucket); - txnData.updatedItemsCache.put(element); - // remove the item from the removed cache, if present - txnData.removedItemsCache.remove(key); - // done - if (isDebugEnabled) - { - logger.debug("In transaction - adding item direct to transactional update cache: \n" + - " cache: " + this + "\n" + - " key: " + key + "\n" + - " value: " + value); + // we have an active transaction - add the item into the updated cache for this transaction + // are we in an overflow condition? + if (txnData.updatedItemsCache.getMemoryStoreSize() >= maxCacheSize) + { + // overflow about to occur or has occured - we can only guarantee non-stale + // data by clearing the shared cache after the transaction. Also, the + // shared cache needs to be ignored for the rest of the transaction. + txnData.isClearOn = true; + } + CacheBucket bucket = null; + if (sharedCache.contains(key)) + { + V existingValue = sharedCache.get(key); + // The value needs to be kept for later checks + bucket = new UpdateCacheBucket(existingValue, value); + } + else + { + // The value didn't exist before + bucket = new NewCacheBucket(value); + } + Element element = new Element(key, bucket); + txnData.updatedItemsCache.put(element); + // remove the item from the removed cache, if present + txnData.removedItemsCache.remove(key); + // done + if (isDebugEnabled) + { + logger.debug("In transaction - adding item direct to transactional update cache: \n" + + " cache: " + this + "\n" + + " key: " + key + "\n" + + " value: " + value); + } } } } @@ -461,53 +471,62 @@ public class TransactionalCache // Ensure that the cache isn't being modified if (txnData.isClosed) { - throw new AlfrescoRuntimeException("onCommit cache modifications are not allowed."); - } - // is the shared cache going to be cleared? - if (txnData.isClearOn) - { - // don't store removals if we're just going to clear it all out later + if (isDebugEnabled) + { + logger.debug( + "In post-commit remove: \n" + + " cache: " + this + "\n" + + " key: " + key); + } } else { - // are we in an overflow condition? - if (txnData.removedItemsCache.getMemoryStoreSize() >= maxCacheSize) + // is the shared cache going to be cleared? + if (txnData.isClearOn) { - // overflow about to occur or has occured - we can only guarantee non-stale - // data by clearing the shared cache after the transaction. Also, the - // shared cache needs to be ignored for the rest of the transaction. - txnData.isClearOn = true; - if (isDebugEnabled) - { - logger.debug("In transaction - removal cache reach capacity reached: \n" + - " cache: " + this + "\n" + - " txn: " + AlfrescoTransactionSupport.getTransactionId()); - } + // don't store removals if we're just going to clear it all out later } else { - V existingValue = sharedCache.get(key); - if (existingValue == null) + // are we in an overflow condition? + if (txnData.removedItemsCache.getMemoryStoreSize() >= maxCacheSize) { - // There is no point doing a remove for a value that doesn't exist + // overflow about to occur or has occured - we can only guarantee non-stale + // data by clearing the shared cache after the transaction. Also, the + // shared cache needs to be ignored for the rest of the transaction. + txnData.isClearOn = true; + if (isDebugEnabled) + { + logger.debug("In transaction - removal cache reach capacity reached: \n" + + " cache: " + this + "\n" + + " txn: " + AlfrescoTransactionSupport.getTransactionId()); + } } else { - // Create a bucket to remove the value from the shared cache - CacheBucket removeBucket = new RemoveCacheBucket(existingValue); - Element element = new Element(key, removeBucket); - txnData.removedItemsCache.put(element); + V existingValue = sharedCache.get(key); + if (existingValue == null) + { + // There is no point doing a remove for a value that doesn't exist + } + else + { + // Create a bucket to remove the value from the shared cache + CacheBucket removeBucket = new RemoveCacheBucket(existingValue); + Element element = new Element(key, removeBucket); + txnData.removedItemsCache.put(element); + } } } - } - // remove the item from the udpated cache, if present - txnData.updatedItemsCache.remove(key); - // done - if (isDebugEnabled) - { - logger.debug("In transaction - adding item direct to transactional removed cache: \n" + - " cache: " + this + "\n" + - " key: " + key); + // remove the item from the udpated cache, if present + txnData.updatedItemsCache.remove(key); + // done + if (isDebugEnabled) + { + logger.debug("In transaction - adding item direct to transactional removed cache: \n" + + " cache: " + this + "\n" + + " key: " + key); + } } } } @@ -531,14 +550,22 @@ public class TransactionalCache // Ensure that the cache isn't being modified if (txnData.isClosed) { - throw new AlfrescoRuntimeException("onCommit cache modifications are not allowed."); + if (isDebugEnabled) + { + logger.debug( + "In post-commit clear: \n" + + " cache: " + this); + } + } + else + { + // the shared cache must be cleared at the end of the transaction + // and also serves to ensure that the shared cache will be ignored + // for the remainder of the transaction + txnData.isClearOn = true; + txnData.updatedItemsCache.removeAll(); + txnData.removedItemsCache.removeAll(); } - // the shared cache must be cleared at the end of the transaction - // and also serves to ensure that the shared cache will be ignored - // for the remainder of the transaction - txnData.isClearOn = true; - txnData.updatedItemsCache.removeAll(); - txnData.removedItemsCache.removeAll(); } else // no transaction { @@ -568,7 +595,6 @@ public class TransactionalCache /** * NO-OP */ - @SuppressWarnings("unchecked") public void beforeCommit(boolean readOnly) { } diff --git a/source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupport.java b/source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupport.java index e97799d5b4..fafe1dae83 100644 --- a/source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupport.java +++ b/source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupport.java @@ -778,6 +778,26 @@ public abstract class AlfrescoTransactionSupport logger.debug("After completion (" + statusStr + "): " + this); } + // Clean up the transactional caches + for (TransactionalCache cache : transactionalCaches) + { + try + { + if (status == TransactionSynchronization.STATUS_COMMITTED) + { + cache.afterCommit(); + } + else + { + cache.afterRollback(); + } + } + catch (RuntimeException e) + { + logger.error("After completion (" + statusStr + ") TransactionalCache exception", e); + } + } + List iterableListeners = getListenersIterable(); // notify listeners if (status == TransactionSynchronization.STATUS_COMMITTED) @@ -833,26 +853,6 @@ public abstract class AlfrescoTransactionSupport } } - // Clean up the transactional caches - for (TransactionalCache cache : transactionalCaches) - { - try - { - if (status == TransactionSynchronization.STATUS_COMMITTED) - { - cache.afterCommit(); - } - else - { - cache.afterRollback(); - } - } - catch (RuntimeException e) - { - logger.error("After completion (" + statusStr + ") TransactionalCache exception", e); - } - } - // clear the thread's registrations and synchronizations AlfrescoTransactionSupport.clearSynchronization(); }