CacheKey NPE and DeadLock

Signed-off-by: Radek Felcman <[email protected]>

Author: rfelcman

foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/junit/helper/ReadLockAcquisitionMetadataTest.java

@@ -0,0 +1,196 @@
+/*
+ * -----------------------------------------------------------------------------
+ * manually
+ * Removed text manually
+ * Removed text manually
+ * -----------------------------------------------------------------------------
+ */
+package org.eclipse.persistence.testing.tests.junit.helper;
+
+import java.util.List;
+
+import org.eclipse.persistence.internal.helper.ConcurrencyManager;
+import org.eclipse.persistence.internal.helper.ReadLockManager;
+import org.eclipse.persistence.internal.helper.type.ReadLockAcquisitionMetadata;
+import org.eclipse.persistence.internal.identitymaps.CacheKey;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Unit test for the class {@link org.eclipse.persistence.internal.helper.ReadLockManager}
+ */
+public class ReadLockAcquisitionMetadataTest {
+
+    // Inner class
+    /**
+     * Unit test extension class to allows to create some hacky methods to access
+     * and manipulate the state of the read lock manager.
+     * Such as adding NULL records into the state.
+     */
+   public static class ReadLockManagerExtension extends ReadLockManager{
+
+       /**
+        * This is a dummy method hack our state maps with null entries.
+        * Our expectation is that this should never happen in real life
+        * but we have seen null pointer exceptions that seem to indicate that this is factuallz posisbly to happen
+        * although we have understoor the underlying reason for the null entries.
+        *
+        * With this method we simulate corrupting our read data structures with null entries.
+        */
+       public void hackReadLockManagerToBeCorruptedWithNullRecordsItsState() {
+           ConcurrencyManager cacheKeyNull = null;
+           readLocks.add(cacheKeyNull);
+           final Thread currentThread = Thread.currentThread();
+           final long currentThreadId = currentThread.getId();
+           ReadLockAcquisitionMetadata readLockAcquisitionMetadataNull = null;
+           List<ReadLockAcquisitionMetadata> readLocksAcquiredDuringCurrentThreadList = mapThreadToReadLockAcquisitionMetadata.get(currentThreadId);
+           readLocksAcquiredDuringCurrentThreadList.add(readLockAcquisitionMetadataNull);
+
+       }
+
+
+   }
+   // helpver variables
+
+   final ConcurrencyManager cacheKeyA = new CacheKey("cacheKeyA");
+   final ConcurrencyManager cacheKeyB = new CacheKey("cacheKeyB");
+
+
+   @Before
+   public void before() {
+
+   }
+
+
+   @Test
+   public void normalHappyPathLogicAddingAndRemovingMetadataIntoTheReadLockManager() {
+       // SETUP:
+       // basic variable initialization
+       ReadLockManagerExtension testee = new ReadLockManagerExtension();
+
+
+       // EXECUTE 1 - Add cache key A
+       testee.addReadLock(cacheKeyA);
+
+       // VERIFY 1
+
+       Assert.assertEquals(1, testee.getReadLocks().size());
+       Assert.assertTrue(testee.getReadLocks().contains(cacheKeyA));
+       Assert.assertFalse(testee.getReadLocks().contains(cacheKeyB));
+
+       Assert.assertEquals(1, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+       List<ReadLockAcquisitionMetadata> cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+       Assert.assertEquals(1, cacheKeyMetadataForCurrentThread.size());
+
+       Assert.assertTrue(cacheKeyA == cacheKeyMetadataForCurrentThread.get(0).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+       Assert.assertFalse(cacheKeyB == cacheKeyMetadataForCurrentThread.get(0).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+
+
+       // EXECUTE 2 - Add cache key B
+       testee.addReadLock(cacheKeyB);
+
+       // VERIFY 2
+
+       Assert.assertEquals(2, testee.getReadLocks().size());
+       Assert.assertTrue(testee.getReadLocks().contains(cacheKeyA));
+       Assert.assertTrue(testee.getReadLocks().contains(cacheKeyB));
+
+       Assert.assertEquals(1, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+       cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+       Assert.assertEquals(2, cacheKeyMetadataForCurrentThread.size());
+
+       // note: when we are adding, we are adding the entries to the HEAD of the list
+       Assert.assertTrue(cacheKeyB == cacheKeyMetadataForCurrentThread.get(0).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+       Assert.assertTrue(cacheKeyA == cacheKeyMetadataForCurrentThread.get(1).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+
+
+       // EXECUTE 3 - Remove cache keys
+       testee.removeReadLock(cacheKeyA);
+
+       // VERIFY 3
+       Assert.assertEquals(1, testee.getReadLocks().size());
+       Assert.assertFalse(testee.getReadLocks().contains(cacheKeyA));
+       Assert.assertTrue(testee.getReadLocks().contains(cacheKeyB));
+
+       Assert.assertEquals(1, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+       cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+       Assert.assertEquals(1, cacheKeyMetadataForCurrentThread.size());
+
+       Assert.assertTrue(cacheKeyB == cacheKeyMetadataForCurrentThread.get(0).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+       Assert.assertFalse(cacheKeyA == cacheKeyMetadataForCurrentThread.get(0).getCacheKeyWhoseNumberOfReadersThreadIsIncrementing());
+
+       // EXECUTE 4 - Remove cache keys
+       testee.removeReadLock(cacheKeyB);
+
+       // VERIFY 4
+       Assert.assertEquals(0, testee.getReadLocks().size());
+       Assert.assertFalse(testee.getReadLocks().contains(cacheKeyA));
+       Assert.assertFalse(testee.getReadLocks().contains(cacheKeyB));
+       Assert.assertEquals(0, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+
+
+   }
+
+   @Test
+   public void testAddNullReadCacheKeyDoesNothing() {
+       // SETUP:
+       // basic variable initialization
+       ReadLockManagerExtension testee = new ReadLockManagerExtension();
+       ConcurrencyManager cacheKeyNull = null;
+
+       // EXECUTE
+       // try to add a null cache key to the map
+       testee.addReadLock(cacheKeyNull);
+
+       // VERIFY
+       Assert.assertEquals(0, testee.getReadLocks().size());
+       Assert.assertEquals(0, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+   }
+
+   /**
+    * The purpose of this unit test is to make sure that if for some unknown reason
+    * we ever end up adding NULL as metadata to either the READ Lock manager
+    * or to the VECTOR of read locks
+    * that we can self heald and remove them from the map automatically.
+    *
+    */
+    @Test
+    public void testRemoveWhen_mapThreadToReadLockAcquisitionMetadata_containsNull() {
+        // SETUP:
+        // let us start by adding some entrires to the read lock manager
+        ReadLockManagerExtension testee = new ReadLockManagerExtension();
+        testee.addReadLock(cacheKeyA);
+        testee.addReadLock(cacheKeyB);
+        Assert.assertEquals(2, testee.getReadLocks().size());
+        Assert.assertEquals(1, testee.getMapThreadToReadLockAcquisitionMetadata().size());
+        List<ReadLockAcquisitionMetadata> cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+        Assert.assertEquals(2, cacheKeyMetadataForCurrentThread.size());
+
+        // SETUP:
+        // now we are going to hack our state to put in here null entires both in the read locks map and in the list of metadata
+        // Validate that that the maps are now properly hacked
+        testee.hackReadLockManagerToBeCorruptedWithNullRecordsItsState();
+        Assert.assertEquals(3, testee.getReadLocks().size());
+        Assert.assertTrue( testee.getReadLocks().contains(null));
+        cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+        Assert.assertEquals(3, cacheKeyMetadataForCurrentThread.size());
+        Assert.assertTrue( cacheKeyMetadataForCurrentThread.contains(null));
+
+        // EXECUTE
+        // Now we will try to REMOVE from the read lock manager a cache key that does not actually exist in the map
+        // this MUST have the side effect of causing the code to loop over ALL OF THE read lock manager metadata and spot that we
+        // have NULL records in the metadata array
+        // in so doing the code should self-heal and get rid of all that garbage
+        ConcurrencyManager cacheKeyNotExistingInTheReadLockManagerToCallFullLoopOverData = new CacheKey("cacheKeyNotExistingInTheReadLockManagerToCallFullLoopOverData");
+        testee.removeReadLock(cacheKeyNotExistingInTheReadLockManagerToCallFullLoopOverData);
+
+        // VERIFY that our code self healeded
+        Assert.assertEquals(2, testee.getReadLocks().size());
+        Assert.assertFalse( testee.getReadLocks().contains(null));
+        cacheKeyMetadataForCurrentThread =testee.getMapThreadToReadLockAcquisitionMetadata().get(Thread.currentThread().getId());
+        Assert.assertEquals(2, cacheKeyMetadataForCurrentThread.size());
+        Assert.assertFalse( cacheKeyMetadataForCurrentThread.contains(null));
+
+    }
+}

foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/config/PersistenceUnitProperties.java

@@ -55,9 +55,11 @@
 
 import java.io.File;
 import java.sql.DriverManager;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
@@ -3837,7 +3839,7 @@ public class PersistenceUnitProperties {
      * An NoSQL datasource is a non-relational datasource such as a legacy database, NoSQL database,
      * XML database, transactional and messaging systems, or ERP systems.
      *
-     * @see javax.resource.cci.ConnectionFactory
+     * {@code javax.resource.cci.ConnectionFactory}
      */
     public static final String NOSQL_CONNECTION_FACTORY = "eclipselink.nosql.connection-factory";
 
@@ -4051,6 +4053,48 @@ public class PersistenceUnitProperties {
      */
     public static final String CONCURRENCY_MANAGER_ALLOW_INTERRUPTED_EXCEPTION  = "eclipselink.concurrency.manager.allow.interruptedexception";
 
+
+    /**
+     * This is the persistence.xml property that tells us how the
+     * org.eclipse.persistence.internal.sessions.AbstractSession.getCacheKeyFromTargetSessionForMergeScenarioMergeManagerNotNullAndCacheKeyOriginalStillNull(CacheKey, Object, ObjectBuilder, ClassDescriptor, MergeManager)
+     * should behave.
+     */
+    public static final String CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION  = "eclipselink.concurrency.manager.allow.abstractsession.getCacheKeyFromTargetSessionForMerge.modeofoperation";
+
+
+    //TODO RFELCMAN enum - begin?
+    /**
+     * This should be the worst possible option. This is the one that gives us the issue:
+     *  https://github.com/eclipse-ee4j/eclipselink/issues/2094.
+     */
+    public static final String CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_BACKWARDS_COMPATIBILITY  = "backwards-compatibility";
+    /**
+     * This is far from an ideal approach but already better than the old backwards-compatibility approach.
+     * In  this code path what will happen is that our merge manger thread will be doing a loop hoping for a cache key
+     * acquired by some other thread releases the cache key.
+     * If that never happens then our merge manager will blow up.
+     * The assumption being that it might be the holder of cache keys that are stopping the processes from going forward.
+     * That is what we saw in the issue  https://github.com/eclipse-ee4j/eclipselink/issues/2094
+     * where our merge manager thread was hold several write lock keys that were making it impossible for other threads doing object building
+     * to finish their work.
+     */
+    public static final String CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_WAIT_LOOP_WITH_TIME_LIMIT  = "wait-loop-with-time-limit";
+    //TODO RFELCMAN enum - end?
+
+
+    public static final String CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_DEFAULT  = CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_WAIT_LOOP_WITH_TIME_LIMIT;
+
+    /**
+     * As the supported options for the {@link org.eclipse.persistence.config.PersistenceUnitProperties.CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION}
+     * persistence unit property.
+     */
+    public static final List<String> CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_LIST_OF_ALL_OPTIONS =  Collections.unmodifiableList( Arrays.asList(
+            PersistenceUnitProperties.CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_BACKWARDS_COMPATIBILITY
+            , PersistenceUnitProperties.CONCURRENCY_MANAGER_ALLOW_ABSTRACT_SESSION_GET_CACHE_KEY_FOR_MERGE_MANAGER_MODE_OF_OPERATION_OPTION_WAIT_LOOP_WITH_TIME_LIMIT
+            ));
+
+
+
     /**
      * <p>
      * This property control (enable/disable) if <code>ConcurrencyException</code> fired when dead-lock diagnostic is enabled.

foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/helper/ConcurrencyManager.java

@@ -18,7 +18,17 @@
 import java.io.Serializable;
 import java.io.StringWriter;
 import java.security.AccessController;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.IdentityHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.Vector;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -853,7 +863,7 @@ public void releaseAllLocksAcquiredByThread(DeferredLockManager lockManager) {
      * @return Never null if the read lock manager does not yet exist for the current thread. otherwise its read log
      *         manager is returned.
      */
-    protected static ReadLockManager getReadLockManager(Thread thread) {
+    public static ReadLockManager getReadLockManager(Thread thread) {
         Map<Thread, ReadLockManager> readLockManagers = getReadLockManagers();
         return readLockManagers.get(thread);
     }
@@ -1107,4 +1117,31 @@ public Lock getInstanceLock() {
     public Condition getInstanceLockCondition() {
         return this.instanceLockCondition;
     }
+
+    /**
+     * We check if cache keys is currently being owned for writing and if that owning thread happens to be the current thread doing the check.
+     * @return FALSE means either the thread is currently not owned by anybody for writing purposes. Or otherwise if is owned by some thread
+     * but the thread is not the current thread. TRUE is returned if and only if the cache key is being owned by some thread
+     * and that thread is not the current thread, it is some other competing thread.
+     */
+    public synchronized boolean isAcquiredForWritingAndOwneddByADifferentThread() {
+        // (a) We start by using the traditional acquire implementation to check if cache key is acquired
+        // if the output says false we immediately return false
+        if(!this.isAcquired()) {
+            return false;
+        }
+
+        // (b) If the active thread is not set then the cache keys is not acquired for writing by anybody
+        if(this.activeThread == null) {
+            return false;
+        }
+
+
+
+        // (c) some thread is acquiring the cache key
+        // return need to check if it is a different thread than ours
+        Thread currentThread = Thread.currentThread();
+        return this.activeThread != currentThread;
+
+    }
 }