This is an automated email from the ASF dual-hosted git repository. ddekany pushed a commit to branch 3 in repository https://gitbox.apache.org/repos/asf/freemarker.git
commit ffd4a5f1f821e33afd889017da8ab81b8a6c27a0 Author: ddekany <[email protected]> AuthorDate: Mon Dec 16 10:34:46 2019 +0100 ClassIntrospector related cleanups, mostly from ported from 2.3 --- .../core/model/impl/DefaultObjectWrapperDesc.java | 2 +- .../core/model/impl/DefaultObjectWrapperInc.java | 2 +- .../impl/DefaultObjectWrapperSingletonsTest.java | 20 +++-- .../core/model/impl/DefaultObjectWrapperTest.java | 2 +- .../impl/Java7MembersOnlyDefaultObjectWrapper.java | 2 +- .../SimpleMapAndCollectionObjectWrapper.java | 2 +- .../core/model/impl/ClassIntrospector.java | 98 ++++++++++++---------- .../core/model/impl/DefaultObjectWrapper.java | 48 +++-------- .../core/model/impl/RestrictedObjectWrapper.java | 6 +- 9 files changed, 87 insertions(+), 95 deletions(-) diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperDesc.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperDesc.java index 0ed6e7c..b0b02bf 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperDesc.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperDesc.java @@ -25,7 +25,7 @@ public class DefaultObjectWrapperDesc extends DefaultObjectWrapper { public DefaultObjectWrapperDesc() { super(new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) - .methodSorter(new AlphabeticalMethodSorter(true)), true); + .methodSorter(new AlphabeticalMethodSorter(true))); } } diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperInc.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperInc.java index eb2cda0..c678f4a 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperInc.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperInc.java @@ -25,7 +25,7 @@ public class DefaultObjectWrapperInc extends DefaultObjectWrapper { public DefaultObjectWrapperInc() { super(new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) - .methodSorter(new AlphabeticalMethodSorter(false)), true); + .methodSorter(new AlphabeticalMethodSorter(false))); } } diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperSingletonsTest.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperSingletonsTest.java index 164ce7d..932dfca 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperSingletonsTest.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperSingletonsTest.java @@ -309,11 +309,21 @@ public class DefaultObjectWrapperSingletonsTest extends TestCase { } public void testClassInrospectorCache() throws TemplateException { - assertFalse(new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) - .usePrivateCaches(true).build().isClassIntrospectionCacheRestricted()); - assertTrue(new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) - .build().isClassIntrospectionCacheRestricted()); - + { + DefaultObjectWrapper ow = new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) + .usePrivateCaches(true) + .build(); + assertFalse(ow.isClassIntrospectionCacheRestricted()); + assertFalse(ow.getClassIntrospector().isShared()); + } + + { + DefaultObjectWrapper ow = new DefaultObjectWrapper.Builder(Configuration.VERSION_3_0_0) + .build(); + assertTrue(ow.isClassIntrospectionCacheRestricted()); + assertTrue(ow.getClassIntrospector().isShared()); + } + ClassIntrospector.Builder.clearInstanceCache(); DefaultObjectWrapper.Builder.clearInstanceCache(); checkClassIntrospectorCacheSize(0); diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperTest.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperTest.java index d2b37f6..97a46f4 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperTest.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapperTest.java @@ -846,7 +846,7 @@ public class DefaultObjectWrapperTest { private static class CustomizedDefaultObjectWrapper extends DefaultObjectWrapper { private CustomizedDefaultObjectWrapper(Version incompatibleImprovements) { - super(new DefaultObjectWrapper.Builder(incompatibleImprovements), true); + super(new DefaultObjectWrapper.Builder(incompatibleImprovements)); } @Override diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/Java7MembersOnlyDefaultObjectWrapper.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/Java7MembersOnlyDefaultObjectWrapper.java index 96c1adf..ff602f1 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/Java7MembersOnlyDefaultObjectWrapper.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/model/impl/Java7MembersOnlyDefaultObjectWrapper.java @@ -83,7 +83,7 @@ public class Java7MembersOnlyDefaultObjectWrapper extends DefaultObjectWrapper { }; public Java7MembersOnlyDefaultObjectWrapper(Version version) { - super(new DefaultObjectWrapper.Builder(version).methodAppearanceFineTuner(POST_JAVA_7_FILTER), true); + super(new DefaultObjectWrapper.Builder(version).methodAppearanceFineTuner(POST_JAVA_7_FILTER)); } private static <T> Set<T> newHashSet(T... items) { diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/templatesuite/models/SimpleMapAndCollectionObjectWrapper.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/templatesuite/models/SimpleMapAndCollectionObjectWrapper.java index 8c8dd76..0343770 100644 --- a/freemarker-core-test/src/test/java/org/apache/freemarker/core/templatesuite/models/SimpleMapAndCollectionObjectWrapper.java +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/templatesuite/models/SimpleMapAndCollectionObjectWrapper.java @@ -36,7 +36,7 @@ import org.apache.freemarker.core.model.impl.SimpleSequence; public class SimpleMapAndCollectionObjectWrapper extends DefaultObjectWrapper { public SimpleMapAndCollectionObjectWrapper(Version incompatibleImprovements) { - super(new DefaultObjectWrapper.Builder(incompatibleImprovements), true); + super(new DefaultObjectWrapper.Builder(incompatibleImprovements)); } @Override diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/ClassIntrospector.java b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/ClassIntrospector.java index 621f876..951d379 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/ClassIntrospector.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/ClassIntrospector.java @@ -133,8 +133,8 @@ class ClassIntrospector { final MethodAppearanceFineTuner methodAppearanceFineTuner; final MethodSorter methodSorter; - /** See {@link #getHasSharedInstanceRestrictons()} */ - final private boolean hasSharedInstanceRestrictons; + /** See {@link #getHasSharedInstanceRestrictions()} */ + final private boolean hasSharedInstanceRestrictions; /** See {@link #isShared()} */ final private boolean shared; @@ -158,22 +158,14 @@ class ClassIntrospector { // Instantiation: /** - * Creates a new instance, that is hence surely not shared (singleton) instance. - * - * @param pa - * Stores what the values of the JavaBean properties of the returned instance will be. Not {@code null}. - */ - ClassIntrospector(Builder pa, Object sharedLock) { - this(pa, sharedLock, false, false); - } - - /** - * @param hasSharedInstanceRestrictons - * {@code true} exactly if we are creating a new instance with {@link Builder}. Then - * it's {@code true} even if it won't put the instance into the cache. + * @param hasSharedInstanceRestrictions + * If the instance should behave as if it is shared, even if it we couldn't put + * it into the shared cache. + * @param shared + * If the instance is in the shared cache, and so is potentially shared. */ ClassIntrospector(Builder builder, Object sharedLock, - boolean hasSharedInstanceRestrictons, boolean shared) { + boolean hasSharedInstanceRestrictions, boolean shared) { _NullArgumentException.check("sharedLock", sharedLock); exposureLevel = builder.getExposureLevel(); @@ -183,7 +175,7 @@ class ClassIntrospector { this.sharedLock = sharedLock; - this.hasSharedInstanceRestrictons = hasSharedInstanceRestrictons; + this.hasSharedInstanceRestrictions = hasSharedInstanceRestrictions; this.shared = shared; if (CLASS_CHANGE_NOTIFIER != null) { @@ -191,14 +183,6 @@ class ClassIntrospector { } } - /** - * Returns a {@link Builder}-s that could be used to invoke an identical {@link #ClassIntrospector} - * . The returned {@link Builder} can be modified without interfering with anything. - */ - Builder createBuilder() { - return new Builder(this); - } - // ------------------------------------------------------------------------------------------------------------------ // Introspection: @@ -829,7 +813,7 @@ class ClassIntrospector { * Corresponds to {@link DefaultObjectWrapper#clearClassIntrospectionCache()}. */ void clearCache() { - if (getHasSharedInstanceRestrictons()) { + if (getHasSharedInstanceRestrictions()) { throw new IllegalStateException( "It's not allowed to clear the whole cache in a read-only " + getClass().getName() + "instance. Use removeFromClassIntrospectionCache(String prefix) instead."); @@ -1008,14 +992,14 @@ class ClassIntrospector { * Returns {@code true} if this instance was created with {@link Builder}, even if it wasn't * actually put into the cache (as we reserve the right to do so in later versions). */ - boolean getHasSharedInstanceRestrictons() { - return hasSharedInstanceRestrictons; + boolean getHasSharedInstanceRestrictions() { + return hasSharedInstanceRestrictions; } /** * Tells if this instance is (potentially) shared among {@link DefaultObjectWrapper} instances. * - * @see #getHasSharedInstanceRestrictons() + * @see #getHasSharedInstanceRestrictions() */ boolean isShared() { return shared; @@ -1041,10 +1025,12 @@ class ClassIntrospector { static final class Builder implements CommonBuilder<ClassIntrospector>, Cloneable { - private static final Map/*<PropertyAssignments, Reference<ClassIntrospector>>*/ INSTANCE_CACHE = new HashMap(); - private static final ReferenceQueue INSTANCE_CACHE_REF_QUEUE = new ReferenceQueue(); + private static final Map<Builder, Reference<ClassIntrospector>> INSTANCE_CACHE = new HashMap<>(); + private static final ReferenceQueue<ClassIntrospector> INSTANCE_CACHE_REF_QUEUE = new ReferenceQueue<>(); // Properties and their *defaults*: + private boolean sharingDisallowed; + private boolean shardingDisallowedSet; private int exposureLevel = DefaultObjectWrapper.EXPOSE_SAFE; private boolean exposureLevelSet; private boolean exposeFields; @@ -1060,13 +1046,6 @@ class ClassIntrospector { private boolean alreadyBuilt; - Builder(ClassIntrospector ci) { - exposureLevel = ci.exposureLevel; - exposeFields = ci.exposeFields; - methodAppearanceFineTuner = ci.methodAppearanceFineTuner; - methodSorter = ci.methodSorter; - } - Builder(Version incompatibleImprovements) { // Warning: incompatibleImprovements must not affect this object at versions increments where there's no // change in the DefaultObjectWrapper.normalizeIncompatibleImprovements results. That is, this class may don't react @@ -1088,6 +1067,7 @@ class ClassIntrospector { public int hashCode() { final int prime = 31; int result = 1; + result = prime * result + (sharingDisallowed ? 1231 : 1237); result = prime * result + (exposeFields ? 1231 : 1237); result = prime * result + exposureLevel; result = prime * result + System.identityHashCode(methodAppearanceFineTuner); @@ -1102,12 +1082,36 @@ class ClassIntrospector { if (getClass() != obj.getClass()) return false; Builder other = (Builder) obj; + if (sharingDisallowed != other.sharingDisallowed) return false; if (exposeFields != other.exposeFields) return false; if (exposureLevel != other.exposureLevel) return false; if (methodAppearanceFineTuner != other.methodAppearanceFineTuner) return false; return methodSorter == other.methodSorter; } + public boolean getShareable() { + return sharingDisallowed; + } + + /** + * Can be used to prevent the sharing of the {@link ClassIntrospector} through the global cache. Setting this + * to {@code false} doesn't guarantee that it will be shared (as some other setting values can make that + * impossible), but setting this to {@code true} guarantees that it won't be shared. Defaults to {@code false}. + * + * @see DefaultObjectWrapper.ExtendableBuilder#setUsePrivateCaches(boolean) + */ + public void setSharingDisallowed(boolean shareable) { + this.sharingDisallowed = shareable; + shardingDisallowedSet = true; + } + + /** + * Tells if the property was explicitly set, as opposed to just holding its default value. + */ + public boolean isShardingDisallowedSet() { + return shardingDisallowedSet; + } + public int getExposureLevel() { return exposureLevel; } @@ -1171,10 +1175,11 @@ class ClassIntrospector { } private static void removeClearedReferencesFromInstanceCache() { - Reference clearedRef; + Reference<? extends ClassIntrospector> clearedRef; while ((clearedRef = INSTANCE_CACHE_REF_QUEUE.poll()) != null) { synchronized (INSTANCE_CACHE) { - findClearedRef: for (Iterator it = INSTANCE_CACHE.values().iterator(); it.hasNext(); ) { + findClearedRef: for (Iterator<Reference<ClassIntrospector>> it = INSTANCE_CACHE.values().iterator(); + it.hasNext(); ) { if (it.next() == clearedRef) { it.remove(); break findClearedRef; @@ -1192,7 +1197,7 @@ class ClassIntrospector { } /** For unit testing only */ - static Map getInstanceCache() { + static Map<Builder, Reference<ClassIntrospector>> getInstanceCache() { return INSTANCE_CACHE; } @@ -1207,16 +1212,17 @@ class ClassIntrospector { } ClassIntrospector instance; - if ((methodAppearanceFineTuner == null || methodAppearanceFineTuner instanceof SingletonCustomizer) + if (!sharingDisallowed + && (methodAppearanceFineTuner == null || methodAppearanceFineTuner instanceof SingletonCustomizer) && (methodSorter == null || methodSorter instanceof SingletonCustomizer)) { // Instance can be cached. synchronized (INSTANCE_CACHE) { - Reference instanceRef = (Reference) INSTANCE_CACHE.get(this); - instance = instanceRef != null ? (ClassIntrospector) instanceRef.get() : null; + Reference<ClassIntrospector> instanceRef = INSTANCE_CACHE.get(this); + instance = instanceRef != null ? instanceRef.get() : null; if (instance == null) { Builder thisClone = (Builder) clone(); // prevent any aliasing issues instance = new ClassIntrospector(thisClone, new Object(), true, true); - INSTANCE_CACHE.put(thisClone, new WeakReference(instance, INSTANCE_CACHE_REF_QUEUE)); + INSTANCE_CACHE.put(thisClone, new WeakReference<>(instance, INSTANCE_CACHE_REF_QUEUE)); } } @@ -1225,7 +1231,7 @@ class ClassIntrospector { // If methodAppearanceFineTuner or methodSorter is specified and isn't marked as a singleton, the // ClassIntrospector can't be shared/cached as those objects could contain a back-reference to the // DefaultObjectWrapper. - instance = new ClassIntrospector(this, new Object(), true, false); + instance = new ClassIntrospector(this, new Object(), !sharingDisallowed, false); } alreadyBuilt = true; diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapper.java b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapper.java index 26d5cc2..e3474b1 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapper.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/DefaultObjectWrapper.java @@ -172,29 +172,16 @@ public class DefaultObjectWrapper implements RichObjectWrapper { /** * Initializes the instance based on the the {@link ExtendableBuilder} specified. - * - * @param finalizeConstruction Decides if the construction is finalized now, or the caller will do some more - * adjustments on the instance and then call {@link #finalizeConstruction()} itself. */ - protected DefaultObjectWrapper(ExtendableBuilder<?, ?> builder, boolean finalizeConstruction) { + protected DefaultObjectWrapper(ExtendableBuilder<?, ?> builder) { incompatibleImprovements = builder.getIncompatibleImprovements(); // normalized defaultDateType = builder.getDefaultDateType(); outerIdentity = builder.getOuterIdentity() != null ? builder.getOuterIdentity() : this; strict = builder.isStrict(); - if (builder.getUsePrivateCaches()) { - // As this is not a read-only DefaultObjectWrapper, the classIntrospector will be possibly replaced for a few times, - // but we need to use the same sharedInrospectionLock forever, because that's what the model factories - // synchronize on, even during the classIntrospector is being replaced. - sharedIntrospectionLock = new Object(); - classIntrospector = new ClassIntrospector(builder.classIntrospectorBuilder, sharedIntrospectionLock); - } else { - // As this is a read-only DefaultObjectWrapper, the classIntrospector is never replaced, and since it's shared by - // other DefaultObjectWrapper instances, we use the lock belonging to the shared ClassIntrospector. - classIntrospector = builder.classIntrospectorBuilder.build(); - sharedIntrospectionLock = classIntrospector.getSharedLock(); - } + classIntrospector = builder.classIntrospectorBuilder.build(); + sharedIntrospectionLock = classIntrospector.getSharedLock(); staticModels = new StaticModels(this); enumModels = new EnumModels(this); @@ -227,21 +214,7 @@ public class DefaultObjectWrapper implements RichObjectWrapper { } } - finalizeConstruction(); - } - - /** - * Meant to be called after {@link DefaultObjectWrapper#DefaultObjectWrapper(ExtendableBuilder, boolean)} when - * its last argument was {@code false}; makes the instance read-only if necessary, then registers the model - * factories in the class introspector. No further changes should be done after calling this, if - * {@code writeProtected} was {@code true}. - */ - protected void finalizeConstruction() { - // Attention! At this point, the DefaultObjectWrapper must be fully initialized, as when the model factories are - // registered below, the DefaultObjectWrapper can immediately get concurrent callbacks. That those other threads will - // see consistent image of the DefaultObjectWrapper is ensured that callbacks are always sync-ed on - // classIntrospector.sharedLock, and so is classIntrospector.registerModelFactory(...). - + // Do this only when all fields were set, as this may causes calls to this instance: registerModelFactories(); } @@ -321,7 +294,7 @@ public class DefaultObjectWrapper implements RichObjectWrapper { * will not be actually shared because there's no one to share with, but this will {@code true} even then. */ public boolean isClassIntrospectionCacheRestricted() { - return classIntrospector.getHasSharedInstanceRestrictons(); + return classIntrospector.getHasSharedInstanceRestrictions(); } private void registerModelFactories() { @@ -1368,7 +1341,7 @@ public class DefaultObjectWrapper implements RichObjectWrapper { @Override public DefaultObjectWrapper invoke(Builder builder) { - return new DefaultObjectWrapper(builder, true); + return new DefaultObjectWrapper(builder); } } @@ -1693,12 +1666,15 @@ public class DefaultObjectWrapper implements RichObjectWrapper { } /** - * Tells if the instance creates should share caches with other {@link DefaultObjectWrapper} instances - * (where possible), or it should always invoke its own caches and not share that with anyone else. - * */ + * Tells if the instance created should share caches with other {@link DefaultObjectWrapper} instances + * (where possible), or it should always use its own caches, and not share that with anyone else. + * In {@link DefaultObjectWrapper.Builder} this defaults to {@code false}, in order to have a single global + * (static) class introspection cache in the JVM. + */ public void setUsePrivateCaches(boolean usePrivateCaches) { this.usePrivateCaches = usePrivateCaches; usePrivateCachesSet = true; + classIntrospectorBuilder.setSharingDisallowed(usePrivateCaches); } /** diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/RestrictedObjectWrapper.java b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/RestrictedObjectWrapper.java index 46347c4..1dff3a6 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/RestrictedObjectWrapper.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/model/impl/RestrictedObjectWrapper.java @@ -36,8 +36,8 @@ import org.apache.freemarker.core.model.TemplateModel; */ public class RestrictedObjectWrapper extends DefaultObjectWrapper { - protected RestrictedObjectWrapper(Builder builder, boolean finalizeConstruction) { - super(builder, finalizeConstruction); + protected RestrictedObjectWrapper(Builder builder) { + super(builder); } /** @@ -97,7 +97,7 @@ public class RestrictedObjectWrapper extends DefaultObjectWrapper { @Override public RestrictedObjectWrapper invoke(Builder builder) { - return new RestrictedObjectWrapper(builder, true); + return new RestrictedObjectWrapper(builder); } }
