[467174] Bad lock state with lock state and revision prefetch

https://bugs.eclipse.org/bugs/show_bug.cgi?id=467174

- Add Bugzilla_467174_Test JUnit test.
- Add fix in CDOViewImpl and AbstractCDOView.

Change-Id: Ic1ca47ac347a0909fd074bac53f222e57e25c9d8
Signed-off-by: Esteban Dugueperoux <esteban.dugueperoux@obeo.fr>
Signed-off-by: Maxime Porhel <maxime.porhel@obeo.fr>
Signed-off-by: Eike Stepper <stepper@esc-net.de>

3 files changed, 149 insertions, 25 deletions

diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_467174_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_467174_Test.java
new file mode 100644
index 0000000000..210bd72325
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_467174_Test.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015 Obeo and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *    Esteban Dugueperoux - initial API and implementation
+ */
+package org.eclipse.emf.cdo.tests.bugzilla;
+
+import org.eclipse.emf.cdo.CDOObject;
+import org.eclipse.emf.cdo.common.id.CDOID;
+import org.eclipse.emf.cdo.common.revision.CDORevision;
+import org.eclipse.emf.cdo.eresource.CDOResource;
+import org.eclipse.emf.cdo.session.CDOSession;
+import org.eclipse.emf.cdo.tests.AbstractCDOTest;
+import org.eclipse.emf.cdo.tests.model1.Category;
+import org.eclipse.emf.cdo.tests.model1.Company;
+import org.eclipse.emf.cdo.transaction.CDOTransaction;
+import org.eclipse.emf.cdo.util.CDOUtil;
+
+import org.eclipse.emf.internal.cdo.view.CDOViewImpl.OptionsImpl;
+
+import org.eclipse.net4j.util.concurrent.IRWLockManager.LockType;
+
+import org.eclipse.emf.common.notify.Notification;
+import org.eclipse.emf.common.notify.impl.AdapterImpl;
+import org.eclipse.emf.ecore.resource.ResourceSet;
+import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
+
+import org.junit.Assert;
+
+import java.util.Collections;
+
+/**
+ * Bug 467174 to test lock state and revision prefetch.
+ *
+ * @author Esteban Dugueperoux
+ */
+public class Bugzilla_467174_Test extends AbstractCDOTest
+{
+  public void testCDOObject_LockStateAndRevisionPrefetch() throws Exception
+  {
+    CDOSession session1 = openSession();
+    CDOTransaction transaction1 = session1.openTransaction();
+
+    OptionsImpl options = (OptionsImpl)transaction1.options();
+    options.setLockNotificationEnabled(true);
+    options.setLockStatePrefetchEnabled(true);
+
+    CDOResource resource1 = transaction1.createResource(getResourcePath("test1.model1"));
+    Company company = getModel1Factory().createCompany();
+    Category category = getModel1Factory().createCategory();
+    company.getCategories().add(category);
+    resource1.getContents().add(company);
+    resource1.save(Collections.emptyMap());
+
+    CDOObject companyCDOObject = CDOUtil.getCDOObject(company);
+    CDOID companyCDOID = companyCDOObject.cdoID();
+    CDOObject categoryCDOObject = CDOUtil.getCDOObject(category);
+    CDOID categoryCDOID = categoryCDOObject.cdoID();
+
+    CDOSession session2 = openSession();
+    ResourceSet resourceSet = new ResourceSetImpl();
+    resourceSet.eAdapters().add(new AdapterImpl()
+    {
+      @Override
+      public void notifyChanged(Notification msg)
+      {
+        Object newValue = msg.getNewValue();
+        if (newValue instanceof CDOResource)
+        {
+          ((CDOResource)newValue).cdoPrefetch(CDORevision.DEPTH_INFINITE);
+        }
+      }
+    });
+
+    CDOTransaction transaction2 = session2.openTransaction(resourceSet);
+    transaction2.options().setLockNotificationEnabled(true);
+    ((OptionsImpl)transaction2.options()).setLockStatePrefetchEnabled(true);
+
+    transaction1.lockObjects(Collections.singletonList(categoryCDOObject), LockType.WRITE, 1);
+    transaction1.lockObjects(Collections.singletonList(companyCDOObject), LockType.WRITE, 1);
+
+    CDOObject companyCDOObjectFromTx2 = transaction2.getObject(companyCDOID);
+    CDOObject categoryCDOObjectFromTx2 = transaction2.getObject(categoryCDOID);
+    Assert.assertTrue(companyCDOObjectFromTx2.cdoWriteLock().isLockedByOthers());
+    Assert.assertTrue(categoryCDOObjectFromTx2.cdoWriteLock().isLockedByOthers());
+  }
+}
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/AbstractCDOView.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/AbstractCDOView.java
index fbbfe8da88..64b46d5dff 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/AbstractCDOView.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/AbstractCDOView.java
@@ -167,6 +167,8 @@ public abstract class AbstractCDOView extends CDOCommitHistoryProviderImpl<CDOOb
 
   private Map<CDOID, InternalCDOObject> objects;
 
+  private int objectCreationCounter;
+
   private CDOStore store = new CDOStoreImpl(this);
 
   private CDOResourceImpl rootResource;
@@ -1824,32 +1826,44 @@ public abstract class AbstractCDOView extends CDOCommitHistoryProviderImpl<CDOOb
       TRACER.trace("Creating object for " + id); //$NON-NLS-1$
     }
 
+    // Remember the current object creation count to be able to optimize the map lookup
+    // away if no objects get created during the getRevision() call.
+    int originalCount = objectCreationCounter;
+
     InternalCDORevision revision = getRevision(id, true);
     if (revision == null)
     {
       throw new ObjectNotFoundException(id, this);
     }
 
