[openjpa] branch 2.2.x updated: OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[openjpa] branch 2.2.x updated: OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition

dazeydev-2
This is an automated email from the ASF dual-hosted git repository.

dazeydev pushed a commit to branch 2.2.x
in repository https://gitbox.apache.org/repos/asf/openjpa.git


The following commit(s) were added to refs/heads/2.2.x by this push:
     new 29b82b2  OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition
     new a41cd39  Merge pull request #43 from dazey3/2767_2.2.x
29b82b2 is described below

commit 29b82b23efa1db5024706851063c1988fb130398
Author: Will Dazey <[hidden email]>
AuthorDate: Tue May 14 14:32:06 2019 -0500

    OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition
   
    Signed-off-by: Will Dazey <[hidden email]>
---
 .../meta/strats/ValueMapDiscriminatorStrategy.java | 83 ++++++++++++++++------
 .../apache/openjpa/meta/MetaDataRepository.java    | 42 +++++------
 2 files changed, 85 insertions(+), 40 deletions(-)

diff --git a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/ValueMapDiscriminatorStrategy.java b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/ValueMapDiscriminatorStrategy.java
index 86f8320..c14db3f 100644
--- a/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/ValueMapDiscriminatorStrategy.java
+++ b/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/ValueMapDiscriminatorStrategy.java
@@ -24,6 +24,8 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import org.apache.openjpa.jdbc.kernel.JDBCStore;
 import org.apache.openjpa.jdbc.meta.ClassMapping;
@@ -48,7 +50,10 @@ public class ValueMapDiscriminatorStrategy
     private static final Localizer _loc = Localizer.forPackage
         (ValueMapDiscriminatorStrategy.class);
 
