diff --git a/pom.xml b/pom.xml index b2f7530e..0454bde1 100644 --- a/pom.xml +++ b/pom.xml @@ -99,7 +99,7 @@ org.apache.sling org.apache.sling.api - 2.21.0 + 2.24.0 provided diff --git a/src/main/java/org/apache/sling/models/impl/AdapterCacheHolder.java b/src/main/java/org/apache/sling/models/impl/AdapterCacheHolder.java new file mode 100644 index 00000000..500b5bf0 --- /dev/null +++ b/src/main/java/org/apache/sling/models/impl/AdapterCacheHolder.java @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.models.impl; + +import java.lang.ref.SoftReference; +import java.util.Collections; +import java.util.Map; +import java.util.WeakHashMap; + +import org.apache.sling.api.SlingHttpServletRequest; +import org.jetbrains.annotations.NotNull; + +class AdapterCacheHolder implements AutoCloseable { + public static final String ADAPTABLE_ADAPTER_CACHE_KEY = + ModelAdapterFactory.class.getName() + ".AdapterCacheHolder"; + + /* + * This is to handle that all requests, including wrappers, utilize the same cache. If it is desired + */ + private static final String SHARED_REQUEST_CACHE = "SHARED_REQUEST_CACHE"; + + // Map> + private final Map, SoftReference>> cacheMap; + + private final boolean sync; + + /** + * Create a cache, setting the sync and the Map fields. This constructor is primarily used to make testing easier. + * @param sync true if maps should be synchronized, + * @param cacheMap the map + */ + AdapterCacheHolder(boolean sync, Map, SoftReference>> cacheMap) { + this.sync = sync; + this.cacheMap = cacheMap; + } + + /** + * Create a cache, optionally using synchronized maps. + * @param sync true if maps should be synchronized, + */ + public AdapterCacheHolder(boolean sync) { + this(sync, newMap(sync)); + } + + /** + * Gets the cache for a given adaptable. + * @param adaptable the adaptalbe + * @return the cache object + */ + public @NotNull Map, SoftReference> getCacheMapForAdaptable(@NotNull Object adaptable) { + Object adaptableOrSharedKey = adaptable; + + // All request wrappers share the cache of the original request + if (adaptable instanceof SlingHttpServletRequest) { + adaptableOrSharedKey = SHARED_REQUEST_CACHE; + } + + return cacheMap.computeIfAbsent(adaptableOrSharedKey, key -> newMap(sync)); + } + + /** + * Clears the cache. Useful for caches that are stored in a {@link org.apache.sling.api.resource.ResourceResolver} + * or a {@link SlingHttpServletRequest}. + */ + public void close() { + cacheMap.values().forEach(Map::clear); + cacheMap.clear(); + } + + /** + * Create a new map, either synchronized or not. + * @param sync true if it should use synchronized maps + * @return a new map + * @param the key type + * @param the value type + */ + static @NotNull Map newMap(boolean sync) { + Map theMap = new WeakHashMap<>(); + + if (sync) { + theMap = Collections.synchronizedMap(theMap); + } + + return theMap; + } +} diff --git a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java index 557e501c..d05040c2 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java +++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java @@ -42,7 +42,6 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -52,6 +51,7 @@ import org.apache.sling.api.adapter.AdapterFactory; import org.apache.sling.api.adapter.AdapterManager; import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.commons.osgi.RankedServices; import org.apache.sling.models.annotations.Model; import org.apache.sling.models.annotations.ValidationStrategy; @@ -126,8 +126,6 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto private static final String REQUEST_MARKER_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".RealRequest"; - private static final String REQUEST_CACHE_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".AdapterCache"; - private static class DisposalCallbackRegistryImpl implements DisposalCallbackRegistry, Disposable { private List callbacks = new ArrayList<>(); @@ -222,7 +220,7 @@ private void clearDisposalCallbackRegistryQueue() { // dependencies) private ThreadLocal invocationCountThreadLocal; - private Map, SoftReference>> adapterCache; + private AdapterCacheHolder adapterCacheHolder; private SlingModelsScriptEngineFactory scriptEngineFactory; @@ -331,21 +329,31 @@ private ModelClass getImplementationTypeForAdapterType( + " from adaptable " + adaptable.getClass()); } - @SuppressWarnings("unchecked") private Map, SoftReference> getOrCreateCache(final Object adaptable) { - Map, SoftReference> adaptableCache; + AdapterCacheHolder cache; if (adaptable instanceof ServletRequest) { ServletRequest request = (ServletRequest) adaptable; - adaptableCache = (Map, SoftReference>) request.getAttribute(REQUEST_CACHE_ATTRIBUTE); - if (adaptableCache == null) { - adaptableCache = Collections.synchronizedMap(new WeakHashMap<>()); - request.setAttribute(REQUEST_CACHE_ATTRIBUTE, adaptableCache); + cache = (AdapterCacheHolder) request.getAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY); + if (cache == null) { + // This cache does not need to be synchronized, since it is just for a single request + cache = new AdapterCacheHolder(false); + request.setAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY, cache); + } + } else if (adaptable instanceof Resource || adaptable instanceof ResourceResolver) { + ResourceResolver rr; + if (adaptable instanceof Resource) { + rr = ((Resource) adaptable).getResourceResolver(); + } else { + rr = (ResourceResolver) adaptable; } + // This cache does not need to be synchronized, since it is just for a single resource or resource resolver + cache = (AdapterCacheHolder) rr.getPropertyMap() + .computeIfAbsent( + AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY, k -> new AdapterCacheHolder(false)); } else { - adaptableCache = - adapterCache.computeIfAbsent(adaptable, k -> Collections.synchronizedMap(new WeakHashMap<>())); + cache = adapterCacheHolder; } - return adaptableCache; + return cache.getCacheMapForAdaptable(adaptable); } @SuppressWarnings("unchecked") @@ -1174,8 +1182,8 @@ protected ThreadInvocationCounter initialValue() { } }; - this.adapterCache = - Collections.synchronizedMap(new WeakHashMap, SoftReference>>()); + // This map needs to be synchronized, since it is shared + this.adapterCacheHolder = new AdapterCacheHolder(true); BundleContext bundleContext = ctx.getBundleContext(); this.queue = new ReferenceQueue<>(); @@ -1210,7 +1218,8 @@ protected ThreadInvocationCounter initialValue() { @Deactivate protected void deactivate() { - this.adapterCache = null; + this.adapterCacheHolder.close(); + this.adapterCacheHolder = null; this.clearDisposalCallbackRegistryQueue(); this.listener.unregisterAll(); this.adapterImplementations.removeAll(); @@ -1419,9 +1428,10 @@ public T getModelFromWrappedRequest( @Override public void requestDestroyed(final ServletRequestEvent sre) { - final Object list = sre.getServletRequest().getAttribute(REQUEST_MARKER_ATTRIBUTE); + ServletRequest request = sre.getServletRequest(); + final Object list = request.getAttribute(REQUEST_MARKER_ATTRIBUTE); if (list != null) { - sre.getServletRequest().removeAttribute(REQUEST_MARKER_ATTRIBUTE); + request.removeAttribute(REQUEST_MARKER_ATTRIBUTE); if (list instanceof List) { final List callbackList = (List) list; for (final Object disposable : callbackList) { @@ -1432,6 +1442,9 @@ public void requestDestroyed(final ServletRequestEvent sre) { callbackList.clear(); } } + Optional.ofNullable(request.getAttribute(AdapterCacheHolder.ADAPTABLE_ADAPTER_CACHE_KEY)) + .map(AdapterCacheHolder.class::cast) + .ifPresent(AdapterCacheHolder::close); } @Override diff --git a/src/test/java/org/apache/sling/models/impl/AdapterCacheHolderTest.java b/src/test/java/org/apache/sling/models/impl/AdapterCacheHolderTest.java new file mode 100644 index 00000000..86326fa4 --- /dev/null +++ b/src/test/java/org/apache/sling/models/impl/AdapterCacheHolderTest.java @@ -0,0 +1,116 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.models.impl; + +import java.lang.ref.SoftReference; +import java.util.HashMap; +import java.util.Map; +import java.util.WeakHashMap; + +import org.apache.sling.api.SlingHttpServletRequest; +import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper; +import org.apache.sling.servlethelpers.MockSlingHttpServletRequest; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + +@RunWith(MockitoJUnitRunner.class) +public class AdapterCacheHolderTest { + + @Test + public void testGetMapNonServletRequestAdaptable() { + Map, SoftReference>> outerMap = spy(new HashMap<>()); + Object adaptable1 = new Object(); + Object adaptable2 = new Object(); + + try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) { + Map, SoftReference> map1 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1); + Map, SoftReference> map2 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1); + Map, SoftReference> map3 = adapterCacheHolder.getCacheMapForAdaptable(adaptable2); + + assertSame(map1, map2); + assertNotSame(map1, map3); + } + } + + @Test + public void testGetMapServletRequestAdaptable() { + Map, SoftReference>> outerMap = spy(new HashMap<>()); + SlingHttpServletRequest adaptable1 = new MockSlingHttpServletRequest(null); + SlingHttpServletRequest adaptable2 = new SlingHttpServletRequestWrapper(adaptable1); + SlingHttpServletRequest adaptable3 = new MockSlingHttpServletRequest(null); + + try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) { + Map, SoftReference> map1 = adapterCacheHolder.getCacheMapForAdaptable(adaptable1); + Map, SoftReference> map2 = adapterCacheHolder.getCacheMapForAdaptable(adaptable2); + Map, SoftReference> map3 = adapterCacheHolder.getCacheMapForAdaptable(adaptable3); + + assertSame(map1, map2); + assertSame(map1, map3); + } + } + + @Test + public void testClose() { + Object adaptable = new Object(); + Object result = new Object(); + SoftReference ref = new SoftReference<>(result); + + Map, SoftReference> innerMap = spy(new HashMap<>()); + innerMap.put(Object.class, ref); + + Map, SoftReference>> outerMap = spy(new HashMap<>()); + outerMap.put(adaptable, innerMap); + + try (AdapterCacheHolder adapterCacheHolder = new AdapterCacheHolder(false, outerMap)) { + // intentionally empty + } + + verify(outerMap).values(); + verify(outerMap).clear(); + verify(innerMap).clear(); + } + + @Test + public void testNewMapSyncMap() { + // This is a very weak test, but it should be sufficient to at least confirm that we are not just using a + // WeakHashMap, in case someone changes it without knowing why. + Map nonSyncMap = AdapterCacheHolder.newMap(false); + Map syncMap = AdapterCacheHolder.newMap(true); + + try { + WeakHashMap weakMap = (WeakHashMap) nonSyncMap; + } catch (ClassCastException e) { + fail("Expected a WeakHashMap. Actual: " + nonSyncMap.getClass().getName()); + } + + try { + WeakHashMap weakMap = (WeakHashMap) syncMap; + fail("Expected a map other than WeakHashMap."); + } catch (ClassCastException e) { + // Expected to throw + } + } +} diff --git a/src/test/java/org/apache/sling/models/impl/CachingTest.java b/src/test/java/org/apache/sling/models/impl/CachingTest.java index f22ed755..398f6406 100644 --- a/src/test/java/org/apache/sling/models/impl/CachingTest.java +++ b/src/test/java/org/apache/sling/models/impl/CachingTest.java @@ -20,17 +20,16 @@ import java.util.Collections; import java.util.Enumeration; +import java.util.HashMap; import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.api.resource.ValueMap; import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper; import org.apache.sling.api.wrappers.ValueMapDecorator; import org.apache.sling.models.impl.injectors.RequestAttributeInjector; import org.apache.sling.models.impl.injectors.ValueMapInjector; -import org.apache.sling.models.testmodels.classes.CachedModel; -import org.apache.sling.models.testmodels.classes.CachedModelWithAdapterTypes12; -import org.apache.sling.models.testmodels.classes.CachedModelWithAdapterTypes23; -import org.apache.sling.models.testmodels.classes.UncachedModel; +import org.apache.sling.models.testmodels.classes.*; import org.apache.sling.models.testmodels.interfaces.AdapterType1; import org.apache.sling.models.testmodels.interfaces.AdapterType2; import org.apache.sling.models.testmodels.interfaces.AdapterType3; @@ -60,6 +59,9 @@ public class CachingTest { @Mock private Resource resource; + @Mock + private ResourceResolver resourceResolver; + private ModelAdapterFactory factory; @Before @@ -88,6 +90,9 @@ public void setup() { ValueMap vm = new ValueMapDecorator(Collections.singletonMap("testValue", "test")); when(resource.adaptTo(ValueMap.class)).thenReturn(vm); + when(resource.getResourceResolver()).thenReturn(resourceResolver); + + when(resourceResolver.getPropertyMap()).thenReturn(new HashMap<>()); } @Test