Fixed AR-546:

VersionCounterDao is enclosed in non-propagating transactions (via config)
   version_counter row is locked while version number is incremented
   Added tests to ensure failure before fixing


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

config/alfresco/core-services-context.xml

@@ -318,10 +318,28 @@
         </property>
     </bean>
 
-    <bean id="versionCounterDaoService" class="org.alfresco.repo.version.common.counter.hibernate.HibernateVersionCounterDaoServiceImpl">
+    <bean id="versionCounterDaoService" class="org.springframework.transaction.interceptor.TransactionProxyFactoryBean">
+        <property name="proxyInterfaces">
+            <value>org.alfresco.repo.version.common.counter.VersionCounterDaoService</value>
+        </property>
+        <property name="transactionManager">
+            <ref bean="transactionManager" />
+        </property>
+        <property name="target">
+            <bean class="org.alfresco.repo.version.common.counter.hibernate.HibernateVersionCounterDaoServiceImpl" init-method="init">
                 <property name="sessionFactory">
                     <ref bean="sessionFactory" />
                 </property>
+                <property name="policyComponent">
+                    <ref bean="policyComponent" />
+                </property>
+            </bean>
+        </property>
+        <property name="transactionAttributes">
+            <props>
+                <prop key="*">${server.transaction.mode.default}, PROPAGATION_REQUIRES_NEW</prop>
+            </props>
+        </property>
     </bean>
 
     <bean id="versionableAspect" class="org.alfresco.repo.version.VersionableAspect" init-method="init">

source/java/org/alfresco/repo/domain/hibernate/VersionCountImpl.java

@@ -19,7 +19,6 @@ package org.alfresco.repo.domain.hibernate;
 import org.alfresco.repo.domain.Node;
 import org.alfresco.repo.domain.StoreKey;
 import org.alfresco.repo.domain.VersionCount;
-import org.alfresco.service.cmr.repository.StoreRef;
 
 /**
  * Hibernate-specific implementation of the domain entity <b>versioncounter</b>.
@@ -29,8 +28,9 @@ import org.alfresco.service.cmr.repository.StoreRef;
 public class VersionCountImpl implements VersionCount
 {
 	private StoreKey key;
+    @SuppressWarnings("unused")
+    private long version;    // used by Hibernate for concurrency
     private int versionCount;
-    private transient StoreRef storeRef;
 
     public VersionCountImpl()
     {
@@ -78,9 +78,9 @@ public class VersionCountImpl implements VersionCount
 		return key;
 	}
 
-	public synchronized void setKey(StoreKey key) {
+	public synchronized void setKey(StoreKey key)
+    {
 		this.key = key;
-        this.storeRef = null;
 	}
     
     /**
@@ -93,7 +93,9 @@ public class VersionCountImpl implements VersionCount
 
     public int incrementVersionCount()
     {
-        return ++versionCount;
+        int versionCount = getVersionCount() + 1;
+        setVersionCount(versionCount);
+        return versionCount;
     }
 
     /**

source/java/org/alfresco/repo/node/index/NodeIndexer.java

@@ -16,14 +16,12 @@
  */
 package org.alfresco.repo.node.index;
 
-import org.alfresco.model.ContentModel;
 import org.alfresco.repo.node.NodeServicePolicies;
 import org.alfresco.repo.policy.JavaBehaviour;
 import org.alfresco.repo.policy.PolicyComponent;
 import org.alfresco.repo.search.Indexer;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
-import org.alfresco.service.cmr.repository.StoreRef;
 import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 
@@ -34,8 +32,7 @@ import org.alfresco.service.namespace.QName;
  * @author Derek Hulley
  */
 public class NodeIndexer
-        implements NodeServicePolicies.BeforeCreateStorePolicy,
+        implements NodeServicePolicies.OnCreateNodePolicy,
-                   NodeServicePolicies.OnCreateNodePolicy,
                    NodeServicePolicies.OnUpdateNodePolicy,
                    NodeServicePolicies.OnDeleteNodePolicy,
                    NodeServicePolicies.OnCreateChildAssociationPolicy,
