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 <dazeyde...@gmail.com> AuthorDate: Tue May 14 14:32:06 2019 -0500 OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition Signed-off-by: Will Dazey <dazeyde...@gmail.com> --- .../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. } }