Author: manik.surtani(a)jboss.com Date: 2009-01-23 07:17:02 -0500 (Fri, 23 Jan 2009) New Revision: 7584 Added: core/trunk/src/test/java/org/jboss/cache/transaction/MarkAsRollbackTest.java Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java Log: JBCACHE-1468: JBoss Cache ignores transactions with STATUS_MARKED_ROLLBACK and auto commits data changes Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java 2009-01-23 12:12:31 UTC (rev 7583) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java 2009-01-23 12:17:02 UTC (rev 7584) @@ -323,13 +323,17 @@ if (trace) log.trace("Created new tx for gtx " + gtx); if (trace) + { log.trace("Started new local tx as result of remote prepare: local tx=" + ltx + " (status=" + ltx.getStatus() + "), gtx=" + gtx); + } } else { //this should be valid if (!TransactionTable.isValid(ltx)) + { throw new CacheException("Transaction " + ltx + " not in correct state to be prepared"); + } //associate this thread with the local transaction associated with the global transaction, IF the localTx is NOT the current tx. if (currentTx == null || !ltx.equals(currentTx)) @@ -372,7 +376,9 @@ if (command.isOnePhaseCommit()) { if (trace) + { log.trace("Using one-phase prepare. Not propagating the prepare call up the stack until called to do so by the sync handler."); + } } else { @@ -562,9 +568,13 @@ throw new RuntimeException(e2); } if (e instanceof RuntimeException) + { throw (RuntimeException) e; + } else + { throw new RuntimeException("Commit failed.", e); + } } } @@ -675,9 +685,17 @@ */ private GlobalTransaction registerTransaction(Transaction tx, InvocationContext ctx) throws Exception { + // we have ascertained that the current thread *is* associated with a transaction. We need to make sure the + // transaction is in a valid state before moving on, and throwing an exception if not. + boolean txValid = TransactionTable.isValid(tx); + if (!txValid) + { + throw new IllegalStateException("Transaction " + tx + " is not in a valid state to be invoking cache operations on."); + } + GlobalTransaction gtx; - if (TransactionTable.isValid(tx) && transactions.add(tx)) + if (transactions.add(tx)) { gtx = txTable.getCurrentTransaction(tx, true); TransactionContext transactionContext; @@ -711,17 +729,7 @@ } else { - if (trace) - { - if (TransactionTable.isValid(tx)) - { - log.trace("Transaction " + tx + " is already registered."); - } - else - { - log.trace("Transaction " + tx + " is not valid! Status: " + tx.getStatus()); - } - } + if (trace) log.trace("Transaction " + tx + " is already registered."); } return gtx; } @@ -867,7 +875,9 @@ setTransactionalContext(tx, gtx, transactionContext, ctx); if (ctx.isOptionsUninitialised() && transactionContext.getOption() != null) + { ctx.setOptionOverrides(transactionContext.getOption()); + } assertCanContinue(); @@ -954,7 +964,9 @@ private void assertCanContinue() { if (!componentRegistry.invocationsAllowed(true) && (ctx.getOptionOverrides() == null || !ctx.getOptionOverrides().isSkipCacheStatusCheck())) + { throw new IllegalStateException("Cache not in STARTED state!"); + } } /** @@ -981,7 +993,9 @@ { // JBCACHE-1114 -- don't call toString() on tx or it can lead to stack overflow if (tx == null) + { return null; + } return tx.getClass().getName() + "@" + System.identityHashCode(tx); } @@ -1053,7 +1067,9 @@ if (result instanceof Throwable) { if (log.isDebugEnabled()) + { log.debug("Transaction needs to be rolled back - the cache returned an instance of Throwable for this prepare call (tx=" + tx + " and gtx=" + gtx + ")", (Throwable) result); + } tx.setRollbackOnly(); throw (Throwable) result; } @@ -1074,9 +1090,13 @@ throw new RuntimeException("setting tx rollback failed ", se); } if (t instanceof RuntimeException) + { throw (RuntimeException) t; + } else + { throw new RuntimeException("", t); + } } finally { Modified: core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java 2009-01-23 12:12:31 UTC (rev 7583) +++ core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java 2009-01-23 12:17:02 UTC (rev 7584) @@ -103,7 +103,9 @@ public GlobalTransaction get(Transaction tx) { if (tx == null) + { return null; + } return tx2gtxMap.get(tx); } @@ -194,7 +196,9 @@ public GlobalTransaction remove(Transaction tx) { if (tx == null) + { return null; + } return tx2gtxMap.remove(tx); } @@ -223,7 +227,9 @@ public String toString(boolean printDetails) { if (!printDetails) + { return toString(); + } StringBuilder sb = new StringBuilder(); sb.append("LocalTransactions: ").append(tx2gtxMap.size()).append("\n"); sb.append("GlobalTransactions: ").append(gtx2ContextMap.size()).append("\n"); @@ -353,14 +359,33 @@ } /** - * Return s true of tx's status is ACTIVE or PREPARING + * Returns true if transaction is STATUS_MARKED_ROLLBACK, false otherwise + */ + public static boolean isMarkedAsRollback(Transaction tx) + { + if (tx == null) return false; + int status; + try + { + status = tx.getStatus(); + return status == Status.STATUS_MARKED_ROLLBACK; + } + catch (SystemException e) + { + return false; + } + } + + + /** + * Returns true of tx's status is ACTIVE or PREPARING or MARKED_ROLLBACK * * @param tx * @return true if the tx is active or preparing */ public static boolean isValid(Transaction tx) { - return isActive(tx) || isPreparing(tx); + return isActive(tx) || isPreparing(tx) || isMarkedAsRollback(tx); } /** @@ -369,14 +394,17 @@ public static void assertTransactionValid(InvocationContext ctx) { Transaction tx = ctx.getTransaction(); - if (!isValid(tx)) try + if (!isValid(tx)) { - throw new CacheException("Invalid transaction " + tx + ", status = " + (tx == null ? null : tx.getStatus())); + try + { + throw new CacheException("Invalid transaction " + tx + ", status = " + (tx == null ? null : tx.getStatus())); + } + catch (SystemException e) + { + throw new CacheException("Exception trying to analyse status of transaction " + tx, e); + } } - catch (SystemException e) - { - throw new CacheException("Exception trying to analyse status of transaction " + tx, e); - } } @@ -425,13 +453,13 @@ return gtx; } - @ManagedAttribute(name="numberOfRegisteredTransactions", description = "Number of registered transactions") + @ManagedAttribute(name = "numberOfRegisteredTransactions", description = "Number of registered transactions") public int getNumberOfRegisteredTransactions() { return tx2gtxMap.size(); } - @ManagedAttribute(name="transactionMap", description = "A String representation of the transaction map") + @ManagedAttribute(name = "transactionMap", description = "A String representation of the transaction map") public String getTransactionMap() { return tx2gtxMap.toString(); Added: core/trunk/src/test/java/org/jboss/cache/transaction/MarkAsRollbackTest.java =================================================================== --- core/trunk/src/test/java/org/jboss/cache/transaction/MarkAsRollbackTest.java (rev 0) +++ core/trunk/src/test/java/org/jboss/cache/transaction/MarkAsRollbackTest.java 2009-01-23 12:17:02 UTC (rev 7584) @@ -0,0 +1,89 @@ +package org.jboss.cache.transaction; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.jboss.cache.CacheSPI; +import org.jboss.cache.Fqn; +import org.jboss.cache.UnitTestCacheFactory; +import org.jboss.cache.config.Configuration; +import org.jboss.cache.util.TestingUtil; +import org.testng.annotations.Test; + +import javax.transaction.RollbackException; +import javax.transaction.TransactionManager; + +@Test(groups = "functional") +public class MarkAsRollbackTest +{ + private static final Fqn fqn = Fqn.fromString("/a/b/c"); + private static final Log log = LogFactory.getLog(MarkAsRollbackTest.class); + + public void testMarkAsRollbackAfterMods() throws Exception + { + Configuration c = new Configuration(); + c.setTransactionManagerLookupClass(DummyTransactionManagerLookup.class.getName()); + CacheSPI<String, String> cache = (CacheSPI<String, String>) new UnitTestCacheFactory<String, String>().createCache(c, MarkAsRollbackTest.class); + try + { + TransactionManager tm = cache.getTransactionManager(); + assert tm != null; + tm.begin(); + cache.put(fqn, "k", "v"); + assert cache.get(fqn, "k").equals("v"); + assert cache.getRoot().getChildren().size() == 1; + tm.setRollbackOnly(); + try + { + tm.commit(); + assert false : "Should have rolled back"; + } + catch (RollbackException expected) + { + } + + assert tm.getTransaction() == null : "There should be no transaction in scope anymore!"; + assert cache.get(fqn, "k") == null : "Expected a null but was " + cache.get(fqn, "k"); + assert cache.getRoot().getChildren().size() == 0; + } + finally + { + log.warn("Cleaning up"); + TestingUtil.killCaches(cache); + } + } + + public void testMarkAsRollbackBeforeMods() throws Exception + { + Configuration c = new Configuration(); + c.setTransactionManagerLookupClass(DummyTransactionManagerLookup.class.getName()); + CacheSPI<String, String> cache = (CacheSPI<String, String>) new UnitTestCacheFactory<String, String>().createCache(c, MarkAsRollbackTest.class); + try + { + TransactionManager tm = cache.getTransactionManager(); + assert tm != null; + tm.begin(); + tm.setRollbackOnly(); + cache.put(fqn, "k", "v"); + assert cache.get(fqn, "k").equals("v"); + assert cache.getRoot().getChildren().size() == 1; + try + { + tm.commit(); + assert false : "Should have rolled back"; + } + catch (RollbackException expected) + { + + } + + assert tm.getTransaction() == null : "There should be no transaction in scope anymore!"; + assert cache.get(fqn, "k") == null : "Expected a null but was " + cache.get(fqn, "k"); + assert cache.getRoot().getChildren().size() == 0; + } + finally + { + log.warn("Cleaning up"); + TestingUtil.killCaches(cache); + } + } +}