[246456] NullPointerException in the revision cache

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

8 files changed, 159 insertions, 19 deletions

diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/revision/cache/CDORevisionCache.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/revision/cache/CDORevisionCache.java
index 1476c13510..73981773cc 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/revision/cache/CDORevisionCache.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/revision/cache/CDORevisionCache.java
@@ -37,6 +37,9 @@ public interface CDORevisionCache extends INotifier.Introspection
 
   public InternalCDORevision removeRevision(CDOID id, int version);
 
+  /**
+   * Returns a list of {@link CDORevision revisions} that are current.
+   */
   public List<CDORevision> getRevisions();
 
   public boolean addRevision(InternalCDORevision revision);
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionHolder.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionHolder.java
index 77f7b18757..eabebfb4aa 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionHolder.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionHolder.java
@@ -34,6 +34,11 @@ public class DLRevisionHolder extends RevisionHolder
     return dlList;
   }
 
+  protected void setDLList(DLRevisionList list)
+  {
+    dlList = list;
+  }
+
   public DLRevisionHolder getDLPrev()
   {
     return dlPrev;
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionList.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionList.java
index 92cc5b88e7..1c642df93c 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionList.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/DLRevisionList.java
@@ -49,6 +49,17 @@ public class DLRevisionList extends DLRevisionHolder
     return getDLPrev();
   }
 
+  @Override
+  protected void setDLList(DLRevisionList list)
+  {
+    if (getPrev() != null || getDLNext() != null || getDLList() != null)
+    {
+      throw new IllegalStateException("Cannot assign to a different list while linked to a list");
+    }
+
+    super.setDLList(list);
+  }
+
   public void setDLTail(DLRevisionHolder tail)
   {
     setDLPrev(tail);
@@ -85,28 +96,52 @@ public class DLRevisionList extends DLRevisionHolder
     addTail(holder);
   }
 
+  protected void validateUnlink(DLRevisionHolder holder)
+  {
+    if (holder.getDLList() != null)
+    {
+      throw new IllegalArgumentException("Holder " + holder + " is still linked in different list");
+    }
+  }
+
+  protected void validateLink(DLRevisionHolder holder)
+  {
+    if (holder.getDLList() != this)
+    {
+      throw new IllegalArgumentException("Holder " + holder + " does not belong to this list");
+    }
+  }
+
   public synchronized void addHead(DLRevisionHolder holder)
   {
+    validateUnlink(holder);
+
     ++size;
     DLRevisionHolder head = getDLHead();
     head.setDLPrev(holder);
     holder.setDLNext(head);
     holder.setDLPrev(this);
+    holder.setDLList(this);
     setDLHead(holder);
   }
 
   public synchronized void addTail(DLRevisionHolder holder)
   {
+    validateUnlink(holder);
+
     ++size;
     DLRevisionHolder tail = getDLTail();
     tail.setDLNext(holder);
     holder.setDLPrev(tail);
     holder.setDLNext(this);
+    holder.setDLList(this);
     setDLTail(holder);
   }
 
   public synchronized void remove(DLRevisionHolder holder)
   {
+    validateLink(holder);
+
     --size;
     DLRevisionHolder prev = holder.getDLPrev();
     DLRevisionHolder next = holder.getDLNext();
@@ -114,6 +149,7 @@ public class DLRevisionList extends DLRevisionHolder
     prev.setDLNext(next);
     holder.setDLPrev(null);
     holder.setDLNext(null);
+    holder.setDLList(null);
     next.setDLPrev(prev);
   }
 
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionCache.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionCache.java
index e62e0a470a..a0f89a4836 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionCache.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionCache.java
@@ -88,7 +88,7 @@ public class LRURevisionCache extends Lifecycle implements CDORevisionCache
 
   public List<CDORevision> getRevisions()
   {
-    ArrayList<CDORevision> currentRevisions = new ArrayList<CDORevision>();
+    List<CDORevision> currentRevisions = new ArrayList<CDORevision>();
     for (RevisionHolder holder : revisions.values())
     {
       InternalCDORevision revision = holder.getRevision();
@@ -97,6 +97,7 @@ public class LRURevisionCache extends Lifecycle implements CDORevisionCache
         currentRevisions.add(revision);
       }
     }
+
     return currentRevisions;
   }
 