@@ -67,10 +64,6 @@ public class NodeIndexer
      */
     public void init()
     {
-        policyComponent.bindClassBehaviour(
-                QName.createQName(NamespaceService.ALFRESCO_URI, "beforeCreateStore"),
-                ContentModel.TYPE_STOREROOT,
-                new JavaBehaviour(this, "beforeCreateStore"));   
         policyComponent.bindClassBehaviour(
                 QName.createQName(NamespaceService.ALFRESCO_URI, "onCreateNode"),
                 this,
@@ -93,11 +86,6 @@ public class NodeIndexer
                 new JavaBehaviour(this, "onDeleteChildAssociation"));   
     }
 
-    public void beforeCreateStore(QName nodeTypeQName, StoreRef storeRef)
-    {
-        // indexer can perform some cleanup here, if required
-    }
-
     public void onCreateNode(ChildAssociationRef childAssocRef)
     {
         indexer.createNode(childAssocRef);

source/java/org/alfresco/repo/security/permissions/impl/hibernate/Permission.hbm.xml

@@ -8,7 +8,7 @@
     <class 
         name="org.alfresco.repo.security.permissions.impl.hibernate.NodePermissionEntryImpl"
         proxy="org.alfresco.repo.security.permissions.impl.hibernate.NodePermissionEntry"
-        table="node_permission"
+        table="node_perm_entry"
         dynamic-insert="false"
         dynamic-update="false"
         select-before-update="false" 
@@ -44,7 +44,7 @@
     <class 
         name="org.alfresco.repo.security.permissions.impl.hibernate.PermissionEntryImpl"
         proxy="org.alfresco.repo.security.permissions.impl.hibernate.PermissionEntry"
-        table="node_perm_entry"
+        table="perm_entry"
         dynamic-insert="false"
         dynamic-update="false"
         select-before-update="false"
@@ -89,7 +89,7 @@
     <class
         name="org.alfresco.repo.security.permissions.impl.hibernate.PermissionReferenceImpl"
         proxy="org.alfresco.repo.security.permissions.impl.hibernate.PermissionReference"
-        table="permission_ref"
+        table="perm_ref"
         dynamic-insert="false"
         dynamic-update="false"
         select-before-update="false"

source/java/org/alfresco/repo/version/common/counter/VersionCounterDaoService.java

@@ -17,6 +17,7 @@
 package org.alfresco.repo.version.common.counter;
 
 import org.alfresco.service.cmr.repository.StoreRef;
+import org.alfresco.service.namespace.QName;
 
 /**
  * Version counter DAO service interface.
@@ -25,6 +26,14 @@ import org.alfresco.service.cmr.repository.StoreRef;
  */
 public interface VersionCounterDaoService
 {
+    /**
+     * Helper method for simple patching
+     * 
+     * @param nodeTypeQName not used
+     * @param storeRef the store to create a counter for
+     */
+    public void beforeCreateStore(QName nodeTypeQName, StoreRef storeRef);
+    
     /**
      * Get the next available version number for the specified store.
      * 

source/java/org/alfresco/repo/version/common/counter/VersionCounterDaoServiceTest.java

@@ -16,75 +16,203 @@
  */
 package org.alfresco.repo.version.common.counter;
 
+import javax.transaction.UserTransaction;
+
+import junit.framework.TestCase;
+
+import org.alfresco.repo.transaction.TransactionUtil;
+import org.alfresco.repo.transaction.TransactionUtil.TransactionWork;
+import org.alfresco.service.ServiceRegistry;
 import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.cmr.repository.StoreRef;
-import org.alfresco.util.BaseSpringTest;
+import org.alfresco.service.transaction.TransactionService;
+import org.alfresco.util.ApplicationContextHelper;
+import org.springframework.context.ApplicationContext;
 
 /**
  * @author Roy Wetherall
  */
-public class VersionCounterDaoServiceTest extends BaseSpringTest
+public class VersionCounterDaoServiceTest extends TestCase
 {
-    /*
+    private static ApplicationContext ctx = ApplicationContextHelper.getApplicationContext();
-     * Test store id's
-     */
-    private final static String STORE_ID_1 = "test1_" + System.currentTimeMillis();
-    private final static String STORE_ID_2 = "test2_" + System.currentTimeMillis();
-    private static final String STORE_NONE = "test3_" + System.currentTimeMillis();;
     
+    private StoreRef storeRef1;
+    private StoreRef storeRef2;
+
+    private TransactionService transactionService;
     private NodeService nodeService;
     private VersionCounterDaoService counter;
     
     @Override
-    public void onSetUpInTransaction()
+    public void setUp() throws Exception
     {
-        nodeService = (NodeService) applicationContext.getBean("dbNodeService");
+        ServiceRegistry serviceRegistry = (ServiceRegistry) ctx.getBean("ServiceRegistry");
-        counter = (VersionCounterDaoService) applicationContext.getBean("versionCounterDaoService");
+        transactionService = serviceRegistry.getTransactionService();
+        nodeService = serviceRegistry.getNodeService();
+        counter = (VersionCounterDaoService) ctx.getBean("versionCounterDaoService");
+        
+        storeRef1 = nodeService.createStore(StoreRef.PROTOCOL_WORKSPACE, "test1_" + System.currentTimeMillis());
+        storeRef2 = nodeService.createStore(StoreRef.PROTOCOL_WORKSPACE, "test2_" + System.currentTimeMillis());
+    }
+    
+    @Override
+    public void tearDown() throws Exception
+    {
+        synchronized(endWait)
+        {
+            endWait.notifyAll();
+        }
     }
     
     public void testSetUp() throws Exception
     {
-        assertNotNull(nodeService);
+        assertNotNull(transactionService);
         assertNotNull(counter);
     }
     
     /**
      * Test nextVersionNumber
      */
-    public void testNextVersionNumber()
+    public void testNextVersionNumber() throws Exception
     {
-        // Create the store references
+        UserTransaction txn = transactionService.getUserTransaction();
-        StoreRef store1 = new StoreRef(StoreRef.PROTOCOL_WORKSPACE, VersionCounterDaoServiceTest.STORE_ID_1);
+        try
-        StoreRef store2 = new StoreRef(StoreRef.PROTOCOL_WORKSPACE, VersionCounterDaoServiceTest.STORE_ID_2);
+        {
-        StoreRef storeNone = new StoreRef(StoreRef.PROTOCOL_WORKSPACE, VersionCounterDaoServiceTest.STORE_NONE);
+            txn.begin();
             
-        int store1Version0 = this.counter.nextVersionNumber(store1);
+            int store1Version0 = counter.nextVersionNumber(storeRef1);
-        assertEquals(store1Version0, 1);
+            assertEquals(1, store1Version0);
             
-        int store1Version1 = this.counter.nextVersionNumber(store1);
+            int store1Version1 = counter.nextVersionNumber(storeRef1);
-        assertEquals(store1Version1, 2);
+            assertEquals(2, store1Version1);
             
-        int store2Version0 = this.counter.nextVersionNumber(store2);
+            int store2Version0 = counter.nextVersionNumber(storeRef2);
-        assertEquals(store2Version0, 1);
+            assertEquals(1, store2Version0);
             
-        int store1Version2 = this.counter.nextVersionNumber(store1);
+            int store1Version2 = counter.nextVersionNumber(storeRef1);
-        assertEquals(store1Version2, 3);
+            assertEquals(3, store1Version2);
             
-        int store2Version1 = this.counter.nextVersionNumber(store2);
+            int store2Version1 = counter.nextVersionNumber(storeRef2);
-        assertEquals(store2Version1, 2);
+            assertEquals(2, store2Version1);
             
-        int store1Current = this.counter.currentVersionNumber(store1);
+            int store1Current = counter.currentVersionNumber(storeRef1);
-        assertEquals(store1Current, 3);
+            assertEquals(3, store1Current);
             
-        int store2Current = this.counter.currentVersionNumber(store2);
+            int store2Current = counter.currentVersionNumber(storeRef2);
-        assertEquals(store2Current, 2);
+            assertEquals(2, store2Current);
-        
-        int storeNoneCurrent = this.counter.currentVersionNumber(storeNone);
-        assertEquals(storeNoneCurrent, 0);
             
             // Need to clean-up since the version counter works in its own transaction
-        this.counter.resetVersionNumber(store1);
+            counter.resetVersionNumber(storeRef1);
-        this.counter.resetVersionNumber(store2);
+            counter.resetVersionNumber(storeRef2);
+        }
+        finally
+        {
+            try { txn.rollback(); } catch (Throwable e) {}
+        }
     }
 
+    public void testConcurrentVersionNumber() throws Throwable
+    {
+        VersionCounterThread[] threads = new VersionCounterThread[5];
+        for (int i = 0; i < threads.length; i++)
+        {
+            threads[i] = new VersionCounterThread("VersionCounterThread_" + i);
+            // start the thread
+            threads[i].start();
+        }
+        // now wait until all the threads are waiting
+        int iteration = 0;
+        while (waitCount < threads.length && iteration++ < 5000)
+        {
+            synchronized (this)
+            {
+                this.wait(20);   // 20 ms wait
+            }
+        }
+        // reset wait count
+        this.waitCount = 0;
+        // kick them off
+        synchronized(beginWait)
+        {
+            beginWait.notifyAll();
+        }
+
+        // now wait until all the threads are waiting
+        while (waitCount < threads.length && iteration++ < 5000)
+        {
+            synchronized (this)
+            {
+                this.wait(20);   // 20 ms wait
+            }
+        }
+        // let them finish
+        iteration = 0;
+        synchronized(endWait)
+        {
+            endWait.notifyAll();
+        }
+        
+        // check for exceptions
+        for (VersionCounterThread thread : threads)
+        {
+            if (thread.error != null)
+            {
+                throw thread.error;
+            }
+        }
+    }
+
+    private Object beginWait = new String("BEGIN_WAIT");
+    private Object endWait = new String("END_WAIT");
+    private int waitCount = 0;
+    
+    private class VersionCounterThread extends Thread
+    {
+        private Throwable error = new RuntimeException("Execution didn't complete");
+        public VersionCounterThread(String name)
+        {
+            super(name);
+        }
+        
+        @Override
+        public void run()
+        {
+            TransactionWork<Object> versionWork = new TransactionWork<Object>()
+            {
+                public Object doWork() throws Exception
+                {
+                    // wait for all other threads to enter into their transactions
+                    synchronized(beginWait)
+                    {
+                        waitCount++;
+                        beginWait.wait(10000L);
+                    }
+                    
+                    int startVersion = counter.currentVersionNumber(storeRef1);
+                    // increment it
+                    int incrementedVersion = counter.nextVersionNumber(storeRef1);
+                    assertTrue("Version number was not incremented", incrementedVersion > startVersion);
+                    
+                    // wait for all other threads to have finished their increments
+                    synchronized(endWait)
+                    {
+                        waitCount++;
+                        endWait.wait(10000L);
+                    }
+                    
+                    return null;
+                }
+            };
+            try
+            {
+                TransactionUtil.executeInNonPropagatingUserTransaction(transactionService, versionWork, false);
+                error = null;
+            }
+            catch (Throwable e)
+            {
+                error = e;
+                e.printStackTrace();
+            }
+        }
+    }
 }

source/java/org/alfresco/repo/version/common/counter/hibernate/HibernateVersionCounterDaoServiceImpl.java

@@ -19,8 +19,14 @@ package org.alfresco.repo.version.common.counter.hibernate;
 import org.alfresco.repo.domain.StoreKey;
 import org.alfresco.repo.domain.VersionCount;
 import org.alfresco.repo.domain.hibernate.VersionCountImpl;
+import org.alfresco.repo.node.NodeServicePolicies;
+import org.alfresco.repo.policy.JavaBehaviour;
+import org.alfresco.repo.policy.PolicyComponent;
 import org.alfresco.repo.version.common.counter.VersionCounterDaoService;
 import org.alfresco.service.cmr.repository.StoreRef;
+import org.alfresco.service.namespace.NamespaceService;
+import org.alfresco.service.namespace.QName;
+import org.hibernate.LockMode;
 import org.springframework.orm.hibernate3.support.HibernateDaoSupport;
 
 /**
@@ -33,28 +39,93 @@ import org.springframework.orm.hibernate3.support.HibernateDaoSupport;
  * 
  * @author Derek Hulley
  */
-public class HibernateVersionCounterDaoServiceImpl extends HibernateDaoSupport implements VersionCounterDaoService
+public class HibernateVersionCounterDaoServiceImpl
+        extends HibernateDaoSupport
+        implements VersionCounterDaoService, NodeServicePolicies.BeforeCreateStorePolicy
 {
+    private PolicyComponent policyComponent;
+
     /**
-     * Retrieves or creates a version counter
+     * @param policyComponent the component to register behaviour
+     */
+    public void setPolicyComponent(PolicyComponent policyComponent)
+    {
+        this.policyComponent = policyComponent;
+    }
+    
+    /**
+     * Bind to receive notifications of store creations
+     */
+    public void init()
+    {
+        policyComponent.bindClassBehaviour(
+                QName.createQName(NamespaceService.ALFRESCO_URI, "beforeCreateStore"),
+                this,
+                new JavaBehaviour(this, "beforeCreateStore"));
+    }
+
+    /**
+     * Create a version counter for the store
+     * @param nodeTypeQName
+     * @param storeRef
+     */
+    public void beforeCreateStore(QName nodeTypeQName, StoreRef storeRef)
+    {
+        final StoreKey storeKey = new StoreKey(storeRef.getProtocol(), storeRef.getIdentifier());
+        VersionCount versionCount = (VersionCount) getHibernateTemplate().get(VersionCountImpl.class, storeKey);
+        if (versionCount != null)
+        {
+            // already exists
+            return;
+        }
+        versionCount = new VersionCountImpl();
+        versionCount.setKey(storeKey);
+        getHibernateTemplate().save(versionCount);
+    }
+
+    /**
+     * Retrieves or creates a version counter.  This locks the counter against updates for the
+     * current transaction.
      * 
-     * @param storeKey
+     * @param storeKey the primary key of the counter
      * @return Returns a current or new version counter
      */
     private VersionCount getVersionCounter(StoreRef storeRef)
     {
-        StoreKey storeKey = new StoreKey(storeRef.getProtocol(), storeRef.getIdentifier());
+        final StoreKey storeKey = new StoreKey(storeRef.getProtocol(), storeRef.getIdentifier());
-        // get the version counter
+        
-        VersionCount versionCounter = (VersionCount) getHibernateTemplate().get(VersionCountImpl.class, storeKey);
         // check if it exists
-        if (versionCounter == null)
+        VersionCount versionCount = (VersionCount) getHibernateTemplate().get(
+                VersionCountImpl.class,
+                storeKey,
+                LockMode.UPGRADE);
+        if (versionCount == null)
         {
-            // create a new one
+            // This could fail on some databases with concurrent adds.  But it is only those databases
-            versionCounter = new VersionCountImpl();
+            // that don't lock the index against an addition of the row, and then it will only fail once.
-            versionCounter.setKey(storeKey);
+            versionCount = new VersionCountImpl();
-            getHibernateTemplate().save(versionCounter);
+            versionCount.setKey(storeKey);
+            getHibernateTemplate().save(versionCount);
+            // debug
+            if (logger.isDebugEnabled())
+            {
+                logger.debug("Created version counter: \n" +
+                        "   Thread: " + Thread.currentThread().getName() + "\n" +
+                        "   Version count: " + versionCount.getVersionCount());
             }
-        return versionCounter;
+        }
+        else
+        {
+            // debug
+            if (logger.isDebugEnabled())
+            {
+                logger.debug("Got version counter: \n" +
+                        "   Thread: " + Thread.currentThread().getName() + "\n" +
+                        "   Version count: " + versionCount.getVersionCount());
+            }
+        }
+        // done
+        return versionCount;
     }
     
     /**
@@ -63,12 +134,21 @@ public class HibernateVersionCounterDaoServiceImpl extends HibernateDaoSupport i
      * @param storeRef  the version store id
      * @return          the next version number
      */
-    public synchronized int nextVersionNumber(StoreRef storeRef)
+    public int nextVersionNumber(StoreRef storeRef)
     {
         // get the version counter
-        VersionCount versionCounter = getVersionCounter(storeRef);
+        VersionCount versionCount = getVersionCounter(storeRef);
         // get an incremented count
-        return versionCounter.incrementVersionCount();
+        int nextCount = versionCount.incrementVersionCount();
+        
+        // done
+        if (logger.isDebugEnabled())
+        {
+            logger.debug("Incremented version count: \n" +
+                    "   Thread: " + Thread.currentThread().getName() + "\n" +
+                    "   New version count: " + versionCount.getVersionCount());
+        }
+        return nextCount;
     }
     
     /**
@@ -77,7 +157,7 @@ public class HibernateVersionCounterDaoServiceImpl extends HibernateDaoSupport i
      * @param storeRef  the store reference
      * @return          the current version number, zero if no version yet allocated.
      */
-    public synchronized int currentVersionNumber(StoreRef storeRef)
+    public int currentVersionNumber(StoreRef storeRef)
     {
         // get the version counter
         VersionCount versionCounter = getVersionCounter(storeRef);