SLING-7584 - accumulating callbacks within the scope of a single request
authorJustin Edelson <justin@apache.org>
Mon, 16 Apr 2018 18:21:40 +0000 (14:21 -0400)
committerJustin Edelson <justin@apache.org>
Mon, 16 Apr 2018 18:21:40 +0000 (14:21 -0400)
src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
src/test/java/org/apache/sling/models/impl/RequestDisposalTest.java

index 3f50ad8..fe1fd90 100644 (file)
@@ -30,6 +30,7 @@ import java.util.Collection;
 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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
 
     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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
             callbacks = Collections.unmodifiableList(callbacks);
         }
 
-        private void onDisposed() {
+
+        @Override
+        public void onDisposed() {
             for (DisposalCallback callback : callbacks) {
                 callback.onDisposed();
             }
@@ -157,11 +160,60 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
 
     }
 
+    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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
         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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
         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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
     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 @@ public class ModelAdapterFactory implements AdapterFactory, Runnable, ModelFacto
     @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();
         }
index b0c30a9..8b612eb 100644 (file)
@@ -41,8 +41,10 @@ import javax.servlet.ServletRequestEvent;
 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 @@ public class RequestDisposalTest {
 
     private ModelAdapterFactory factory;
 
-    private TestDisposalCallback callback;
+    private Set<TestDisposalCallback> callbacks;
 
     @Before
     public void setup() {
@@ -96,8 +98,7 @@ public class RequestDisposalTest {
                 return attributes.get(invocation.getArguments()[0]);
             }
         });
-
-        callback = new TestDisposalCallback();
+        callbacks = new HashSet<>();
     }
 
     @Test
@@ -112,11 +113,33 @@ public class RequestDisposalTest {
         TestModel model = factory.getAdapter(adaptableRequest, TestModel.class);
         assertEquals("teststring", model.testString);
 
-        assertFalse(callback.isDisposed());
+        assertNoneDisposed();
+
+        factory.requestDestroyed(new ServletRequestEvent(servletContext, destroyedRequest));
+
+        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));
 
-        assertTrue(callback.isDisposed());
+        assertAllDisposed();
     }
 
     @Test
@@ -130,11 +153,23 @@ public class RequestDisposalTest {
         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 @@ public class RequestDisposalTest {
         @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";
         }