Fixed bug caused by transaction listeners registering more listeners during callbacks

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@2347 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupport.java

@@ -16,8 +16,10 @@
  */
 package org.alfresco.repo.transaction;
 
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -486,6 +488,14 @@ public abstract class AlfrescoTransactionSupport
             return listeners;
         }
         
+        /**
+         * @return Returns the listeners in a list disconnected from the original set
+         */
+        private List<TransactionListener> getListenersIterable()
+        {
+            return new ArrayList<TransactionListener>(listeners);
+        }
+
         public String toString()
         {
             StringBuilder sb = new StringBuilder(50);
@@ -511,7 +521,7 @@ public abstract class AlfrescoTransactionSupport
                 integrityChecker.checkIntegrity();
             }
             // flush listeners
-            for (TransactionListener listener : listeners)
+            for (TransactionListener listener : getListenersIterable())
             {
                 listener.flush();
             }
@@ -568,7 +578,7 @@ public abstract class AlfrescoTransactionSupport
             }
 
             // These are still considered part of the transaction so are executed here
-            for (TransactionListener listener : listeners)
+            for (TransactionListener listener : getListenersIterable())
             {
                 listener.beforeCommit(readOnly);
             }
@@ -590,7 +600,7 @@ public abstract class AlfrescoTransactionSupport
                 logger.debug("Before completion: " + this);
             }
             // notify listeners
-            for (TransactionListener listener : listeners)
+            for (TransactionListener listener : getListenersIterable())
             {
                 listener.beforeCompletion();
             }
@@ -636,10 +646,11 @@ public abstract class AlfrescoTransactionSupport
                 }
             }
             
+            List<TransactionListener> iterableListeners = getListenersIterable();
             // notify listeners
             if (status  == TransactionSynchronization.STATUS_COMMITTED)
             {
-                for (TransactionListener listener : listeners)
+                for (TransactionListener listener : iterableListeners)
                 {
                     try
                     {
@@ -655,7 +666,7 @@ public abstract class AlfrescoTransactionSupport
             }
             else
             {
-                for (TransactionListener listener : listeners)
+                for (TransactionListener listener : iterableListeners)
                 {
                     try
                     {

source/java/org/alfresco/repo/transaction/AlfrescoTransactionSupportTest.java

@@ -23,6 +23,7 @@ import javax.transaction.UserTransaction;
 
 import junit.framework.TestCase;
 
+import org.alfresco.repo.transaction.TransactionUtil.TransactionWork;
 import org.alfresco.service.ServiceRegistry;
 import org.alfresco.service.transaction.TransactionService;
 import org.alfresco.util.ApplicationContextHelper;
@@ -42,10 +43,12 @@ public class AlfrescoTransactionSupportTest extends TestCase
     private static ApplicationContext ctx = ApplicationContextHelper.getApplicationContext();
 
     private ServiceRegistry serviceRegistry;
+    TransactionService transactionService;
     
     public void setUp() throws Exception
     {
         serviceRegistry = (ServiceRegistry) ctx.getBean(ServiceRegistry.SERVICE_REGISTRY);
+        transactionService = serviceRegistry.getTransactionService();
     }
     
     public void testTransactionId() throws Exception
@@ -60,16 +63,6 @@ public class AlfrescoTransactionSupportTest extends TestCase
         String txnId = AlfrescoTransactionSupport.getTransactionId();
         assertNotNull("Expected thread to have a txn id", txnId);
         
-        // check that it is threadlocal
-        Thread thread = new Thread(new Runnable()
-                {
-                    public void run()
-                    {
-                        String txnId = AlfrescoTransactionSupport.getTransactionId();
-                        assertNull("New thread seeing txn id");
-                    }
-                });
-        
         // check that the txn id doesn't change
         String txnIdCheck = AlfrescoTransactionSupport.getTransactionId();
         assertEquals("Transaction ID changed on same thread", txnId, txnIdCheck);
@@ -138,7 +131,6 @@ public class AlfrescoTransactionSupportTest extends TestCase
         };
         
         // begin a transaction
-        TransactionService transactionService = serviceRegistry.getTransactionService();
         UserTransaction txn = transactionService.getUserTransaction();
         txn.begin();
         
@@ -155,4 +147,77 @@ public class AlfrescoTransactionSupportTest extends TestCase
         assertTrue("beforeCompletion not called on listener", strings.contains("beforeCompletion"));
         assertTrue("afterCommit not called on listener", strings.contains("afterCommit"));
     }
+    
+    /**
+     * Tests the condition whereby a listener can cause failure by attempting to bind itself to
+     * the transaction in the pre-commit callback.  This is caused by the listener set being
+     * modified during calls to the listeners.
+     */
+    public void testPreCommitListenerBinding() throws Exception
+    {
+        final String beforeCommit = "beforeCommit";
+        final String afterCommitInner = "afterCommit - inner";
+        final String afterCommitOuter = "afterCommit = outer";
+        
+        // the listeners will play with this
+        final List<String> testList = new ArrayList<String>(1);
+        testList.add(beforeCommit);
+        testList.add(afterCommitInner);
+        testList.add(afterCommitOuter);
+        
+        final TransactionListener listener = new TransactionListenerAdapter()
+        {
+            @Override
+            public int hashCode()
+            {
+                // force this listener to be first in the bound set
+                return 100;
+            }
+            @Override
+            public void beforeCommit(boolean readOnly)
+            {
+                testList.remove(beforeCommit);
+                TransactionListener postCommitListener = new TransactionListenerAdapter()
+                {
+                    @Override
+                    public void afterCommit()
+                    {
+                        testList.remove(afterCommitInner);
+                    }
+                };
+                // register bogus on the transaction
+                AlfrescoTransactionSupport.bindListener(postCommitListener);
+            }
+            @Override
+            public void afterCommit()
+            {
+                testList.remove(afterCommitOuter);
+            }
+        };
+        final TransactionListener dummyListener = new TransactionListenerAdapter()
+        {
+            @Override
+            public int hashCode()
+            {
+                // force the dummy listener to be AFTER the binding listener
+                return 200;
+            }
+        };
+        // start a transaction
+        TransactionWork<Object> bindWork = new TransactionWork<Object>()
+        {
+            public Object doWork() throws Exception
+            {
+                // just bind the listener to the transaction
+                AlfrescoTransactionSupport.bindListener(dummyListener);
+                AlfrescoTransactionSupport.bindListener(listener);
+                return null;
+            }
+        };
+        // kick it all off
+        TransactionUtil.executeInNonPropagatingUserTransaction(transactionService, bindWork);
+        
+        // make sure that the binding all worked
+        assertTrue("Expected callbacks not all processed: " + testList, testList.size() == 0);
+    }
 }

source/java/org/alfresco/repo/transaction/TransactionListenerAdapter.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2005 Alfresco, Inc.
+ *
+ * Licensed under the Mozilla Public License version 1.1 
+ * with a permitted attribution clause. You may obtain a
+ * copy of the License at
+ *
+ *   http://www.alfresco.org/legal/license.txt
+ *
+ * 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.alfresco.repo.transaction;
+
+/**
+ * NO-OP listener.
+ * 
+ * @author Derek Hulley
+ */
+public abstract class TransactionListenerAdapter implements TransactionListener
+{
+    /**
+     * @inheritDoc
+     */
+    public void flush()
+    {
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public void beforeCommit(boolean readOnly)
+    {
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public void beforeCompletion()
+    {
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public void afterCommit()
+    {
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public void afterRollback()
+    {
+    }
+}