@@ -236,6 +237,20 @@ public class LRURevisionCache extends Lifecycle implements CDORevisionCache
     return revision;
   }
 
+  public synchronized boolean removeCachedRevisions(CDOID id)
+  {
+    RevisionHolder lookupHolder = revisions.get(id);
+    RevisionHolder holder = lookupHolder;
+    while (holder != null)
+    {
+      RevisionHolder nextHolder = holder.getNext();
+      removeHolder(holder);
+      holder = nextHolder;
+    }
+
+    return lookupHolder != null;
+  }
+
   @Override
   protected void doActivate() throws Exception
   {
@@ -252,6 +267,11 @@ public class LRURevisionCache extends Lifecycle implements CDORevisionCache
     super.doDeactivate();
   }
 
+  protected RevisionHolder createHolder(InternalCDORevision revision)
+  {
+    return new LRURevisionHolder(revision);
+  }
+
   private void adjustHolder(InternalCDORevision revision, RevisionHolder holder, RevisionHolder prevHolder,
       RevisionHolder nextHolder)
   {
@@ -324,12 +344,6 @@ public class LRURevisionCache extends Lifecycle implements CDORevisionCache
     holder.setNext(null);
   }
 
-  private RevisionHolder createHolder(InternalCDORevision revision)
-  {
-    LRURevisionList list = revision.isCurrent() ? currentLRU : revisedLRU;
-    return new LRURevisionHolder(list, revision);
-  }
-
   /**
    * @author Eike Stepper
    */
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionHolder.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionHolder.java
index 0e91ec0f25..c031144fa6 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionHolder.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/revision/cache/lru/LRURevisionHolder.java
@@ -19,9 +19,9 @@ public class LRURevisionHolder extends DLRevisionHolder
 {
   private long usedStamp;
 
-  public LRURevisionHolder(LRURevisionList list, InternalCDORevision revision)
+  public LRURevisionHolder(InternalCDORevision revision)
   {
-    super(list, revision);
+    super(null, revision);
     usedStamp = System.currentTimeMillis();
   }
 
@@ -31,6 +31,19 @@ public class LRURevisionHolder extends DLRevisionHolder
     return (LRURevisionList)super.getDLList();
   }
 
+  @Override
+  protected void setDLList(DLRevisionList list)
+  {
+    if (list == null || list instanceof LRURevisionList)
+    {
+      super.setDLList(list);
+    }
+    else
+    {
+      throw new IllegalArgumentException("Not a " + LRURevisionList.class.getName() + ": " + list);
+    }
+  }
+
   public long getUsedStamp()
   {
     return usedStamp;
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTests.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTests.java
index d347185291..506cfeb3a2 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTests.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTests.java
@@ -13,6 +13,7 @@ package org.eclipse.emf.cdo.tests;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_241464_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_243310_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_246442_Test;
+import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_246456_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_246622_Test;
 
 import junit.framework.Test;
@@ -50,10 +51,6 @@ public class AllTests
     suite.addTestSuite(RevisionDeltaWithoutDeltaSupportTest.class);
     suite.addTestSuite(IndexReconstructionTest.class);
     suite.addTestSuite(NoLegacyTest.class);
-    suite.addTestSuite(Bugzilla_241464_Test.class);
-    suite.addTestSuite(Bugzilla_243310_Test.class);
-    suite.addTestSuite(Bugzilla_246622_Test.class);
-    suite.addTestSuite(Bugzilla_246442_Test.class);
     suite.addTestSuite(AutoAttacherTest.class);
     suite.addTestSuite(SavepointTest.class);
     suite.addTestSuite(ChangeSubscriptionTest.class);
@@ -64,6 +61,13 @@ public class AllTests
     // Specific for MEMStore
     suite.addTestSuite(QueryTest.class);
 
+    // Bugzilla verifications
+    suite.addTestSuite(Bugzilla_241464_Test.class);
+    suite.addTestSuite(Bugzilla_243310_Test.class);
+    suite.addTestSuite(Bugzilla_246442_Test.class);
+    suite.addTestSuite(Bugzilla_246456_Test.class);
+    suite.addTestSuite(Bugzilla_246622_Test.class);
+
     // TODO suite.addTestSuite(GeneratedEcoreTest.class);
     // $JUnit-END$
 
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/RevisionHolderTest.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/RevisionHolderTest.java
index f37e6d0a7b..5ba140d0f2 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/RevisionHolderTest.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/RevisionHolderTest.java
@@ -46,7 +46,7 @@ public class RevisionHolderTest extends AbstractOMTest
     validateList(list, 0);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addFirst(holder);
       list.addHead(holder);
       validateList(list, i + 1);
@@ -61,7 +61,7 @@ public class RevisionHolderTest extends AbstractOMTest
     validateList(list, 0);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addLast(holder);
       list.addTail(holder);
       validateList(list, i + 1);
@@ -75,7 +75,7 @@ public class RevisionHolderTest extends AbstractOMTest
     LRURevisionList list = new LRURevisionList(100);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addLast(holder);
       list.addTail(holder);
     }