-    private Map _vals = null;
+    private Map<String, Class<?>> _vals;
+    private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(true);
+    private final Lock _readLock = rwl.readLock();
+    private final Lock _writeLock = rwl.writeLock();
 
     public String getAlias() {
         return ALIAS;
@@ -62,8 +67,8 @@ public class ValueMapDiscriminatorStrategy
         // if the user wants the type to be null, we need a jdbc-type
         // on the column or an existing column to figure out the java type
         DiscriminatorMappingInfo info = disc.getMappingInfo();
-        List cols = info.getColumns();
-        Column col = (cols.isEmpty()) ? null : (Column) cols.get(0);
+        List<Column> cols = info.getColumns();
+        Column col = (cols.isEmpty()) ? null : cols.get(0);
         if (col != null) {
             if (col.getJavaType() != JavaTypes.OBJECT)
                 return col.getJavaType();
@@ -81,29 +86,67 @@ public class ValueMapDiscriminatorStrategy
 
     protected Class getClass(Object val, JDBCStore store)
         throws ClassNotFoundException {
-        if (_vals == null) {
-            ClassMapping cls = disc.getClassMapping();
-            ClassMapping[] subs = cls.getJoinablePCSubclassMappings();
-            Map map = new HashMap((int) ((subs.length + 1) * 1.33 + 1));
-            mapDiscriminatorValue(cls, map);
-            for (int i = 0; i < subs.length; i++)
-                mapDiscriminatorValue(subs[i], map);
-            _vals = map;
+
+        if(_vals == null) {
+            _writeLock.lock();
+            try {
+                if(_vals == null) {
+                    _vals = constructCache(disc);
+                }
+            } finally {
+                _writeLock.unlock();
+            }
+        }
+
+        String className = (val == null) ? null : val.toString();
+        _readLock.lock();
+        try {
+            Class<?> clz = _vals.get(className);
+            if (clz != null)
+                return clz;
+        } finally {
+            _readLock.unlock();
+        }
+
+        _writeLock.lock();
+        try {
+            Class<?> clz = _vals.get(className);
+            if (clz != null)
+                return clz;
+
+            //Rebuild the cache to check for updates
+            _vals = constructCache(disc);
+
+            //Try get again
+            clz = _vals.get(className);
+            if (clz != null)
+                return clz;
+            throw new ClassNotFoundException(_loc.get("unknown-discrim-value",
+                    new Object[]{ className, disc.getClassMapping().getDescribedType().
+                    getName(), new TreeSet<String>(_vals.keySet()) }).getMessage());
+        } finally {
+            _writeLock.unlock();
         }
+    }
 
-        String str = (val == null) ? null : val.toString();
-        Class cls = (Class) _vals.get(str);
-        if (cls != null)
-            return cls;
-        throw new ClassNotFoundException(_loc.get("unknown-discrim-value",
-            new Object[]{ str, disc.getClassMapping().getDescribedType().
-            getName(), new TreeSet(_vals.keySet()) }).getMessage());
+    /**
+     * Build a class cache map from the discriminator
+     */
+    private static Map<String, Class<?>> constructCache(Discriminator disc) {
+        //Build the cache map
+        ClassMapping cls = disc.getClassMapping();
+        ClassMapping[] subs = cls.getJoinablePCSubclassMappings();
+        Map<String, Class<?>> map = new HashMap<String, Class<?>>((int) ((subs.length + 1) * 1.33 + 1));
+        mapDiscriminatorValue(cls, map);
+        for (int i = 0; i < subs.length; i++)
+            mapDiscriminatorValue(subs[i], map);
+        return map;
     }
 
     /**
      * Map the stringified version of the discriminator value of the given type.
      */
-    private static void mapDiscriminatorValue(ClassMapping cls, Map map) {
+    private static void mapDiscriminatorValue(ClassMapping cls, Map<String, Class<?>> map) {
         // possible that some types will never be persisted and therefore
         // can have no discriminator value
         Object val = cls.getDiscriminator().getValue();
@@ -111,7 +154,7 @@ public class ValueMapDiscriminatorStrategy
             return;
 
         String str = (val == Discriminator.NULL) ? null : val.toString();
-        Class exist = (Class) map.get(str);
+        Class<?> exist = map.get(str);
         if (exist != null)
             throw new MetaDataException(_loc.get("dup-discrim-value",
                 str, exist, cls));
diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/meta/MetaDataRepository.java b/openjpa-kernel/src/main/java/org/apache/openjpa/meta/MetaDataRepository.java
index b631e3d..37c8a6b 100644
--- a/openjpa-kernel/src/main/java/org/apache/openjpa/meta/MetaDataRepository.java
+++ b/openjpa-kernel/src/main/java/org/apache/openjpa/meta/MetaDataRepository.java
@@ -124,7 +124,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
     private Map<Class<?>, Class<?>> _metamodel = Collections.synchronizedMap(new HashMap<Class<?>, Class<?>>());
 
     // map of classes to lists of their subclasses
-    private Map<Class<?>, List<Class<?>>> _subs = Collections.synchronizedMap(new HashMap<Class<?>, List<Class<?>>>());
+    private Map<Class<?>, Collection<Class<?>>> _subs =
+        Collections.synchronizedMap(new HashMap<Class<?>, Collection<Class<?>>>());
 
     // xml mapping
     protected final XMLMetaData[] EMPTY_XMLMETAS;
@@ -1622,19 +1623,20 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
     }
 
     /**
-     * Updates our datastructures with the latest registered classes.
+     * Updates our data structures with the latest registered classes.
+     *
+     * This method is synchronized to make sure that all data structures are fully updated
+     *  before other threads attempt to call this method
      */
-    Class<?>[] processRegisteredClasses(ClassLoader envLoader) {
-        if (_registered.isEmpty())
+    synchronized Class<?>[] processRegisteredClasses(ClassLoader envLoader) {
+        if (_registered.isEmpty()) {
             return EMPTY_CLASSES;
+        }
 
         // copy into new collection to avoid concurrent mod errors on reentrant
         // registrations
-        Class<?>[] reg;
-        synchronized (_registered) {
-            reg = _registered.toArray(new Class[_registered.size()]);
-            _registered.clear();
-        }
+        Class<?>[] reg = _registered.toArray(new Class[_registered.size()]);
+        _registered.clear();
 
         Collection<String> pcNames = getPersistentTypeNames(false, envLoader);
         Collection<Class<?>> failed = null;
@@ -1643,18 +1645,18 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
             if (pcNames != null && !pcNames.isEmpty() && !pcNames.contains(reg[i].getName())) {
                 continue;
             }
-            
+
             // If the compatibility option "filterPCRegistryClasses" is enabled, then verify that the type is
             // accessible to the envLoader/Thread Context ClassLoader
             if (_filterRegisteredClasses) {
                 Log log = (_conf == null) ? null : _conf.getLog(OpenJPAConfiguration.LOG_RUNTIME);
                 ClassLoader loadCL = (envLoader != null) ?
-                        envLoader :
-                        AccessController.doPrivileged(J2DoPrivHelper.getContextClassLoaderAction());
-                        
+                    envLoader :
+                    AccessController.doPrivileged(J2DoPrivHelper.getContextClassLoaderAction());
+
                 try {
                     Class<?> classFromAppClassLoader = Class.forName(reg[i].getName(), true, loadCL);
-                    
+
                     if (!reg[i].equals(classFromAppClassLoader)) {
                         // This is a class that belongs to a ClassLoader not associated with the Application,
                         // so it should be processed.
@@ -1837,7 +1839,7 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
     /**
      * Add the given value to the collection cached in the given map under the given key.
      */
-    private void addToCollection(Map map, Class<?> key, Class<?> value, boolean inheritance) {
+    private void addToCollection(Map<Class<?>, Collection<Class<?>>> map, Class<?> key, Class<?> value, boolean inheritance) {
         if (_locking) {
             synchronized (map) {
                 addToCollectionInternal(map, key, value, inheritance);
@@ -1847,8 +1849,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
         }
     }
 
-    private void addToCollectionInternal(Map map, Class<?> key, Class<?> value, boolean inheritance) {
-        Collection coll = (Collection) map.get(key);
+    private  void addToCollectionInternal(Map<Class<?>, Collection<Class<?>>> map, Class<?> key, Class<?> value, boolean inheritance) {
+        Collection<Class<?>> coll = map.get(key);
         if (coll == null) {
             if (inheritance) {
                 InheritanceComparator comp = new InheritanceComparator();
@@ -1921,8 +1923,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
 
     public void endConfiguration() {
         initializeMetaDataFactory();
- if (_implGen == null)
- _implGen = new InterfaceImplGenerator(this);
+        if (_implGen == null)
+            _implGen = new InterfaceImplGenerator(this);
         if (_preload == true) {
             _oids = new HashMap<Class<?>, Class<?>>();
             _impls = new HashMap<Class<?>, Collection<Class<?>>>();
@@ -1930,7 +1932,7 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
             _aliases = new HashMap<String, List<Class<?>>>();
             _pawares = new HashMap<Class<?>, NonPersistentMetaData>();
             _nonMapped = new HashMap<Class<?>, NonPersistentMetaData>();
-            _subs = new HashMap<Class<?>, List<Class<?>>>();
+            _subs = new HashMap<Class<?>, Collection<Class<?>>>();
             // Wait till we're done loading MetaData to flip _lock boolean.
         }            
     }