GERONIMO-452, GERONIMO-5648, GERONIMO-5649 add retry logic for rollback, add logic for commit and rollback response to XAER_RMFAIL, and set tx timeout on XAResource.  Port from rev 1023970 in 2.2 branch

git-svn-id: https://svn.apache.org/repos/asf/geronimo/components/txmanager/trunk@1026099 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/CommitTask.java b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/CommitTask.java
index a6fb482..7ea6722 100644
--- a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/CommitTask.java
+++ b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/CommitTask.java
@@ -20,13 +20,13 @@
 
 package org.apache.geronimo.transaction.manager;
 
-import java.util.Iterator;
 import java.util.List;
 
 import javax.transaction.Status;
+import javax.transaction.SystemException;
 import javax.transaction.xa.XAException;
 import javax.transaction.xa.Xid;
-
+import org.apache.geronimo.transaction.manager.TransactionImpl.ReturnableTransactionBranch;
 import org.apache.geronimo.transaction.manager.TransactionImpl.TransactionBranch;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -39,19 +39,17 @@
     private final Xid xid;
     private final List<TransactionBranch> rms;
     private final Object logMark;
-    private final RetryScheduler retryScheduler;
-    private final TransactionLog txLog;
+    private final TransactionManagerImpl txManager;
     private int count = 0;
     private int status;
     private XAException cause;
     private boolean evercommit;
 