@@ -95,7 +95,7 @@ public class RevisionHolderTest extends AbstractOMTest
     LRURevisionList list = new LRURevisionList(100);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addLast(holder);
       list.addTail(holder);
     }
@@ -115,7 +115,7 @@ public class RevisionHolderTest extends AbstractOMTest
     LRURevisionList list = new LRURevisionList(100);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addLast(holder);
       list.addTail(holder);
     }
@@ -135,7 +135,7 @@ public class RevisionHolderTest extends AbstractOMTest
     LRURevisionList list = new LRURevisionList(100);
     for (int i = 0; i < 10; i++)
     {
-      LRURevisionHolder holder = new LRURevisionHolder(list, new RevisionStub(i));
+      LRURevisionHolder holder = new LRURevisionHolder(new RevisionStub(i));
       linkedList.addLast(holder);
       list.addTail(holder);
     }
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_246456_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_246456_Test.java
new file mode 100644
index 0000000000..0fb7b53e7e
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_246456_Test.java
@@ -0,0 +1,65 @@
+/***************************************************************************
+ * Copyright (c) 2004 - 2008 Eike Stepper, Germany.
+ * 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:
+ *    Simon McDuff - initial API and implementation
+ **************************************************************************/
+package org.eclipse.emf.cdo.tests.bugzilla;
+
+import org.eclipse.emf.cdo.eresource.CDOResource;
+import org.eclipse.emf.cdo.internal.common.revision.cache.lru.LRURevisionCache;
+import org.eclipse.emf.cdo.internal.common.revision.cache.two.TwoLevelRevisionCache;
+import org.eclipse.emf.cdo.tests.AbstractCDOTest;
+import org.eclipse.emf.cdo.tests.model1.Model1Factory;
+import org.eclipse.emf.cdo.tests.model1.Supplier;
+
+import org.eclipse.emf.internal.cdo.CDOSessionImpl;
+import org.eclipse.emf.internal.cdo.CDOTransactionImpl;
+
+/**
+ * @author Simon McDuff
+ */
+public class Bugzilla_246456_Test extends AbstractCDOTest
+{
+  public void testBugzilla_246456() throws Exception
+  {
+    msg("Opening session");
+    CDOSessionImpl session = (CDOSessionImpl)openModel1Session();
+
+    msg("Opening transaction");
+    CDOTransactionImpl transaction = session.openTransaction();
+    ((LRURevisionCache)((TwoLevelRevisionCache)transaction.getSession().getRevisionManager().getCache()).getLevel1())
+        .setCapacityRevised(10);
+    ((LRURevisionCache)((TwoLevelRevisionCache)transaction.getSession().getRevisionManager().getCache()).getLevel1())
+        .setCapacityCurrent(10);
+    transaction.setUniqueResourceContents(false);
+
+    msg("Creating resource");
+    CDOResource resource = transaction.createResource("/test1");
+
+    msg("Creating supplier");
+    Supplier supplier = Model1Factory.eINSTANCE.createSupplier();
+
+    msg("Adding supplier");
+    resource.getContents().add(supplier);
+
+    msg("Committing");
+    transaction.commit();
+    for (int i = 0; i < 10; i++)
+    {
+      Supplier supplier2 = Model1Factory.eINSTANCE.createSupplier();
+      resource.getContents().add(supplier2);
+      transaction.commit();
+    }
+
+    Supplier supplier2 = Model1Factory.eINSTANCE.createSupplier();
+    resource.getContents().add(supplier2);
+
+    msg("Committing");
+    transaction.commit();
+  }
+}