diff --git a/pom.xml b/pom.xml index 220de4c..1921b53 100644 --- a/pom.xml +++ b/pom.xml @@ -99,7 +99,7 @@ org.apache.sling org.apache.sling.api - 2.21.0 + 2.27.2 provided diff --git a/src/main/java/org/apache/sling/models/impl/DisposalCallbackRegistryImpl.java b/src/main/java/org/apache/sling/models/impl/DisposalCallbackRegistryImpl.java index 455beab..36a9134 100644 --- a/src/main/java/org/apache/sling/models/impl/DisposalCallbackRegistryImpl.java +++ b/src/main/java/org/apache/sling/models/impl/DisposalCallbackRegistryImpl.java @@ -18,30 +18,88 @@ */ package org.apache.sling.models.impl; +import java.io.Closeable; +import java.lang.ref.PhantomReference; +import java.lang.ref.ReferenceQueue; import java.util.ArrayList; -import java.util.Collections; import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ResourceResolverFactory; import org.apache.sling.models.spi.DisposalCallback; import org.apache.sling.models.spi.DisposalCallbackRegistry; import org.jetbrains.annotations.NotNull; public class DisposalCallbackRegistryImpl implements DisposalCallbackRegistry { - List callbacks = new ArrayList<>(); + private static final String RESOURCE_RESOLVER_DISPOSABLE = + ModelAdapterFactory.class.getName().concat(".Disposable"); + + private static final ReferenceQueue QUEUE = new ReferenceQueue<>(); + + private static final Map, DisposalCallbackRegistryImpl> REFERENCES = + new ConcurrentHashMap<>(); + + private final List callbacks = new ArrayList<>(); + + private boolean sealed = false; @Override public void addDisposalCallback(@NotNull DisposalCallback callback) { + if (sealed) { + throw new IllegalStateException("DisposalCallbackRegistry is sealed"); + } callbacks.add(callback); } - void seal() { - callbacks = Collections.unmodifiableList(callbacks); + @SuppressWarnings("resource") + public void registerIfNotEmpty(final ResourceResolverFactory factory, final Object referencedObject) { + if (!this.callbacks.isEmpty()) { + this.sealed = true; + + final ResourceResolver resolver = factory.getThreadResourceResolver(); + if (resolver != null) { + @SuppressWarnings("unchecked") + final List list = + (List) resolver.getPropertyMap() + .computeIfAbsent(RESOURCE_RESOLVER_DISPOSABLE, key -> new CloseableList()); + list.add(this); + } else { + final PhantomReference ref = new PhantomReference<>(referencedObject, QUEUE); + REFERENCES.put(ref, this); + } + } } - public void onDisposed() { + public void close() { for (DisposalCallback callback : callbacks) { callback.onDisposed(); } + callbacks.clear(); + } + + private static final class CloseableList extends ArrayList implements Closeable { + + @Override + public void close() { + for (final DisposalCallbackRegistryImpl registry : this) { + registry.close(); + } + this.clear(); + } + } + + @SuppressWarnings("unchecked") + public static void cleanupDisposables() { + PhantomReference ref = (PhantomReference) QUEUE.poll(); + while (ref != null) { + final DisposalCallbackRegistryImpl registry = REFERENCES.remove(ref); + if (registry != null) { + registry.close(); + } + ref = (PhantomReference) QUEUE.poll(); + } } } 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 69a039b..4510afb 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java +++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java @@ -20,11 +20,8 @@ import javax.annotation.PostConstruct; import javax.servlet.ServletRequest; -import javax.servlet.ServletRequestEvent; -import javax.servlet.ServletRequestListener; -import java.lang.ref.PhantomReference; -import java.lang.ref.ReferenceQueue; +import java.io.PrintWriter; import java.lang.ref.SoftReference; import java.lang.reflect.InvocationHandler; import java.lang.reflect.InvocationTargetException; @@ -37,7 +34,6 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.Hashtable; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -50,9 +46,9 @@ import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.adapter.Adaptable; -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.ResourceResolverFactory; import org.apache.sling.models.annotations.Model; import org.apache.sling.models.annotations.ValidationStrategy; import org.apache.sling.models.annotations.ViaProviderType; @@ -92,10 +88,7 @@ import org.jetbrains.annotations.Nullable; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; -import org.osgi.framework.Constants; import org.osgi.framework.FrameworkUtil; -import org.osgi.framework.ServiceRegistration; -import org.osgi.service.component.ComponentContext; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; @@ -104,52 +97,33 @@ import org.osgi.service.component.annotations.ReferenceCardinality; import org.osgi.service.component.annotations.ReferencePolicy; import org.osgi.service.component.annotations.ReferencePolicyOption; -import org.osgi.service.http.whiteboard.HttpWhiteboardConstants; import org.osgi.service.metatype.annotations.Designate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @Component( immediate = true, - service = {ModelFactory.class, ServletRequestListener.class}, + service = {ModelFactory.class}, property = { - HttpWhiteboardConstants.HTTP_WHITEBOARD_LISTENER + "=true", - HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_SELECT + "=(" - + HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_NAME + "=*)" + "felix.webconsole.label=slingmodels", + "felix.webconsole.title=Sling Models", + "felix.webconsole.configprinter.modes=always", + "scheduler.name=Sling Models OSGi Service Disposal Job", + "scheduler.concurrent=false" }) @Designate(ocd = ModelAdapterFactoryConfiguration.class) @SuppressWarnings("deprecation") -public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFactory, ServletRequestListener { +public class ModelAdapterFactory implements ModelFactory, Runnable { // hard code this value since we always know exactly how many there are private static final int VALUE_PREPARERS_COUNT = 2; - private static final String REQUEST_MARKER_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".RealRequest"; - private static final String REQUEST_CACHE_ATTRIBUTE = ModelAdapterFactory.class.getName() + ".AdapterCache"; private final Logger log = LoggerFactory.getLogger(ModelAdapterFactory.class); - private ReferenceQueue queue; - - private ConcurrentMap, DisposalCallbackRegistryImpl> disposalCallbacks; - - @Override - public void run() { - clearDisposalCallbackRegistryQueue(); - } - - private void clearDisposalCallbackRegistryQueue() { - java.lang.ref.Reference ref = queue.poll(); - while (ref != null) { - log.debug("calling disposal for {}.", ref); - final DisposalCallbackRegistryImpl registry = disposalCallbacks.remove(ref); - if (registry != null) { - registry.onDisposed(); - } - ref = queue.poll(); - } - } + @Reference + ResourceResolverFactory resourceResolverFactory; /** Injectors are sorted by DS according to their service ranking */ @Reference( @@ -201,9 +175,7 @@ private void clearDisposalCallbackRegistryQueue() { final AdapterImplementations adapterImplementations = new AdapterImplementations(); - private ServiceRegistration jobRegistration; - - private ServiceRegistration configPrinterRegistration; + private ModelConfigurationPrinter configPrinter; // Use threadlocal to count recursive invocations and break recursing if a max. limit is reached (to avoid cyclic // dependencies) @@ -214,6 +186,10 @@ private void clearDisposalCallbackRegistryQueue() { private SlingModelsScriptEngineFactory scriptEngineFactory; @Override + public void run() { + DisposalCallbackRegistryImpl.cleanupDisposables(); + } + @SuppressWarnings("null") public AdapterType getAdapter(Object adaptable, Class type) { Result result = internalCreateModel(adaptable, type); @@ -698,7 +674,7 @@ private Result createInvocationHandler( } } - this.registerCallbackRegistry(registry, adaptable, handler); + registry.registerIfNotEmpty(this.resourceResolverFactory, handler); if (missingElements != null) { MissingElementsException missingElementsException = new MissingElementsException( "Could not create all mandatory methods for interface of model " + modelClass); @@ -735,13 +711,13 @@ private Result createObject(final Object adaptable, final Result result = newInstanceWithConstructorInjection( constructorToUse, adaptable, modelClass, registry, preparedValues); if (!result.wasSuccessful()) { - registry.onDisposed(); + registry.close(); return result; } else { object = result.getValue(); } } catch (InstantiationException | InvocationTargetException | IllegalAccessException ex) { - registry.onDisposed(); + registry.close(); throw ex; } } @@ -761,7 +737,7 @@ private Result createObject(final Object adaptable, final } } - this.registerCallbackRegistry(registry, adaptable, object); + registry.registerIfNotEmpty(this.resourceResolverFactory, object); if (missingElements != null) { MissingElementsException missingElementsException = new MissingElementsException("Could not inject all required fields into " + modelClass.getType()); @@ -1155,7 +1131,7 @@ private static boolean isAcceptableType(Class type, Type genericType, Object } @Activate - protected void activate(final ComponentContext ctx, final ModelAdapterFactoryConfiguration configuration) { + protected void activate(final BundleContext bundleContext, final ModelAdapterFactoryConfiguration configuration) { this.invocationCountThreadLocal = new ThreadLocal() { @Override protected ThreadInvocationCounter initialValue() { @@ -1166,51 +1142,24 @@ protected ThreadInvocationCounter initialValue() { this.adapterCache = Collections.synchronizedMap(new WeakHashMap, SoftReference>>()); - BundleContext bundleContext = ctx.getBundleContext(); - this.queue = new ReferenceQueue<>(); - this.disposalCallbacks = new ConcurrentHashMap<>(); - Hashtable properties = new Hashtable<>(); - properties.put(Constants.SERVICE_VENDOR, "Apache Software Foundation"); - properties.put(Constants.SERVICE_DESCRIPTION, "Sling Models OSGi Service Disposal Job"); - properties.put("scheduler.name", "Sling Models OSGi Service Disposal Job"); - properties.put("scheduler.concurrent", false); - properties.put("scheduler.period", configuration.cleanup_job_period()); - - this.jobRegistration = bundleContext.registerService(Runnable.class, this, properties); - this.scriptEngineFactory = new SlingModelsScriptEngineFactory(bundleContext.getBundle()); this.listener = new ModelPackageBundleListener( - ctx.getBundleContext(), + bundleContext, this, this.adapterImplementations, bindingsValuesProvidersByContext, scriptEngineFactory); - Hashtable printerProps = new Hashtable<>(); - printerProps.put(Constants.SERVICE_VENDOR, "Apache Software Foundation"); - printerProps.put(Constants.SERVICE_DESCRIPTION, "Sling Models Configuration Printer"); - printerProps.put("felix.webconsole.label", "slingmodels"); - printerProps.put("felix.webconsole.title", "Sling Models"); - printerProps.put("felix.webconsole.configprinter.modes", "always"); - - this.configPrinterRegistration = bundleContext.registerService( - Object.class, new ModelConfigurationPrinter(this, bundleContext, adapterImplementations), printerProps); + this.configPrinter = new ModelConfigurationPrinter(bundleContext); } @Deactivate protected void deactivate() { this.adapterCache = null; - this.clearDisposalCallbackRegistryQueue(); this.listener.unregisterAll(); this.adapterImplementations.removeAll(); - if (jobRegistration != null) { - jobRegistration.unregister(); - jobRegistration = null; - } - if (configPrinterRegistration != null) { - configPrinterRegistration.unregister(); - configPrinterRegistration = null; - } + this.configPrinter = null; + DisposalCallbackRegistryImpl.cleanupDisposables(); } @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC) @@ -1249,38 +1198,6 @@ protected void unbindViaProvider(final ViaProvider viaProvider, final Map getInjectors() { - return injectors; - } - - @NotNull - Collection getInjectAnnotationProcessorFactories() { - return injectAnnotationProcessorFactories; - } - - @NotNull - Collection getInjectAnnotationProcessorFactories2() { - return injectAnnotationProcessorFactories2; - } - - @NotNull - Collection getStaticInjectAnnotationProcessorFactories() { - synchronized (this) { - return new ArrayList<>(staticInjectAnnotationProcessorFactories.values()); - } - } - - @NotNull - List getImplementationPickers() { - return this.implementationPickers; - } - - @NotNull - Map, ViaProvider> getViaProviders() { - return viaProviders; - } - @Override public boolean isModelAvailableForRequest(@NotNull SlingHttpServletRequest request) { return adapterImplementations.getModelClassForRequest(request) != null; @@ -1374,46 +1291,19 @@ public T getModelFromWrappedRequest( .adaptTo(targetClass); } - @Override - public void requestDestroyed(final ServletRequestEvent sre) { - final Object list = sre.getServletRequest().getAttribute(REQUEST_MARKER_ATTRIBUTE); - if (list != null) { - sre.getServletRequest().removeAttribute(REQUEST_MARKER_ATTRIBUTE); - if (list instanceof List) { - final List callbackList = (List) list; - for (final Object disposable : callbackList) { - if (disposable instanceof DisposalCallbackRegistryImpl) { - ((DisposalCallbackRegistryImpl) disposable).onDisposed(); - } - } - callbackList.clear(); - } - } - } - - @Override - public void requestInitialized(final ServletRequestEvent sre) { - sre.getServletRequest().setAttribute(REQUEST_MARKER_ATTRIBUTE, new ArrayList<>()); - } - - @SuppressWarnings("unchecked") - private void registerCallbackRegistry( - final DisposalCallbackRegistryImpl registry, final Object adaptable, final Object handler) { - if (!registry.callbacks.isEmpty()) { - registry.seal(); - - boolean registered = false; - if (adaptable instanceof SlingHttpServletRequest) { - final Object list = ((SlingHttpServletRequest) adaptable).getAttribute(REQUEST_MARKER_ATTRIBUTE); - if (list instanceof List) { - ((List) list).add(registry); - registered = true; - } - } - if (!registered) { - PhantomReference reference = new PhantomReference<>(handler, queue); - disposalCallbacks.put(reference, registry); - } + public void printConfiguration(final PrintWriter printWriter) { + final Collection factories; + synchronized (this) { + factories = new ArrayList<>(this.staticInjectAnnotationProcessorFactories.values()); } + this.configPrinter.printConfiguration( + printWriter, + this.adapterImplementations, + this.injectors, + this.injectAnnotationProcessorFactories, + this.injectAnnotationProcessorFactories2, + factories, + this.implementationPickers, + this.viaProviders); } } diff --git a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java index 86b25e1..37e0fc1 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java +++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactoryConfiguration.java @@ -29,5 +29,5 @@ @AttributeDefinition( name = "Cleanup Job Period", description = "Period in seconds at which OSGi service references from ThreadLocals will be cleaned up.") - long cleanup_job_period() default 30l; + long scheduler_period() default 30l; } diff --git a/src/main/java/org/apache/sling/models/impl/ModelConfigurationPrinter.java b/src/main/java/org/apache/sling/models/impl/ModelConfigurationPrinter.java index e580e2f..f9a709f 100644 --- a/src/main/java/org/apache/sling/models/impl/ModelConfigurationPrinter.java +++ b/src/main/java/org/apache/sling/models/impl/ModelConfigurationPrinter.java @@ -22,7 +22,6 @@ import java.io.PrintWriter; import java.util.Collection; -import java.util.List; import java.util.Map; import org.apache.sling.models.annotations.ViaProviderType; @@ -42,24 +41,24 @@ public class ModelConfigurationPrinter { private static final String EXPORT_SERVLET_FILTER = "(" + ModelPackageBundleListener.PROP_EXPORTER_SERVLET_CLASS + "=*)"; - private final ModelAdapterFactory modelAdapterFactory; private final BundleContext bundleContext; - private final AdapterImplementations adapterImplementations; - ModelConfigurationPrinter( - ModelAdapterFactory modelAdapterFactory, - BundleContext bundleContext, - AdapterImplementations adapterImplementations) { - this.modelAdapterFactory = modelAdapterFactory; + ModelConfigurationPrinter(BundleContext bundleContext) { this.bundleContext = bundleContext; - this.adapterImplementations = adapterImplementations; } - public void printConfiguration(PrintWriter printWriter) { + public void printConfiguration( + PrintWriter printWriter, + final AdapterImplementations adapterImplementations, + final Collection injectors, + final Collection factories, + final Collection factories2, + final Collection staticFactories, + final Collection pickers, + final Map, ViaProvider> viaProviders) { // injectors printWriter.println("Sling Models Injectors:"); - Collection injectors = modelAdapterFactory.getInjectors(); if (injectors.isEmpty()) { printWriter.println("none"); } else { @@ -73,12 +72,6 @@ public void printConfiguration(PrintWriter printWriter) { // inject annotations processor factories printWriter.println("Sling Models Inject Annotation Processor Factories:"); - Collection factories = - modelAdapterFactory.getInjectAnnotationProcessorFactories(); - Collection factories2 = - modelAdapterFactory.getInjectAnnotationProcessorFactories2(); - Collection staticFactories = - modelAdapterFactory.getStaticInjectAnnotationProcessorFactories(); if ((factories.isEmpty()) && (factories2.isEmpty()) && (staticFactories.isEmpty())) { printWriter.println("none"); } else { @@ -99,7 +92,6 @@ public void printConfiguration(PrintWriter printWriter) { // implementation pickers printWriter.println("Sling Models Implementation Pickers:"); - List pickers = modelAdapterFactory.getImplementationPickers(); if (pickers.size() == 0) { printWriter.println("none"); } else { @@ -113,7 +105,6 @@ public void printConfiguration(PrintWriter printWriter) { // implementation pickers printWriter.println("Sling Models Via Providers:"); - Map, ViaProvider> viaProviders = modelAdapterFactory.getViaProviders(); if (viaProviders == null || viaProviders.size() == 0) { printWriter.println("none"); } else { @@ -150,21 +141,19 @@ public void printConfiguration(PrintWriter printWriter) { // registered exporter servlets printWriter.println("Sling Models Exporter Servlets:"); try { - ServiceReference[] servlets = - bundleContext.getServiceReferences(Servlet.class.getName(), EXPORT_SERVLET_FILTER); - if (servlets != null) { - for (ServiceReference ref : servlets) { - printWriter.print(ref.getProperty(ModelPackageBundleListener.PROP_EXPORTER_SERVLET_CLASS)); - printWriter.print(" exports '"); - printWriter.print(ref.getProperty("sling.servlet.resourceTypes")); - printWriter.print("' with selector '"); - printWriter.print(ref.getProperty("sling.servlet.selectors")); - printWriter.print("' and extension '"); - printWriter.print(ref.getProperty("sling.servlet.extensions")); - printWriter.print("' with exporter '"); - printWriter.print(ref.getProperty(ModelPackageBundleListener.PROP_EXPORTER_SERVLET_NAME)); - printWriter.println("'"); - } + Collection> servlets = + bundleContext.getServiceReferences(Servlet.class, EXPORT_SERVLET_FILTER); + for (ServiceReference ref : servlets) { + printWriter.print(ref.getProperty(ModelPackageBundleListener.PROP_EXPORTER_SERVLET_CLASS)); + printWriter.print(" exports '"); + printWriter.print(ref.getProperty("sling.servlet.resourceTypes")); + printWriter.print("' with selector '"); + printWriter.print(ref.getProperty("sling.servlet.selectors")); + printWriter.print("' and extension '"); + printWriter.print(ref.getProperty("sling.servlet.extensions")); + printWriter.print("' with exporter '"); + printWriter.print(ref.getProperty(ModelPackageBundleListener.PROP_EXPORTER_SERVLET_NAME)); + printWriter.println("'"); } } catch (InvalidSyntaxException e) { // ignore diff --git a/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java b/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java index 573f7d2..17f3ab6 100644 --- a/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java +++ b/src/test/java/org/apache/sling/models/impl/AdapterFactoryTest.java @@ -18,6 +18,7 @@ */ package org.apache.sling.models.impl; +import java.io.Closeable; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -25,6 +26,8 @@ import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ResourceResolverFactory; import org.apache.sling.api.resource.ValueMap; import org.apache.sling.api.wrappers.ValueMapDecorator; import org.apache.sling.models.annotations.Model; @@ -50,12 +53,14 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; import org.osgi.framework.BundleContext; -import org.osgi.service.component.ComponentContext; import org.osgi.util.converter.Converter; import org.osgi.util.converter.Converters; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -76,18 +81,38 @@ public static ModelAdapterFactory createModelAdapterFactory() { } public static ModelAdapterFactory createModelAdapterFactory(BundleContext bundleContext) { - ComponentContext componentCtx = Mockito.mock(ComponentContext.class); - when(componentCtx.getBundleContext()).thenReturn(bundleContext); - ModelAdapterFactory factory = new ModelAdapterFactory(); Converter c = Converters.standardConverter(); Map map = new HashMap<>(); ModelAdapterFactoryConfiguration config = c.convert(map).to(ModelAdapterFactoryConfiguration.class); - factory.activate(componentCtx, config); + factory.activate(bundleContext, config); factory.injectAnnotationProcessorFactories = Collections.emptyList(); factory.injectAnnotationProcessorFactories2 = Collections.emptyList(); factory.injectors = Collections.emptyList(); factory.implementationPickers = Collections.emptyList(); + final ResourceResolverFactory rrf = mock(ResourceResolverFactory.class); + final ResourceResolver rr = mock(ResourceResolver.class); + lenient().when(rrf.getThreadResourceResolver()).thenReturn(rr); + final Map props = new HashMap<>(); + lenient().when(rr.getPropertyMap()).thenReturn(props); + lenient() + .doAnswer(new Answer() { + public Object answer(InvocationOnMock invocation) { + for (final Object v : props.values()) { + if (v instanceof Closeable) { + try { + ((Closeable) v).close(); + } catch (Exception e) { + // ignore + } + } + } + return null; + } + }) + .when(rr) + .close(); + factory.resourceResolverFactory = rrf; return factory; } diff --git a/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java b/src/test/java/org/apache/sling/models/impl/ModelAdapterFactoryTest.java similarity index 54% rename from src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java rename to src/test/java/org/apache/sling/models/impl/ModelAdapterFactoryTest.java index bdd85fa..3b09daf 100644 --- a/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java +++ b/src/test/java/org/apache/sling/models/impl/ModelAdapterFactoryTest.java @@ -20,19 +20,16 @@ import javax.inject.Inject; import javax.servlet.ServletContext; -import javax.servlet.ServletRequestEvent; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Type; import java.util.Arrays; -import java.util.HashMap; import java.util.HashSet; -import java.util.Map; import java.util.Set; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.resource.Resource; -import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper; +import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.models.annotations.Model; import org.apache.sling.models.spi.DisposalCallback; import org.apache.sling.models.spi.DisposalCallbackRegistry; @@ -43,19 +40,15 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.MockitoJUnitRunner; -import org.mockito.stubbing.Answer; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) -public class RequestDisposalTest { +public class ModelAdapterFactoryTest { @Mock private Resource resource; @@ -75,91 +68,39 @@ public void setup() { factory.injectors = Arrays.asList(new DisposedInjector()); factory.adapterImplementations.addClassesAsAdapterAndImplementation(TestModel.class); - final Map attributes = new HashMap<>(); - - doAnswer(new Answer() { - - @Override - public Void answer(InvocationOnMock invocation) { - attributes.put( - (String) invocation.getArguments()[0], - invocation.getArguments()[1]); - return null; - } - }) - .when(request) - .setAttribute(any(String.class), any()); - - when(request.getAttribute(any(String.class))).then(new Answer() { - - @Override - public Object answer(InvocationOnMock invocation) { - return attributes.get(invocation.getArguments()[0]); - } - }); callbacks = new HashSet<>(); } @Test - public void testWithInitializedRequest() { - // destroy a wrapper of the root request - SlingHttpServletRequest destroyedRequest = new SlingHttpServletRequestWrapper(request); - factory.requestInitialized(new ServletRequestEvent(servletContext, destroyedRequest)); - - // but adapt from a wrapper of a wrapper of that wrapper - SlingHttpServletRequest adaptableRequest = - new SlingHttpServletRequestWrapper(new SlingHttpServletRequestWrapper(destroyedRequest)); - - TestModel model = factory.getAdapter(adaptableRequest, TestModel.class); + public void testDisposableOnResourceResolverClose() { + TestModel model = factory.getAdapter(request, TestModel.class); assertEquals("teststring", model.testString); assertNoneDisposed(); - factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest)); + factory.resourceResolverFactory.getThreadResourceResolver().close(); assertAllDisposed(); } @Test - public void testTwoInstancesWithInitializedRequest() { - // destroy a wrapper of the root request - SlingHttpServletRequest destroyedRequest = new SlingHttpServletRequestWrapper(request); - factory.requestInitialized(new ServletRequestEvent(servletContext, destroyedRequest)); - - // but adapt from a wrapper of a wrapper of that wrapper - SlingHttpServletRequest adaptableRequest = - new SlingHttpServletRequestWrapper(new SlingHttpServletRequestWrapper(destroyedRequest)); + public void testDisposableWithPhantomResource() { + DisposalCallbackRegistryImpl.cleanupDisposables(); + final ResourceResolver resourceResolver = factory.resourceResolverFactory.getThreadResourceResolver(); + when(factory.resourceResolverFactory.getThreadResourceResolver()).thenReturn(null); - TestModel model = factory.getAdapter(adaptableRequest, TestModel.class); + TestModel model = factory.getAdapter(request, TestModel.class); assertEquals("teststring", model.testString); - - TestModel model2 = factory.getAdapter(adaptableRequest, TestModel.class); - assertEquals("teststring", model2.testString); + model = null; + System.gc(); assertNoneDisposed(); - factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest)); - - assertAllDisposed(); - } - - @Test - public void testWithUnitializedRequest() { - // destroy a wrapper of the root request - SlingHttpServletRequest destroyedRequest = new SlingHttpServletRequestWrapper(request); - - // but adapt from a wrapper of a wrapper of that wrapper - SlingHttpServletRequest adaptableRequest = - new SlingHttpServletRequestWrapper(new SlingHttpServletRequestWrapper(destroyedRequest)); - - TestModel model = factory.getAdapter(adaptableRequest, TestModel.class); - assertEquals("teststring", model.testString); - + resourceResolver.close(); assertNoneDisposed(); - factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest)); - - assertNoneDisposed(); + DisposalCallbackRegistryImpl.cleanupDisposables(); + assertAllDisposed(); } private void assertNoneDisposed() { diff --git a/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java b/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java index a0fc22f..badc29d 100644 --- a/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java +++ b/src/test/java/org/apache/sling/models/impl/OSGiInjectionTest.java @@ -20,7 +20,6 @@ import java.util.Arrays; import java.util.Collections; -import java.util.Dictionary; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.scripting.SlingBindings; @@ -50,7 +49,6 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -229,9 +227,7 @@ public void testSetOSGiModelField() throws Exception { SetOSGiModel model = factory.getAdapter(res, SetOSGiModel.class); assertNull(model); - verify(bundleContext).registerService(eq(Runnable.class), eq(factory), any(Dictionary.class)); verify(bundleContext).addBundleListener(any(BundleListener.class)); - verify(bundleContext).registerService(eq(Object.class), any(Object.class), any(Dictionary.class)); verify(bundleContext).getBundles(); verify(bundleContext).getBundle(); verifyNoMoreInteractions(res, bundleContext); diff --git a/src/test/java/org/apache/sling/models/impl/StaticInjectionAPFLoadOrderTest.java b/src/test/java/org/apache/sling/models/impl/StaticInjectionAPFLoadOrderTest.java index e06383a..89af34d 100644 --- a/src/test/java/org/apache/sling/models/impl/StaticInjectionAPFLoadOrderTest.java +++ b/src/test/java/org/apache/sling/models/impl/StaticInjectionAPFLoadOrderTest.java @@ -18,9 +18,13 @@ */ package org.apache.sling.models.impl; +import java.util.HashMap; +import java.util.Map; + import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.adapter.AdapterManager; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ResourceResolverFactory; import org.apache.sling.models.annotations.Model; import org.apache.sling.models.annotations.injectorspecific.InjectionStrategy; import org.apache.sling.models.annotations.injectorspecific.SlingObject; @@ -37,6 +41,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; /** @@ -54,6 +59,9 @@ public class StaticInjectionAPFLoadOrderTest { @Mock private ResourceResolver resourceResolver; + @Mock + private ResourceResolverFactory resourceResolverFactory; + @Mock private BindingsValuesProvidersByContext bindingsValuesProvidersByContext; @@ -64,6 +72,9 @@ public class StaticInjectionAPFLoadOrderTest { @Before public void setUp() { + lenient().when(resourceResolverFactory.getThreadResourceResolver()).thenReturn(resourceResolver); + final Map props = new HashMap<>(); + lenient().when(resourceResolver.getPropertyMap()).thenReturn(props); registerServices(); } @@ -124,6 +135,7 @@ public void testFactory_Model_Injector_WithResourceResolver() { private void registerServices() { context.registerService(BindingsValuesProvidersByContext.class, bindingsValuesProvidersByContext); context.registerService(AdapterManager.class, adapterManager); + context.registerService(ResourceResolverFactory.class, resourceResolverFactory); factory = context.registerInjectActivateService(new ModelAdapterFactory()); }