-    EClass eClass = revision.getEClass();
-    InternalCDOObject object;
-    if (CDOModelUtil.isResource(eClass) && id != rootResourceID)
-    {
-      object = (InternalCDOObject)newResourceInstance(revision);
-      // object is PROXY
-    }
-    else
+    // If the object has been created and registered during revision loading
+    // (for example by lock state prefetcher) don't create a duplicate.
+    InternalCDOObject object = objectCreationCounter == originalCount ? null : objects.get(id);
+    if (object == null)
     {
-      object = newInstance(eClass);
-      // object is TRANSIENT
-    }
+      EClass eClass = revision.getEClass();
+
+      if (CDOModelUtil.isResource(eClass) && id != rootResourceID)
+      {
+        object = (InternalCDOObject)newResourceInstance(revision);
+        // object is PROXY
+      }
+      else
+      {
+        object = newInstance(eClass);
+        // object is TRANSIENT
+      }
 
-    cleanObject(object, revision);
-    CDOStateMachine.INSTANCE.dispatchLoadNotification(object);
+      ++objectCreationCounter;
 
-    // Bug 435198: Have object's resource added to the ResourceSet on call to CDOView.getObject(CDOID)
-    if (!CDOModelUtil.isResource(eClass))
-    {
-      getStore().getResource(object);
+      cleanObject(object, revision);
+      CDOStateMachine.INSTANCE.dispatchLoadNotification(object);
+
+      // Bug 435198: Have object's resource added to the ResourceSet on call to CDOView.getObject(CDOID)
+      if (!CDOModelUtil.isResource(eClass))
+      {
+        getStore().getResource(object);
+      }
     }
 
     return object;
diff --git a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
index 71a6e6c5cd..694efefc37 100644
--- a/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
+++ b/plugins/org.eclipse.emf.cdo/src/org/eclipse/emf/internal/cdo/view/CDOViewImpl.java
@@ -455,6 +455,18 @@ public class CDOViewImpl extends AbstractCDOView implements IExecutorServiceProv
    */
   protected void updateLockStates(CDOLockState[] newLockStates)
   {
+    updateLockStates(newLockStates, false);
+  }
+
+  /**
+   * Updates the lock states of objects held in this view
+   *
+   * @param newLockStates new {@link CDOLockState lockStates} to integrate in cache
+   * @param loadOnDemand true to load corresponding {@link CDOObject} if not already loaded to be able to store lockState in cache, false otherwise
+   * @since 4.5
+   */
+  protected void updateLockStates(CDOLockState[] newLockStates, boolean loadOnDemand)
+  {
     for (CDOLockState lockState : newLockStates)
     {
       Object lockedObject = lockState.getLockedObject();
@@ -478,7 +490,7 @@ public class CDOViewImpl extends AbstractCDOView implements IExecutorServiceProv
         throw new IllegalStateException("Unexpected: " + lockedObject.getClass().getSimpleName());
       }
 
-      InternalCDOObject object = getObject(id, false);
+      InternalCDOObject object = getObject(id, loadOnDemand);
       if (object != null)
       {
         InternalCDOLockState existingLockState = (InternalCDOLockState)lockStates.get(object);
@@ -2660,7 +2672,7 @@ public class CDOViewImpl extends AbstractCDOView implements IExecutorServiceProv
         CDOSessionProtocol sessionProtocol = session.getSessionProtocol();
         CDOLockState[] lockStates = sessionProtocol.getLockStates(viewID, ids, revisionsLoadedEvent.getPrefetchDepth());
 
-        updateLockStatesForAllViews(lockStates);
+        updateLockStatesForAllViews(lockStates, true);
 
         // add missing lock states
         List<CDOLockState> missingLockStates = new ArrayList<CDOLockState>();
@@ -2671,7 +2683,10 @@ public class CDOViewImpl extends AbstractCDOView implements IExecutorServiceProv
           if (object != null && CDOViewImpl.this.lockStates.get(object) == null)
           {
             Object lockedObject = getLockTarget(object); // CDOID or CDOIDAndBranch
-            missingLockStates.add(CDOLockUtil.createLockState(lockedObject));
+            if (lockedObject != null)
+            {
+              missingLockStates.add(CDOLockUtil.createLockState(lockedObject));
+            }
           }
         }
 
@@ -2685,24 +2700,27 @@ public class CDOViewImpl extends AbstractCDOView implements IExecutorServiceProv
             if (object != null && CDOViewImpl.this.lockStates.get(object) == null)
             {
               Object lockedObject = getLockTarget(object); // CDOID or CDOIDAndBranch
-              missingLockStates.add(CDOLockUtil.createLockState(lockedObject));
+              if (lockedObject != null)
+              {
+                missingLockStates.add(CDOLockUtil.createLockState(lockedObject));
+              }
             }
           }
         }
 
-        updateLockStatesForAllViews(missingLockStates.toArray(new CDOLockState[missingLockStates.size()]));
+        updateLockStatesForAllViews(missingLockStates.toArray(new CDOLockState[missingLockStates.size()]), false);
       }
     }
 
-    private void updateLockStatesForAllViews(CDOLockState[] lockStates)
+    private void updateLockStatesForAllViews(CDOLockState[] lockStates, boolean loadOnDemand)
     {
-      updateLockStates(lockStates);
+      updateLockStates(lockStates, loadOnDemand);
 
       for (CDOCommonView view : getSession().getViews())
       {
         if (view != CDOViewImpl.this && view.getBranch() == getBranch())
         {
-          updateLockStates(lockStates);
+          updateLockStates(lockStates, loadOnDemand);
         }
       }
     }