[353843] [DB] Duplicate entry / Violation of unique index in 'cdo_package_units_idx0'

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

7 files changed, 101 insertions, 16 deletions

diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/model/CDOPackageRegistryImpl.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/model/CDOPackageRegistryImpl.java
index 5bd33df87d..72b865d98d 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/model/CDOPackageRegistryImpl.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/internal/common/model/CDOPackageRegistryImpl.java
@@ -119,6 +119,8 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     this.packageLoader = packageLoader;
   }
 
+  static int lockCount;
+
   @Override
   public Object get(Object key)
   {
@@ -126,8 +128,9 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return super.get(key);
   }
 
-  public Set<String> getAllKeys()
+  public synchronized Set<String> getAllKeys()
   {
+    LifecycleUtil.checkActive(this);
     Set<String> result = new HashSet<String>();
     result.addAll(keySet());
     if (delegateRegistry != null)
@@ -145,8 +148,9 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return result;
   }
 
-  public Object getWithDelegation(String nsURI, boolean resolve)
+  public synchronized Object getWithDelegation(String nsURI, boolean resolve)
   {
+    LifecycleUtil.checkActive(this);
     Object result = getFrom(this, nsURI, resolve);
     if (result == null && delegateRegistry != null)
     {
@@ -166,7 +170,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return registry.get(nsURI);
   }
 
-  public Object basicPut(String nsURI, Object value)
+  public synchronized Object basicPut(String nsURI, Object value)
   {
     LifecycleUtil.checkActive(this);
     if (TRACER.isEnabled())
@@ -237,6 +241,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
 
   public synchronized Object putEPackage(EPackage ePackage)
   {
+    LifecycleUtil.checkActive(this);
     return put(ePackage.getNsURI(), ePackage);
   }
 
@@ -263,6 +268,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
 
   public synchronized void putPackageUnits(InternalCDOPackageUnit[] packageUnits, State state)
   {
+    LifecycleUtil.checkActive(this);
     for (InternalCDOPackageUnit packageUnit : packageUnits)
     {
       if (state != null)
@@ -339,8 +345,9 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return packageInfos;
   }
 
-  public InternalCDOPackageUnit getPackageUnit(EPackage ePackage)
+  public synchronized InternalCDOPackageUnit getPackageUnit(EPackage ePackage)
   {
+    LifecycleUtil.checkActive(this);
     CDOPackageInfo packageInfo = getPackageInfo(ePackage);
     if (packageInfo == null)
     {
@@ -383,7 +390,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return null;
   }
 
-  public InternalCDOPackageUnit[] getPackageUnits(long startTime, long endTime)
+  public synchronized InternalCDOPackageUnit[] getPackageUnits(long startTime, long endTime)
   {
     LifecycleUtil.checkActive(this);
     if (endTime == CDOBranchPoint.UNSPECIFIED_DATE)
@@ -421,7 +428,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return result.toArray(new InternalCDOPackageUnit[result.size()]);
   }
 
-  public InternalCDOPackageUnit[] getPackageUnits(boolean withSystemPackages)
+  public synchronized InternalCDOPackageUnit[] getPackageUnits(boolean withSystemPackages)
   {
     LifecycleUtil.checkActive(this);
     return collectPackageUnits(withSystemPackages);
@@ -490,8 +497,9 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
     return result.toArray(new EPackage[result.size()]);
   }
 
-  public EEnumLiteral getEnumLiteralFor(Enumerator value)
+  public synchronized EEnumLiteral getEnumLiteralFor(Enumerator value)
   {
+    LifecycleUtil.checkActive(this);
     EEnumLiteral result = enumLiterals.get(value);
     if (result != null)
     {
@@ -541,6 +549,7 @@ public class CDOPackageRegistryImpl extends EPackageRegistryImpl implements Inte
 
   public synchronized Map<EClass, List<EClass>> getSubTypes()
   {
+    LifecycleUtil.checkActive(this);
     if (subTypes == null)
     {
       subTypes = CDOModelUtil.getSubTypes(this);
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageInfo.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageInfo.java
index b6e3bc1b48..c1ff2302f9 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageInfo.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageInfo.java
@@ -21,6 +21,8 @@ import java.io.IOException;
 /**
  * @author Eike Stepper
  * @since 2.0
+ * @noextend This interface is not intended to be extended by clients.
+ * @noimplement This interface is not intended to be implemented by clients.
  */
 public interface InternalCDOPackageInfo extends CDOPackageInfo, Adapter.Internal
 {
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageRegistry.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageRegistry.java
index 3a73cf3400..a891faf885 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageRegistry.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageRegistry.java
@@ -23,6 +23,8 @@ import java.util.Set;
 /**
  * @author Eike Stepper
  * @since 2.0
+ * @noextend This interface is not intended to be extended by clients.
+ * @noimplement This interface is not intended to be implemented by clients.
  */
 public interface InternalCDOPackageRegistry extends CDOPackageRegistry, ILifecycle
 {
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageUnit.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageUnit.java
index 2ac4ea8719..9ea712595f 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageUnit.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/spi/common/model/InternalCDOPackageUnit.java
@@ -23,6 +23,8 @@ import java.io.IOException;
 /**
  * @author Eike Stepper
  * @since 2.0
+ * @noextend This interface is not intended to be extended by clients.
+ * @noimplement This interface is not intended to be implemented by clients.
  */
 public interface InternalCDOPackageUnit extends CDOPackageUnit
 {
diff --git a/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/Repository.java b/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/Repository.java
index 404b516040..711bbd08b1 100644
--- a/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/Repository.java
+++ b/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/Repository.java
@@ -109,6 +109,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
+import java.util.concurrent.Semaphore;
 
 /**
  * @author Eike Stepper
@@ -136,6 +137,11 @@ public class Repository extends Container<Object> implements InternalRepository
 
   private boolean ensuringReferentialIntegrity;
 
+  /**
+   * Must not be thread-bound to support XA commits.
+   */
+  private Semaphore packageRegistryCommitLock = new Semaphore(1);
+
   private InternalCDOPackageRegistry packageRegistry;
 
   private InternalCDOBranchManager branchManager;
@@ -648,6 +654,11 @@ public class Repository extends Container<Object> implements InternalRepository
     return packageRegistry;
   }
 
+  public Semaphore getPackageRegistryCommitLock()
+  {
+    return packageRegistryCommitLock;
+  }
+
   public InternalCDOPackageRegistry getPackageRegistry()
   {
     return getPackageRegistry(true);
diff --git a/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/TransactionCommitContext.java b/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/TransactionCommitContext.java
index 3cef064445..c8d220e616 100644
--- a/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/TransactionCommitContext.java
+++ b/plugins/org.eclipse.emf.cdo.server/src/org/eclipse/emf/cdo/internal/server/TransactionCommitContext.java
@@ -67,6 +67,7 @@ import org.eclipse.net4j.util.StringUtil;
 import org.eclipse.net4j.util.collection.IndexedList;
 import org.eclipse.net4j.util.concurrent.IRWLockManager.LockType;
 import org.eclipse.net4j.util.io.ExtendedDataInputStream;
+import org.eclipse.net4j.util.lifecycle.LifecycleUtil;
 import org.eclipse.net4j.util.om.monitor.Monitor;
 import org.eclipse.net4j.util.om.monitor.OMMonitor;
 
@@ -96,7 +97,7 @@ public class TransactionCommitContext implements InternalCommitContext
 {
   private static final InternalCDORevision DETACHED = new StubCDORevision(null);
 
-  private InternalTransaction transaction;
+  private final InternalTransaction transaction;
 
   private InternalRepository repository;
 
@@ -104,6 +105,10 @@ public class TransactionCommitContext implements InternalCommitContext
 
   private InternalLockManager lockManager;
 
+  private InternalCDOPackageRegistry repositoryPackageRegistry;
+
+  private boolean packageRegistryLocked;
+
   private TransactionPackageRegistry packageRegistry;
 
   private IStoreAccessor accessor;
@@ -157,7 +162,8 @@ public class TransactionCommitContext implements InternalCommitContext
     lockManager = repository.getLockManager();
     ensuringReferentialIntegrity = repository.isEnsuringReferentialIntegrity();
 
-    packageRegistry = new TransactionPackageRegistry(repository.getPackageRegistry(false));
+    repositoryPackageRegistry = repository.getPackageRegistry(false);
+    packageRegistry = new TransactionPackageRegistry(repositoryPackageRegistry);
     packageRegistry.activate();
   }
 
@@ -403,6 +409,28 @@ public class TransactionCommitContext implements InternalCommitContext
       monitor.begin(107);
       dirtyObjects = new InternalCDORevision[dirtyObjectDeltas.length];
 
+      if (newPackageUnits.length != 0 && repository instanceof Repository)
+      {
+        ((Repository)repository).getPackageRegistryCommitLock().acquire();
+        packageRegistryLocked = true;
+
+        List<InternalCDOPackageUnit> noDuplicates = new ArrayList<InternalCDOPackageUnit>();
+        for (InternalCDOPackageUnit newPackageUnit : newPackageUnits)
+        {
+          String id = newPackageUnit.getID();
+          if (!repositoryPackageRegistry.containsKey(id))
+          {
+            noDuplicates.add(newPackageUnit);
+          }
+        }
+
+        int newSize = noDuplicates.size();
+        if (newPackageUnits.length != newSize)
+        {
+          newPackageUnits = noDuplicates.toArray(new InternalCDOPackageUnit[newSize]);
+        }
+      }
+
       lockObjects(); // Can take long and must come before setTimeStamp()
       monitor.worked();
 
@@ -415,14 +443,13 @@ public class TransactionCommitContext implements InternalCommitContext
 
       checkXRefs();
       monitor.worked();
-      if (rollbackMessage != null)
+
+      if (rollbackMessage == null)
       {
-        return;
+        detachObjects(monitor.fork());
+        repository.notifyWriteAccessHandlers(transaction, this, true, monitor.fork());
+        accessor.write(this, monitor.fork(100));
       }
-
-      detachObjects(monitor.fork());
-      repository.notifyWriteAccessHandlers(transaction, this, true, monitor.fork());
-      accessor.write(this, monitor.fork(100));
     }
     catch (Throwable t)
     {
@@ -529,6 +556,11 @@ public class TransactionCommitContext implements InternalCommitContext
 
   public void postCommit(boolean success)
   {
+    if (packageRegistryLocked)
+    {
+      ((Repository)repository).getPackageRegistryCommitLock().release();
+    }
+
     try
     {
       InternalSession sender = transaction.getSession();
@@ -643,7 +675,6 @@ public class TransactionCommitContext implements InternalCommitContext
 
     try
     {
-
       final boolean supportingBranches = repository.isSupportingBranches();
 
       CDOFeatureDeltaVisitor deltaTargetLocker = null;
@@ -1135,6 +1166,7 @@ public class TransactionCommitContext implements InternalCommitContext
     @Override
     public synchronized void putPackageUnit(InternalCDOPackageUnit packageUnit)
     {
+      LifecycleUtil.checkActive(this);
       packageUnit.setPackageRegistry(this);
       for (InternalCDOPackageInfo packageInfo : packageUnit.getPackageInfos())
       {
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
index b20bc5e43d..2cbfef92c2 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
@@ -585,6 +585,33 @@ public class PackageRegistryTest extends AbstractCDOTest
     }
   }
 
+  /**
+   * Bug 353246.
+   * <p>
+   * Cannot reproduce the problem with MEMStore because MEMStoreAccessor.writePackageUnits() is empty.
+   */
+  @CleanRepositoriesBefore
+  public void testConcurrentPackageRegistration2() throws Exception
+  {
+    CDOSession session1 = openSession();
+    CDOTransaction transaction1 = session1.openTransaction();
+    CDOResource resource1 = transaction1.createResource(getResourcePath("/res1"));
+    transaction1.commit();
+
+    CDOSession session2 = openSession();
+    CDOTransaction transaction2 = session2.openTransaction();
+    CDOResource resource2 = transaction2.createResource(getResourcePath("/res2"));
+    transaction2.commit();
+
+    session2.options().setPassiveUpdateEnabled(false);
+
+    resource1.getContents().add(getModel1Factory().createCompany());
+    transaction1.commit();
+
+    resource2.getContents().add(getModel1Factory().createCompany());
+    transaction2.commit();
+  }
+
   @CleanRepositoriesBefore
   public void testPopulator() throws Exception
   {