You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2018/10/17 16:43:30 UTC
[GitHub] klcodanr closed pull request #5: SLING-7584 - accumulating
callbacks within the scope of a single request
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:
users@infra.apache.org
With regards,
Apache Git Services