-    public CommitTask(Xid xid, List<TransactionBranch> rms, Object logMark, RetryScheduler retryScheduler, TransactionLog txLog) {
+    public CommitTask(Xid xid, List<TransactionBranch> rms, Object logMark, TransactionManagerImpl txManager) {
         this.xid = xid;
         this.rms = rms;
         this.logMark = logMark;
-        this.retryScheduler = retryScheduler;
-        this.txLog = txLog;
+        this.txManager = txManager;
     }
 
     @Override
@@ -59,38 +57,64 @@
         synchronized (this) {
             status = Status.STATUS_COMMITTING;
         }
-        for (Iterator<TransactionBranch> i = rms.iterator(); i.hasNext();) {
-            TransactionBranch manager = i.next();
+        for (int index = 0; index < rms.size(); ) {
+            TransactionBranch manager = rms.get(index);
             try {
                 try {
                     manager.getCommitter().commit(manager.getBranchId(), false);
+                    remove(index);
                     evercommit = true;
-                    i.remove();
                 } catch (XAException e) {
                     log.error("Unexpected exception committing " + manager.getCommitter() + "; continuing to commit other RMs", e);
 
                     if (e.errorCode == XAException.XA_HEURRB) {
-                        i.remove();
+                        remove(index);
                         log.info("Transaction has been heuristically rolled back");
                         cause = e;
                         manager.getCommitter().forget(manager.getBranchId());
                     } else if (e.errorCode == XAException.XA_HEURMIX) {
-                        i.remove();
+                        remove(index);
                         log.info("Transaction has been heuristically committed and rolled back");
                         cause = e;
                         evercommit = true;
                         manager.getCommitter().forget(manager.getBranchId());
                     } else if (e.errorCode == XAException.XA_HEURCOM) {
-                        i.remove();
+                        remove(index);
                         // let's not throw an exception as the transaction has been committed
                         log.info("Transaction has been heuristically committed");
                         evercommit = true;
                         manager.getCommitter().forget(manager.getBranchId());
                     } else if (e.errorCode == XAException.XA_RETRY) {
                         // do nothing, retry later
+                        index++;
+                    } else if (e.errorCode == XAException.XAER_RMFAIL) {
+                        //refresh the xa resource from the NamedXAResourceFactory
+                        if (manager.getCommitter() instanceof NamedXAResource) {
+                            String xaResourceName = ((NamedXAResource)manager.getCommitter()).getName();
+                            NamedXAResourceFactory namedXAResourceFactory = txManager.getNamedXAResourceFactory(xaResourceName);
+                            if (namedXAResourceFactory != null) {
+                                try {
+                                    TransactionBranch newManager = new ReturnableTransactionBranch(manager.getBranchXid(), namedXAResourceFactory);
+                                    remove(index);
+                                    rms.add(index, newManager);
+                                    //loop will try this one again immediately.
+                                } catch (SystemException e1) {
+                                    //try again later
+                                    index++;
+                                }
+                            } else {
+                                //else hope NamedXAResourceFactory reappears soon.
+                                index++;
+                            }
+                        } else {
+                            //no hope
+                            remove(index);
+                            cause = e;
+                        }
                     } else {
+                        //at least RMERR, which we can do nothing about
                         //nothing we can do about it.... keep trying
-                        i.remove();
+                        remove(index);
                         cause = e;
                     }
                 }
@@ -106,7 +130,7 @@
         //if all resources were read only, we didn't write a prepare record.
         if (rms.isEmpty()) {
             try {
-                txLog.commit(xid, logMark);
+                txManager.getTransactionLog().commit(xid, logMark);
                 synchronized (this) {
                     status = Status.STATUS_COMMITTED;
                 }
@@ -115,7 +139,17 @@
                 cause = (XAException) new XAException("Unexpected error logging commit completion for xid " + xid).initCause(e);
             }
         } else {
-            retryScheduler.retry(this, count++);
+            synchronized (this) {
+                status = Status.STATUS_UNKNOWN;
+            }
+            txManager.getRetryScheduler().retry(this, count++);
+        }
+    }
+
+    private void remove(int index) {
+        TransactionBranch manager = rms.remove(index);
+        if (manager instanceof ReturnableTransactionBranch) {
+            ((ReturnableTransactionBranch)manager).returnXAResource();
         }
     }
 
@@ -130,4 +164,5 @@
     public int getStatus() {
         return status;
     }
+
 }
diff --git a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RecoveryImpl.java b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RecoveryImpl.java
index d3d1668..7d3f349 100644
--- a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RecoveryImpl.java
+++ b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RecoveryImpl.java
@@ -48,9 +48,8 @@
 public class RecoveryImpl implements Recovery {
     private static final Logger log = LoggerFactory.getLogger("Recovery");
 
-    private final TransactionLog txLog;
-    private final XidFactory xidFactory;
-    private final RetryScheduler retryScheduler;
+
+    private final TransactionManagerImpl txManager;
 
     private final Map<Xid, TransactionImpl> externalXids = new HashMap<Xid, TransactionImpl>();
     private final Map<ByteArrayWrapper, XidBranchesPair> ourXids = new HashMap<ByteArrayWrapper, XidBranchesPair>();
@@ -59,23 +58,21 @@
 
     private final List<Exception> recoveryErrors = new ArrayList<Exception>();
 
-    public RecoveryImpl(final TransactionLog txLog, final XidFactory xidFactory, RetryScheduler retryScheduler) {
-        this.txLog = txLog;
-        this.xidFactory = xidFactory;
-        this.retryScheduler = retryScheduler;
+    public RecoveryImpl(TransactionManagerImpl txManager) {
+        this.txManager = txManager;
     }
 
     public synchronized void recoverLog() throws XAException {
         Collection<XidBranchesPair> preparedXids;
         try {
-            preparedXids = txLog.recover(xidFactory);
+            preparedXids = txManager.getTransactionLog().recover(txManager.getXidFactory());
         } catch (LogException e) {
             throw (XAException) new XAException(XAException.XAER_RMERR).initCause(e);
         }
         for (XidBranchesPair xidBranchesPair : preparedXids) {
             Xid xid = xidBranchesPair.getXid();
             log.trace("read prepared global xid from log: " + toString(xid));
-            if (xidFactory.matchesGlobalId(xid.getGlobalTransactionId())) {
+            if (txManager.getXidFactory().matchesGlobalId(xid.getGlobalTransactionId())) {
                 ourXids.put(new ByteArrayWrapper(xid.getGlobalTransactionId()), xidBranchesPair);
                 for (TransactionBranchInfo transactionBranchInfo : xidBranchesPair.getBranches()) {
                     log.trace("read branch from log: " + transactionBranchInfo.toString());
@@ -89,7 +86,7 @@
                 }
             } else {
                 log.trace("read external prepared xid from log: " + toString(xid));
-                TransactionImpl externalTx = new ExternalTransaction(xid, txLog, retryScheduler, xidBranchesPair.getBranches());
+                TransactionImpl externalTx = new ExternalTransaction(xid, txManager, xidBranchesPair.getBranches());
                 externalXids.put(xid, externalTx);
                 externalGlobalIdMap.put(xid.getGlobalTransactionId(), externalTx);
             }
@@ -123,7 +120,7 @@
                 } else {
                     log.trace("This xid was prepared from another XAResource, ignoring");
                 }
-            } else if (xidFactory.matchesGlobalId(xid.getGlobalTransactionId())) {
+            } else if (txManager.getXidFactory().matchesGlobalId(xid.getGlobalTransactionId())) {
                 //ours, but prepare not logged
                 log.trace("this xid was initiated from this tm but not prepared: rolling back");
                 try {
@@ -132,7 +129,7 @@
                     recoveryErrors.add(e);
                     log.error("Could not roll back", e);
                 }
-            } else if (xidFactory.matchesBranchId(xid.getBranchQualifier())) {
+            } else if (txManager.getXidFactory().matchesBranchId(xid.getBranchQualifier())) {
                 //our branch, but we did not start this tx.
                 TransactionImpl externalTx = externalGlobalIdMap.get(xid.getGlobalTransactionId());
                 if (externalTx == null) {
@@ -190,7 +187,7 @@
         if (xidBranchesPair.getBranches().isEmpty() && 0 != removed ) {
             try {
                 ourXids.remove(new ByteArrayWrapper(xidBranchesPair.getXid().getGlobalTransactionId()));
-                txLog.commit(xidBranchesPair.getXid(), xidBranchesPair.getMark());
+                txManager.getTransactionLog().commit(xidBranchesPair.getXid(), xidBranchesPair.getMark());
             } catch (LogException e) {
                 recoveryErrors.add(e);
                 log.error("Could not commit", e);
@@ -273,8 +270,8 @@
     private static class ExternalTransaction extends TransactionImpl {
         private final Set<String> resourceNames = new HashSet<String>();
 
-        public ExternalTransaction(Xid xid, TransactionLog txLog, RetryScheduler retryScheduler, Set<TransactionBranchInfo> resourceNames) {
-            super(xid, txLog, retryScheduler);
+        public ExternalTransaction(Xid xid, TransactionManagerImpl txManager, Set<TransactionBranchInfo> resourceNames) {
+            super(xid, txManager);
             for (TransactionBranchInfo info: resourceNames) {
                 this.resourceNames.add(info.getResourceName());
             }
diff --git a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RollbackTask.java b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RollbackTask.java
new file mode 100644
index 0000000..c8170ca
--- /dev/null
+++ b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/RollbackTask.java
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+
+package org.apache.geronimo.transaction.manager;
+
+import java.util.List;
+
+import javax.transaction.Status;
+import javax.transaction.SystemException;
+import javax.transaction.xa.XAException;
+import javax.transaction.xa.Xid;
+import org.apache.geronimo.transaction.manager.TransactionImpl.ReturnableTransactionBranch;
+import org.apache.geronimo.transaction.manager.TransactionImpl.TransactionBranch;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * @version $Rev$ $Date$
+ */
+public class RollbackTask implements Runnable {
+    private static final Logger log = LoggerFactory.getLogger(RollbackTask.class);
+    private final Xid xid;
+    private final List<TransactionBranch> rms;
+    private final Object logMark;
+    private final TransactionManagerImpl txManager;
+    private int count = 0;
+    private int status;
+    private XAException cause;
+    private boolean everRolledBack;
+
+    public RollbackTask(Xid xid, List<TransactionBranch> rms, Object logMark, TransactionManagerImpl txManager) {
+        this.xid = xid;
+        this.rms = rms;
+        this.logMark = logMark;
+        this.txManager = txManager;
+    }
+
+    public void run() {
+        synchronized (this) {
+            status = Status.STATUS_ROLLING_BACK;
+        }
+        for (int index = 0; index < rms.size(); ) {
+            TransactionBranch manager = rms.get(index);
+            try {
+                try {
+                    manager.getCommitter().rollback(manager.getBranchId());
+                    remove(index);
+                    everRolledBack = true;
+                } catch (XAException e) {
+                    log.error("Unexpected exception committing " + manager.getCommitter() + "; continuing to commit other RMs", e);
+
+                    if (e.errorCode == XAException.XA_HEURRB) {
+                        remove(index);
+                        // let's not throw an exception as the transaction has been rolled back
+                        log.info("Transaction has been heuristically rolled back");
+                        everRolledBack = true;
+                        manager.getCommitter().forget(manager.getBranchId());
+                    } else if (e.errorCode == XAException.XA_HEURMIX) {
+                        remove(index);
+                        log.info("Transaction has been heuristically committed and rolled back");
+                        everRolledBack = true;
+                        cause = e;
+                        manager.getCommitter().forget(manager.getBranchId());
+                    } else if (e.errorCode == XAException.XA_HEURCOM) {
+                        remove(index);
+                        log.info("Transaction has been heuristically committed");
+                        cause = e;
+                        manager.getCommitter().forget(manager.getBranchId());
+                    } else if (e.errorCode == XAException.XA_RETRY) {
+                        // do nothing, retry later
+                        index++;
+                    } else if (e.errorCode == XAException.XAER_RMFAIL) {
+                        //refresh the xa resource from the NamedXAResourceFactory
+                        if (manager.getCommitter() instanceof NamedXAResource) {
+                            String xaResourceName = ((NamedXAResource)manager.getCommitter()).getName();
+                            NamedXAResourceFactory namedXAResourceFactory = txManager.getNamedXAResourceFactory(xaResourceName);
+                            if (namedXAResourceFactory != null) {
+                                try {
+                                    TransactionBranch newManager = new ReturnableTransactionBranch(manager.getBranchXid(), namedXAResourceFactory);
+                                    remove(index);
+                                    rms.add(index, newManager);
+                                    //loop will try this one again immediately.
+                                } catch (SystemException e1) {
+                                    //try again later
+                                    index++;
+                                }
+                            } else {
+                                //else hope NamedXAResourceFactory reappears soon.
+                                index++;
+                            }
+                        } else {
+                            //no hope
+                            remove(index);
+                            cause = e;
+                        }
+                    } else {
+                        //at least these error codes:
+                        // XAException.XA_RMERR
+                        // XAException.XA_RBROLLBACK
+                        // XAException.XAER_NOTA
+                        //nothing we can do about it
+                        remove(index);
+                        cause = e;
+                    }
+                }
+            } catch (XAException e) {
+                if (e.errorCode == XAException.XAER_NOTA) {
+                    // NOTA in response to forget, means the resource already forgot the transaction
+                    // ignore
+                } else {
+                    cause = e;
+                }
+            }
+        }
+
+        if (rms.isEmpty()) {
+            try {
+                if (logMark != null) {
+                    txManager.getTransactionLog().rollback(xid, logMark);
+                }
+                synchronized (this) {
+                    status = Status.STATUS_ROLLEDBACK;
+                }
+            } catch (LogException e) {
+                log.error("Unexpected exception logging commit completion for xid " + xid, e);
+                cause = (XAException) new XAException("Unexpected error logging commit completion for xid " + xid).initCause(e);
+            }
+        } else {
+            synchronized (this) {
+                status = Status.STATUS_UNKNOWN;
+            }
+            txManager.getRetryScheduler().retry(this, count++);
+        }
+    }
+
+    private void remove(int index) {
+        TransactionBranch manager = rms.remove(index);
+        if (manager instanceof ReturnableTransactionBranch) {
+            ((ReturnableTransactionBranch)manager).returnXAResource();
+        }
+    }
+
+    public XAException getCause() {
+        return cause;
+    }
+
+    public boolean isEverRolledBack() {
+        return everRolledBack;
+    }
+
+    public int getStatus() {
+        return status;
+    }
+}
diff --git a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionImpl.java b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionImpl.java
index 08f70fc..3a0445b 100644
--- a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionImpl.java
+++ b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionImpl.java
@@ -48,10 +48,8 @@
 public class TransactionImpl implements Transaction {
     private static final Logger log = LoggerFactory.getLogger("Transaction");
 
-    private final XidFactory xidFactory;
+    private final TransactionManagerImpl txManager;
     private final Xid xid;
-    private final TransactionLog txnLog;
-    private final RetryScheduler retryScheduler;
     private final long timeout;
     private final List<Synchronization> syncList = new ArrayList<Synchronization>(5);
     private final List<Synchronization> interposedSyncList = new ArrayList<Synchronization>(3);
@@ -63,18 +61,16 @@
 
     private final Map<Object, Object> resources = new HashMap<Object, Object>();
 
-    TransactionImpl(XidFactory xidFactory, TransactionLog txnLog, RetryScheduler retryScheduler, long transactionTimeoutMilliseconds) throws SystemException {
-        this(xidFactory.createXid(), xidFactory, txnLog, retryScheduler, transactionTimeoutMilliseconds);
+    TransactionImpl(TransactionManagerImpl txManager, long transactionTimeoutMilliseconds) throws SystemException {
+        this(txManager.getXidFactory().createXid(), txManager, transactionTimeoutMilliseconds);
     }
 
-    TransactionImpl(Xid xid, XidFactory xidFactory, TransactionLog txnLog, RetryScheduler retryScheduler, long transactionTimeoutMilliseconds) throws SystemException {
-        this.xidFactory = xidFactory;
-        this.txnLog = txnLog;
-        this.retryScheduler = retryScheduler;
+    TransactionImpl(Xid xid, TransactionManagerImpl txManager, long transactionTimeoutMilliseconds) throws SystemException {
+        this.txManager = txManager;
         this.xid = xid;
         this.timeout = transactionTimeoutMilliseconds + TransactionTimer.getCurrentTime();
         try {
-            txnLog.begin(xid);
+            txManager.getTransactionLog().begin(xid);
         } catch (LogException e) {
             status = Status.STATUS_MARKED_ROLLBACK;
             SystemException ex = new SystemException("Error logging begin; transaction marked for roll back)");
@@ -85,11 +81,9 @@
     }
 
     //reconstruct a tx for an external tx found in recovery
-    public TransactionImpl(Xid xid, TransactionLog txLog, RetryScheduler retryScheduler) {
-        this.xidFactory = null;
-        this.txnLog = txLog;
+    public TransactionImpl(Xid xid, TransactionManagerImpl txManager) {
+        this.txManager = txManager;
         this.xid = xid;
-        this.retryScheduler = retryScheduler;
         status = Status.STATUS_PREPARED;
         //TODO is this a good idea?
         this.timeout = Long.MAX_VALUE;
@@ -204,8 +198,10 @@
                 }
             }
             //we know nothing about this XAResource or resource manager
-            Xid branchId = xidFactory.createBranch(xid, resourceManagers.size() + 1);
+            Xid branchId = txManager.getXidFactory().createBranch(xid, resourceManagers.size() + 1);
             xaRes.start(branchId, XAResource.TMNOFLAGS);
+            //Set the xaresource timeout in seconds to match the time left in this tx.
+            xaRes.setTransactionTimeout((int)(timeout - TransactionTimer.getCurrentTime())/1000);
             activeXaResources.put(xaRes, addBranchXid(xaRes, branchId));
             return true;
         } catch (XAException e) {
@@ -266,7 +262,7 @@
             }
 
             if (status == Status.STATUS_MARKED_ROLLBACK) {
-                rollbackResourcesDuringCommit(resourceManagers, false);
+                rollbackResources(resourceManagers, false);
                 if (timedout) {
                     throw new RollbackException("Unable to commit: Transaction timeout");
                 } else {
@@ -309,7 +305,7 @@
                 // two-phase
                 willCommit = internalPrepare();
             } catch (SystemException e) {
-                rollbackResources(resourceManagers);
+                rollbackResources(resourceManagers, false);
                 throw e;
             }
 
@@ -326,7 +322,8 @@
             } else {
                 // set everRollback to true here because the rollback here is caused by
                 // XAException during the above internalPrepare
-                rollbackResourcesDuringCommit(resourceManagers, true);
+                rollbackResources(resourceManagers, true);
+                throw new RollbackException("transaction rolled back due to problems in prepare");
             }
         } finally {
             afterCompletion();
@@ -365,7 +362,11 @@
                     result = XAResource.XA_OK;
                 }
             } else {
-                rollbackResources(rms);
+                try {
+                    rollbackResources(rms, false);
+                } catch (HeuristicMixedException e) {
+                    throw (SystemException)new SystemException("Unable to commit and heuristic exception during rollback").initCause(e);
+                }
                 throw new RollbackException("Unable to commit");
             }
         } finally {
@@ -456,10 +457,10 @@
         // log our decision
         if (willCommit && !resourceManagers.isEmpty()) {
             try {
-                logMark = txnLog.prepare(xid, resourceManagers);
+                logMark = txManager.getTransactionLog().prepare(xid, resourceManagers);
             } catch (LogException e) {
                 try {
-                    rollbackResources(resourceManagers);
+                    rollbackResources(resourceManagers, false);
                 } catch (Exception se) {
                     log.error("Unable to rollback after failure to log prepare", se.getCause());
                 }
@@ -486,19 +487,10 @@
 
         endResources();
         try {
-            rollbackResources(rms);
-            //only write rollback record if we have already written prepare record.
-            if (logMark != null) {
-                try {
-                    txnLog.rollback(xid, logMark);
-                } catch (LogException e) {
-                    try {
-                        rollbackResources(rms);
-                    } catch (Exception se) {
-                        log.error("Unable to rollback after failure to log decision", se.getCause());
-                    }
-                    throw (SystemException) new SystemException("Error logging rollback").initCause(e);
-                }
+            try {
+                rollbackResources(rms, false);
+            } catch (HeuristicMixedException e) {
+                throw (SystemException)new SystemException("Unable to roll back due to heuristics").initCause(e);
             }
         } finally {
             afterCompletion();
@@ -584,98 +576,19 @@
         }
     }
 
-    private void rollbackResources(List<TransactionBranch> rms) throws SystemException {
-        SystemException cause = null;
+    private void rollbackResources(List<TransactionBranch> rms, boolean everRb) throws HeuristicMixedException, SystemException {
+        RollbackTask rollbackTask = new RollbackTask(xid, rms, logMark, txManager);
         synchronized (this) {
             status = Status.STATUS_ROLLING_BACK;
         }
-        try {
-            for (Iterator i = rms.iterator(); i.hasNext();) {
-                TransactionBranch manager = (TransactionBranch) i.next();
-                try {
-                    manager.getCommitter().rollback(manager.getBranchId());
-                } catch (XAException e) {
-                    log.error("Unexpected exception rolling back " + manager.getCommitter() + "; continuing with rollback", e);
-                    if (e.errorCode == XAException.XA_HEURRB) {
-                        // let's not set the cause here
-                        log.info("Transaction has been heuristically rolled back " + manager.getCommitter() + "; continuing with rollback", e);
-                        manager.getCommitter().forget(manager.getBranchId());
-                    } else if (e.errorCode == XAException.XA_RBROLLBACK
-                            || e.errorCode == XAException.XAER_RMERR
-                            || e.errorCode == XAException.XAER_NOTA
-                            || e.errorCode == XAException.XAER_RMFAIL) {
-                        // let's not set the cause here because we expect the transaction to be rolled back eventually
-                        // TODO: for RMFAIL, it means resource unavailable temporarily.
-                        // do we need keep sending request to resource to make sure the roll back completes?
-                    } else if (cause == null) {
-                        cause = new SystemException(e.errorCode);
-                    }
-                }
-            }
-        } catch (XAException e) {
-            throw (SystemException) new SystemException("Error during rolling back").initCause(e);
-        }
-
+        rollbackTask.run();
         synchronized (this) {
-            status = Status.STATUS_ROLLEDBACK;
+            status = rollbackTask.getStatus();
         }
+        XAException cause = rollbackTask.getCause();
+        boolean everRolledback = everRb || rollbackTask.isEverRolledBack();
+
         if (cause != null) {
-            throw cause;
-        }
-    }
-
-    private void rollbackResourcesDuringCommit(List<TransactionBranch> rms, boolean everRb) throws HeuristicMixedException, RollbackException, SystemException {
-        XAException cause = null;
-        boolean everRolledback = everRb;
-        synchronized (this) {
-            status = Status.STATUS_ROLLING_BACK;
-        }
-        try {
-            for (Iterator i = rms.iterator(); i.hasNext();) {
-                TransactionBranch manager = (TransactionBranch) i.next();
-                try {
-                    manager.getCommitter().rollback(manager.getBranchId());
-                    everRolledback = true;
-                } catch (XAException e) {
-                    if (e.errorCode == XAException.XA_HEURRB) {
-                        // let's not set the cause here as the resulting behavior is same as requested behavior
-                        log.error("Transaction has been heuristically rolled back " + manager.getCommitter() + "; continuing with rollback", e);
-                        everRolledback = true;
-                        manager.getCommitter().forget(manager.getBranchId());
-                    } else if (e.errorCode == XAException.XA_HEURMIX) {
-                        log.error("Transaction has been heuristically committed and rolled back " + manager.getCommitter() + "; continuing with rollback", e);
-                        cause = e;
-                        everRolledback = true;
-                        manager.getCommitter().forget(manager.getBranchId());
-                    } else if (e.errorCode == XAException.XA_HEURCOM) {
-                        log.error("Transaction has been heuristically committed " + manager.getCommitter() + "; continuing with rollback", e);
-                        cause = e;
-                        manager.getCommitter().forget(manager.getBranchId());
-                    } else if (e.errorCode == XAException.XA_RBROLLBACK
-                            || e.errorCode == XAException.XAER_RMERR
-                            || e.errorCode == XAException.XAER_NOTA
-                            || e.errorCode == XAException.XAER_RMFAIL) {
-                        // XAException.XA_RBROLLBACK during commit/rollback, thus RollbackException is expected
-                        // XAException.XAER_RMERR means transaction branch error and transaction has been rolled back
-                        // let's not set the cause here because we expect the transaction to be rolled back eventually
-                        // TODO: for RMFAIL, it means resource unavailable temporarily.
-                        // do we need keep sending request to resource to make sure the roll back completes?
-                    } else if (cause == null) {
-                        cause = e;
-                    }
-                }
-            }
-        } catch (XAException e) {
-            throw (SystemException) new SystemException("System error during commit/rolling back").initCause(e);
-        }
-
-        synchronized (this) {
-            status = Status.STATUS_ROLLEDBACK;
-        }
-
-        if (cause == null) {
-            throw (RollbackException) new RollbackException("Unable to commit: transaction marked for rollback").initCause(cause);
-        } else {
             if (cause.errorCode == XAException.XA_HEURCOM && everRolledback) {
                 throw (HeuristicMixedException) new HeuristicMixedException("HeuristicMixed error during commit/rolling back").initCause(cause);
             } else if (cause.errorCode == XAException.XA_HEURMIX) {
@@ -753,7 +666,7 @@
     }
 
     private void commitResources(List<TransactionBranch> rms) throws HeuristicRollbackException, HeuristicMixedException, SystemException {
-        CommitTask commitTask = new CommitTask(xid, rms, logMark, retryScheduler, txnLog);
+        CommitTask commitTask = new CommitTask(xid, rms, logMark, txManager);
         synchronized (this) {
             status = Status.STATUS_COMMITTING;
         }
@@ -855,5 +768,16 @@
         }
     }
 
+    static class ReturnableTransactionBranch extends TransactionBranch {
+        private final NamedXAResourceFactory namedXAResourceFactory;
 
+        ReturnableTransactionBranch(Xid branchId, NamedXAResourceFactory namedXAResourceFactory) throws SystemException {
+            super(namedXAResourceFactory.getNamedXAResource(), branchId);
+            this.namedXAResourceFactory = namedXAResourceFactory;
+        }
+
+        public void returnXAResource() {
+            namedXAResourceFactory.returnNamedXAResource((NamedXAResource) getCommitter());
+        }
+    }
 }
diff --git a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java
index 8b8cc5e..d6daa1c 100644
--- a/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java
+++ b/geronimo-transaction/src/main/java/org/apache/geronimo/transaction/manager/TransactionManagerImpl.java
@@ -45,8 +45,8 @@
     protected static final int DEFAULT_TIMEOUT = 600;
     protected static final byte[] DEFAULT_TM_ID = new byte[] {71,84,77,73,68};
 
-    final TransactionLog transactionLog;
-    final XidFactory xidFactory;
+    private final TransactionLog transactionLog;
+    private final XidFactory xidFactory;
     private final int defaultTransactionTimeoutMilliseconds;
     private final ThreadLocal<Long> transactionTimeoutMilliseconds = new ThreadLocal<Long>();
     private final ThreadLocal<Transaction> threadTx = new ThreadLocal<Transaction>();
@@ -100,7 +100,7 @@
             this.xidFactory = new XidFactoryImpl(DEFAULT_TM_ID);
         }
 
-        recovery = new RecoveryImpl(this.transactionLog, this.xidFactory, retryScheduler);
+        recovery = new RecoveryImpl(this);
         recovery.recoverLog();
     }
 
@@ -156,7 +156,7 @@
         if (getStatus() != Status.STATUS_NO_TRANSACTION) {
             throw new NotSupportedException("Nested Transactions are not supported");
         }
-        TransactionImpl tx = new TransactionImpl(xidFactory, transactionLog, retryScheduler, getTransactionTimeoutMilliseconds(transactionTimeoutMilliseconds));
+        TransactionImpl tx = new TransactionImpl(this, getTransactionTimeoutMilliseconds(transactionTimeoutMilliseconds));
 //        timeoutTimer.schedule(tx, getTransactionTimeoutMilliseconds(transactionTimeoutMilliseconds));
         try {
             associate(tx);
@@ -274,7 +274,7 @@
         if (transactionTimeoutMilliseconds < 0) {
             throw new SystemException("transaction timeout must be positive or 0 to reset to default");
         }
-        return new TransactionImpl(xid, xidFactory, transactionLog, retryScheduler, getTransactionTimeoutMilliseconds(transactionTimeoutMilliseconds));
+        return new TransactionImpl(xid, this, getTransactionTimeoutMilliseconds(transactionTimeoutMilliseconds));
     }
 
     public void commit(Transaction tx, boolean onePhase) throws XAException {
@@ -357,6 +357,22 @@
         namedXAResourceFactories.remove(namedXAResourceFactoryName);
     }
 
+    NamedXAResourceFactory getNamedXAResourceFactory(String xaResourceName) {
+        return namedXAResourceFactories.get(xaResourceName);
+    }
+
+    XidFactory getXidFactory() {
+        return xidFactory;
+    }
+
+    TransactionLog getTransactionLog() {
+        return transactionLog;
+    }
+
+    RetryScheduler getRetryScheduler() {
+        return retryScheduler;
+    }
+
     public Map<Xid, TransactionImpl> getExternalXids() {
         return new HashMap<Xid, TransactionImpl>(recovery.getExternalXids());
     }
diff --git a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/AbstractRecoveryTest.java b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/AbstractRecoveryTest.java
index 3c11f5d..ba64e26 100644
--- a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/AbstractRecoveryTest.java
+++ b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/AbstractRecoveryTest.java
@@ -38,14 +38,17 @@
     protected TransactionLog txLog;
 
     protected final XidFactory xidFactory = new XidFactoryImpl();
-    protected final RetryScheduler retryScheduler = new ExponentialtIntervalRetryScheduler();
+    protected TransactionManagerImpl txManager;
     private static final String RM1 = "rm1";
     private static final String RM2 = "rm2";
     private static final String RM3 = "rm3";
     private static final int XID_COUNT = 3;
     private int branchCounter;
 
-    public void testDummy() throws Exception {}
+    @Override
+    protected void setUp() throws Exception {
+        txManager = new TransactionManagerImpl(1, xidFactory, txLog);
+    }
 
     public void test2ResOnlineAfterRecoveryStart() throws Exception {
         Xid[] xids = getXidArray(XID_COUNT);
@@ -56,8 +59,7 @@
         addBranch(txInfos, xares2);
         prepareLog(txLog, txInfos);
         prepareForReplay();
-        Recovery recovery = new RecoveryImpl(txLog, xidFactory, retryScheduler);
-        recovery.recoverLog();
+        Recovery recovery = txManager.recovery;
         assertTrue(!recovery.hasRecoveryErrors());
         assertTrue(recovery.getExternalXids().isEmpty());
         assertTrue(!recovery.localRecoveryComplete());
@@ -105,8 +107,7 @@
         addBranch(txInfos23, xares3);
         prepareLog(txLog, txInfos23);
         prepareForReplay();
-        Recovery recovery = new RecoveryImpl(txLog, xidFactory, retryScheduler);
-        recovery.recoverLog();
+        Recovery recovery = txManager.recovery;
         assertTrue(!recovery.hasRecoveryErrors());
         assertTrue(recovery.getExternalXids().isEmpty());
         assertEquals(XID_COUNT * 3, recovery.localUnrecoveredCount());
diff --git a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/HOWLLogRecoveryTest.java b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/HOWLLogRecoveryTest.java
index af9e764..4b22e87 100644
--- a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/HOWLLogRecoveryTest.java
+++ b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/HOWLLogRecoveryTest.java
@@ -54,6 +54,7 @@
             }
         }
         setUpHowlLog();
+        super.setUp();
     }
 
     private void setUpHowlLog() throws Exception {
@@ -86,6 +87,7 @@
     protected void prepareForReplay() throws Exception {
         tearDown();
         setUpHowlLog();
+        super.setUp();
     }
 
     public static Test suite() {
diff --git a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/MockLogRecoveryTest.java b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/MockLogRecoveryTest.java
index 9715107..9b1e097 100644
--- a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/MockLogRecoveryTest.java
+++ b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/MockLogRecoveryTest.java
@@ -27,6 +27,7 @@
 
     protected void setUp() throws Exception {
         txLog = new MockLog();
+        super.setUp();
     }
 
     protected void tearDown() throws Exception {
@@ -34,5 +35,6 @@
     }
 
     protected void prepareForReplay() throws Exception {
+        txManager.recovery.recoverLog();
     }
 }
diff --git a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/RecoveryTest.java b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/RecoveryTest.java
index 3720b64..62bd727 100644
--- a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/RecoveryTest.java
+++ b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/RecoveryTest.java
@@ -36,14 +36,19 @@
 public class RecoveryTest extends TestCase {
 
     XidFactory xidFactory = new XidFactoryImpl();
-    protected final RetryScheduler retryScheduler = new ExponentialtIntervalRetryScheduler();
+    MockLog mockLog = new MockLog();
+    protected TransactionManagerImpl txManager;
     private final String RM1 = "rm1";
     private final String RM2 = "rm2";
     private final String RM3 = "rm3";
     private int count = 0;
 
+    @Override
+    protected void setUp() throws Exception {
+        txManager = new TransactionManagerImpl(1, xidFactory, mockLog);
+    }
+
     public void testCommittedRMToBeRecovered() throws Exception {
-        MockLog mockLog = new MockLog();
         Xid[] xids = getXidArray(1);
         // specifies an empty Xid array such that this XAResource has nothing
         // to recover. This means that the presumed abort protocol has failed
@@ -53,7 +58,7 @@
         MockTransactionInfo[] txInfos = makeTxInfos(xids);
         addBranch(txInfos, xares1);
         prepareLog(mockLog, txInfos);
-        Recovery recovery = new RecoveryImpl(mockLog, xidFactory, retryScheduler);
+        Recovery recovery = new RecoveryImpl(txManager);
         recovery.recoverLog();
         assertTrue(!recovery.hasRecoveryErrors());
         assertTrue(recovery.getExternalXids().isEmpty());
@@ -65,7 +70,6 @@
     }
     
     public void test2ResOnlineAfterRecoveryStart() throws Exception {
-        MockLog mockLog = new MockLog();
         Xid[] xids = getXidArray(3);
         MockXAResource xares1 = new MockXAResource(RM1);
         MockXAResource xares2 = new MockXAResource(RM2);
@@ -73,7 +77,7 @@
         addBranch(txInfos, xares1);
         addBranch(txInfos, xares2);
         prepareLog(mockLog, txInfos);
-        Recovery recovery = new RecoveryImpl(mockLog, xidFactory, retryScheduler);
+        Recovery recovery = new RecoveryImpl(txManager);
         recovery.recoverLog();
         assertTrue(!recovery.hasRecoveryErrors());
         assertTrue(recovery.getExternalXids().isEmpty());
@@ -106,7 +110,6 @@
     }
 
     public void test3ResOnlineAfterRecoveryStart() throws Exception {
-        MockLog mockLog = new MockLog();
         Xid[] xids12 = getXidArray(3);
         List xids12List = Arrays.asList(xids12);
         Xid[] xids13 = getXidArray(3);
@@ -141,7 +144,7 @@
         addBranch(txInfos23, xares2);
         addBranch(txInfos23, xares3);
         prepareLog(mockLog, txInfos23);
-        Recovery recovery = new RecoveryImpl(mockLog, xidFactory, retryScheduler);
+        Recovery recovery = new RecoveryImpl(txManager);
         recovery.recoverLog();
         assertTrue(!recovery.hasRecoveryErrors());
         assertTrue(recovery.getExternalXids().isEmpty());
diff --git a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/TransactionManagerImplTest.java b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/TransactionManagerImplTest.java
index 372a29f..fed029d 100644
--- a/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/TransactionManagerImplTest.java
+++ b/geronimo-transaction/src/test/java/org/apache/geronimo/transaction/manager/TransactionManagerImplTest.java
@@ -244,7 +244,7 @@
     //This test depends on using the resource that will be recovered by the resource manager.
     public void testSimpleRecovery() throws Exception {
         //create a transaction in our own transaction manager
-        Xid xid = tm.xidFactory.createXid();
+        Xid xid = tm.getXidFactory().createXid();
         Transaction tx = tm.importXid(xid, 0);
         tm.resume(tx);
         assertSame(tx, tm.getTransaction());