klcodanr closed pull request #5: SLING-7584 - accumulating callbacks within the
scope of a single request
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/5
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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 736c344..7d93bbb 100644
--- a/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
+++ b/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
@@ -31,6 +31,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;
@@ -137,7 +138,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<>();
@@ -150,7 +151,9 @@ private void seal() {
callbacks = Collections.unmodifiableList(callbacks);
}
- private void onDisposed() {
+
+ @Override
+ public void onDisposed() {
for (DisposalCallback callback : callbacks) {
callback.onDisposed();
}
@@ -158,11 +161,60 @@ private void onDisposed() {
}
+ 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() {
@@ -173,7 +225,7 @@ private void clearDisposalCallbackRegistryQueue() {
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();
}
@@ -1072,7 +1124,7 @@ protected ThreadInvocationCounter initialValue() {
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");
@@ -1101,7 +1153,7 @@ protected ThreadInvocationCounter initialValue() {
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();
}
}
@@ -1314,7 +1366,7 @@ private Object handleBoundModelResult(Result<?> result) {
@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 99ec6b5..bcf8e4a 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 @@ public Object answer(InvocationOnMock invocation) {
return attributes.get(invocation.getArguments()[0]);
}
});
-
- callback = new TestDisposalCallback();
+ callbacks = new HashSet<>();
}
@Test
@@ -112,11 +113,33 @@ public void testWithInitializedRequest() {
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 void testWithUnitializedRequest() {
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)
@@ -154,7 +189,9 @@ public String getName() {
@Nullable
@Override
- public Object getValue(@NotNull Object o, String s, @NotNull Type
type, @NotNull AnnotatedElement annotatedElement, @NotNull
DisposalCallbackRegistry disposalCallbackRegistry) {
+ 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";
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
