This is an automated email from the ASF dual-hosted git repository. dazeydev pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/openjpa.git
The following commit(s) were added to refs/heads/master by this push: new 389a82d OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition new cd81d36 Merge pull request #45 from dazey3/2767_master 389a82d is described below commit 389a82db97b96bd2bf6b519afdc64e833f5c0478 Author: Will Dazey <dazeyde...@gmail.com> AuthorDate: Tue May 14 15:25:43 2019 -0500 OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and MetaDataRepository race condition Signed-off-by: Will Dazey <dazeyde...@gmail.com> --- .../meta/strats/ValueMapDiscriminatorStrategy.java | 84 ++++++++++++++++------ .../apache/openjpa/meta/MetaDataRepository.java | 34 ++++----- 2 files changed, 81 insertions(+), 37 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 cedfa85..5c44c27 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; @@ -43,7 +45,6 @@ import org.apache.openjpa.util.MetaDataException; public class ValueMapDiscriminatorStrategy extends InValueDiscriminatorStrategy { - private static final long serialVersionUID = 1L; public static final String ALIAS = "value-map"; @@ -51,7 +52,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(); @Override public String getAlias() { @@ -67,8 +71,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(); @@ -88,29 +92,67 @@ public class ValueMapDiscriminatorStrategy @Override 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(); @@ -118,7 +160,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 7bc33b9..0525e22 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 @@ -126,7 +126,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; @@ -1625,19 +1626,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; @@ -1652,8 +1654,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con 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); @@ -1840,7 +1842,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); @@ -1850,8 +1852,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(); @@ -1927,8 +1929,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con @Override public void endConfiguration() { initializeMetaDataFactory(); - if (_implGen == null) - _implGen = new InterfaceImplGenerator(this); + if (_implGen == null) + _implGen = new InterfaceImplGenerator(this); if (_preload == true) { _oids = new HashMap<>(); _impls = new HashMap<>();