SLING-7584 - accumulating callbacks within the scope of a single request
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 3f50ad8..fe1fd90 100644
--- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
+++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
@@ -30,6 +30,7 @@
import java.util.Collections;
import java.util.Dictionary;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Hashtable;
import java.util.Iterator;
import java.util.List;
@@ -136,7 +137,7 @@
private static final Object REQUEST_MARKER_VALUE = new Object();
- private static class DisposalCallbackRegistryImpl implements DisposalCallbackRegistry {
+ private static class DisposalCallbackRegistryImpl implements DisposalCallbackRegistry, Disposable {
private List<DisposalCallback> callbacks = new ArrayList<>();
@@ -149,7 +150,9 @@
callbacks = Collections.unmodifiableList(callbacks);
}
- private void onDisposed() {
+
+ @Override
+ public void onDisposed() {
for (DisposalCallback callback : callbacks) {
callback.onDisposed();
}
@@ -157,11 +160,60 @@
}
+ private static class CombinedDisposable implements Disposable {
+ private final Collection<Disposable> delegates = Collections.synchronizedCollection(new HashSet<Disposable>());
+
+ private void add(Disposable disposable) {
+ delegates.add(disposable);
+ }
+
+ @Override
+ public void onDisposed() {
+ for (Disposable delegate : delegates) {
+ delegate.onDisposed();
+ }
+ }
+ }
+
+ private interface Disposable {
+ void onDisposed();
+ }
+
+ private static class RequestDisposalCallbacks {
+ private ConcurrentHashMap<ServletRequest, Disposable> callbacks = new ConcurrentHashMap<>();
+
+ public Collection<Disposable> values() {
+ return callbacks.values();
+ }
+
+ public Disposable remove(ServletRequest request) {
+ return callbacks.remove(request);
+ }
+
+ public void put(ServletRequest request, Disposable registry) {
+ synchronized (callbacks) {
+ CombinedDisposable combinedDisposable = null;
+ Disposable current = callbacks.get(request);
+ if (current == null) {
+ callbacks.put(request, registry);
+ return;
+ } else if (current instanceof CombinedDisposable) {
+ combinedDisposable = (CombinedDisposable) current;
+ } else {
+ combinedDisposable = new CombinedDisposable();
+ combinedDisposable.add(current);
+ callbacks.put(request, combinedDisposable);
+ }
+ combinedDisposable.add(registry);
+ }
+ }
+ }
+
private ReferenceQueue<Object> queue;
- private ConcurrentMap<java.lang.ref.Reference<Object>, DisposalCallbackRegistryImpl> disposalCallbacks;
+ private ConcurrentMap<java.lang.ref.Reference<Object>, Disposable> disposalCallbacks;
- private ConcurrentHashMap<ServletRequest, DisposalCallbackRegistryImpl> requestDisposalCallbacks;
+ private RequestDisposalCallbacks requestDisposalCallbacks;
@Override
public void run() {
@@ -172,7 +224,7 @@
java.lang.ref.Reference<?> ref = queue.poll();
while (ref != null) {
log.debug("calling disposal for {}.", ref.toString());
- DisposalCallbackRegistryImpl registry = disposalCallbacks.remove(ref);
+ Disposable registry = disposalCallbacks.remove(ref);
registry.onDisposed();
ref = queue.poll();
}
@@ -1070,7 +1122,7 @@
BundleContext bundleContext = ctx.getBundleContext();
this.queue = new ReferenceQueue<>();
this.disposalCallbacks = new ConcurrentHashMap<>();
- this.requestDisposalCallbacks = new ConcurrentHashMap<>();
+ this.requestDisposalCallbacks = new RequestDisposalCallbacks();
Hashtable<String, Object> properties = new Hashtable<>();
properties.put(Constants.SERVICE_VENDOR, "Apache Software Foundation");
properties.put(Constants.SERVICE_DESCRIPTION, "Sling Models OSGi Service Disposal Job");
@@ -1099,7 +1151,7 @@
protected void deactivate() {
this.adapterCache = null;
if (this.requestDisposalCallbacks != null) {
- for (final DisposalCallbackRegistryImpl requestRegistries : this.requestDisposalCallbacks.values()) {
+ for (final Disposable requestRegistries : this.requestDisposalCallbacks.values()) {
requestRegistries.onDisposed();
}
}
@@ -1312,7 +1364,7 @@
@Override
public void requestDestroyed(ServletRequestEvent sre) {
ServletRequest request = unwrapRequest(sre.getServletRequest());
- DisposalCallbackRegistryImpl registry = requestDisposalCallbacks.remove(request);
+ Disposable registry = requestDisposalCallbacks.remove(request);
if (registry != null) {
registry.onDisposed();
}
diff --git a/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java b/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java
index b0c30a9..8b612eb 100644
--- a/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java
+++ b/src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java
@@ -41,8 +41,10 @@
import java.lang.reflect.AnnotatedElement;
import java.lang.reflect.Type;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Hashtable;
import java.util.Map;
+import java.util.Set;
import static org.mockito.Mockito.*;
import static org.junit.Assert.*;
@@ -66,7 +68,7 @@
private ModelAdapterFactory factory;
- private TestDisposalCallback callback;
+ private Set<TestDisposalCallback> callbacks;
@Before
public void setup() {
@@ -96,8 +98,7 @@
return attributes.get(invocation.getArguments()[0]);
}
});
-
- callback = new TestDisposalCallback();
+ callbacks = new HashSet<>();
}
@Test
@@ -112,11 +113,33 @@
TestModel model = factory.getAdapter(adaptableRequest, TestModel.class);
assertEquals("teststring", model.testString);
- assertFalse(callback.isDisposed());
+ assertNoneDisposed();
factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest));
- assertTrue(callback.isDisposed());
+ 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));
+
+ TestModel model = factory.getAdapter(adaptableRequest, TestModel.class);
+ assertEquals("teststring", model.testString);
+
+ TestModel model2 = factory.getAdapter(adaptableRequest, TestModel.class);
+ assertEquals("teststring", model2.testString);
+
+ assertNoneDisposed();
+
+ factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest));
+
+ assertAllDisposed();
}
@Test
@@ -130,11 +153,23 @@
TestModel model = factory.getAdapter(adaptableRequest, TestModel.class);
assertEquals("teststring", model.testString);
- assertFalse(callback.isDisposed());
+ assertNoneDisposed();
factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest));
- assertFalse(callback.isDisposed());
+ assertNoneDisposed();
+ }
+
+ private void assertNoneDisposed() {
+ for (TestDisposalCallback callback : callbacks) {
+ assertFalse(callback.isDisposed());
+ }
+ }
+
+ private void assertAllDisposed() {
+ for (TestDisposalCallback callback : callbacks) {
+ assertTrue(callback.isDisposed());
+ }
}
@Model(adaptables = SlingHttpServletRequest.class)
@@ -155,6 +190,8 @@
@CheckForNull
@Override
public Object getValue(@Nonnull Object o, String s, @Nonnull Type type, @Nonnull AnnotatedElement annotatedElement, @Nonnull DisposalCallbackRegistry disposalCallbackRegistry) {
+ TestDisposalCallback callback = new TestDisposalCallback();
+ callbacks.add(callback);
disposalCallbackRegistry.addDisposalCallback(callback);
return "teststring";
}
