This is an automated email from the ASF dual-hosted git repository.
jsedding pushed a commit to branch
fix/SLING-12019-duplicate-ResourceResolverFactory-registration
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
commit eb0d3eaded8d9113e4cc15aef3c9b136b6c4b1e9
Author: Julian Sedding <jsedd...@apache.org>
AuthorDate: Wed Sep 6 11:59:59 2023 +0200
SLING-12019 - Avoid duplicate ResourceResolverFactory registrations
---
.../impl/ResourceResolverFactoryActivator.java | 190 +++++++++++----------
.../stateful/AuthenticatedResourceProvider.java | 9 +-
.../impl/MockedResourceResolverImplTest.java | 19 ++-
.../impl/mapping/ResourceMapperImplTest.java | 14 +-
4 files changed, 137 insertions(+), 95 deletions(-)
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
index 9e727c7..5e809aa 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
@@ -22,6 +22,11 @@ import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.*;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.TimeUnit;
import org.apache.commons.collections4.BidiMap;
@@ -32,6 +37,7 @@ import org.apache.sling.api.resource.ResourceResolverFactory;
import org.apache.sling.api.resource.path.Path;
import org.apache.sling.api.resource.runtime.RuntimeService;
import org.apache.sling.resourceresolver.impl.helper.ResourceDecoratorTracker;
+import org.apache.sling.resourceresolver.impl.mapping.MapEntries;
import org.apache.sling.resourceresolver.impl.mapping.Mapping;
import
org.apache.sling.resourceresolver.impl.mapping.StringInterpolationProvider;
import
org.apache.sling.resourceresolver.impl.observation.ResourceChangeListenerWhiteboard;
@@ -68,17 +74,50 @@ import org.slf4j.LoggerFactory;
@Component(name =
"org.apache.sling.jcr.resource.internal.JcrResourceResolverFactoryImpl")
public class ResourceResolverFactoryActivator {
-
private static final class FactoryRegistration {
/** Registration .*/
- public volatile ServiceRegistration<ResourceResolverFactory>
factoryRegistration;
+ private final ServiceRegistration<ResourceResolverFactory>
factoryRegistration;
/** Runtime registration. */
- public volatile ServiceRegistration<RuntimeService>
runtimeRegistration;
+ private final ServiceRegistration<RuntimeService> runtimeRegistration;
- public volatile CommonResourceResolverFactoryImpl commonFactory;
- }
+ private final CommonResourceResolverFactoryImpl commonFactory;
+ FactoryRegistration(BundleContext context,
ResourceResolverFactoryActivator activator) {
+ commonFactory = new CommonResourceResolverFactoryImpl(activator);
+ commonFactory.activate(context);
+
+ // activate and register factory
+ final Dictionary<String, Object> serviceProps = new Hashtable<>();
+ serviceProps.put(Constants.SERVICE_VENDOR, "The Apache Software
Foundation");
+ serviceProps.put(Constants.SERVICE_DESCRIPTION, "Apache Sling
Resource Resolver Factory");
+ factoryRegistration =
context.registerService(ResourceResolverFactory.class,
+ new ServiceFactory<ResourceResolverFactory>() {
+
+ @Override
+ public ResourceResolverFactory getService(final Bundle
bundle, final ServiceRegistration<ResourceResolverFactory> registration) {
+ if (activator.isDeactivating) {
+ // TODO - how can this happen?
+ return null;
+ }
+ return new
ResourceResolverFactoryImpl(commonFactory, bundle,
activator.getServiceUserMapper());
+ }
+
+ @Override
+ public void ungetService(final Bundle bundle, final
ServiceRegistration<ResourceResolverFactory> registration, final
ResourceResolverFactory service) {
+ // nothing to do
+ }
+ }, serviceProps);
+
+ runtimeRegistration =
context.registerService(RuntimeService.class, activator.getRuntimeService(),
null);
+ }
+
+ void unregister() {
+ runtimeRegistration.unregister();
+ factoryRegistration.unregister();
+ commonFactory.deactivate();
+ }
+ }
/** Logger. */
private final Logger logger = LoggerFactory.getLogger(this.getClass());
@@ -145,6 +184,15 @@ public class ResourceResolverFactoryActivator {
/** Factory registration. */
private volatile FactoryRegistration factoryRegistration;
+ /** Queue to serialize registration and unregistration of the
ResourceResolverFactory ServiceFactory */
+ private final BlockingQueue<Runnable> factoryRegistrationTasks = new
LinkedBlockingQueue<>();
+
+ private ExecutorService factoryRegistrationWorker;
+
+ private static final Runnable POISON_PILL_TASK = () -> {};
+
+ private volatile boolean isDeactivating = false;
+
/**
* Get the resource decorator tracker.
*/
@@ -425,20 +473,18 @@ public class ResourceResolverFactoryActivator {
@Override
public void providerAdded() {
- if ( factoryRegistration == null ) {
- checkFactoryPreconditions(null, null);
- }
-
+ factoryRegistrationTasks.add(
+ () -> maybeRegisterFactory(null, null));
}
@Override
public void providerRemoved(final String name, final
String pid, final boolean stateful, final boolean isUsed) {
- if ( factoryRegistration != null ) {
+ factoryRegistrationTasks.add(() -> {
if ( isUsed && (stateful ||
config.resource_resolver_providerhandling_paranoid()) ) {
unregisterFactory();
}
- checkFactoryPreconditions(name, pid);
- }
+ maybeRegisterFactory(name, pid);
+ });
}
});
} else {
@@ -446,8 +492,20 @@ public class ResourceResolverFactoryActivator {
requiredResourceProvidersLegacy,
requiredResourceProviderNames,
resourceProviderTracker);
- this.checkFactoryPreconditions(null, null);
- }
+ factoryRegistrationTasks.add(() -> maybeRegisterFactory(null,
null));
+ }
+ factoryRegistrationWorker = Executors.newSingleThreadExecutor(
+ r -> new Thread(r, "ResourceResolverFactory
registration/deregistration"));
+ factoryRegistrationWorker.submit(() -> {
+ try {
+ Runnable task;
+ while ((task = factoryRegistrationTasks.take()) !=
POISON_PILL_TASK) {
+ task.run();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ });
}
/**
@@ -456,6 +514,7 @@ public class ResourceResolverFactoryActivator {
@Modified
protected void modified(final BundleContext bundleContext, final
ResourceResolverFactoryConfig config) {
this.deactivate();
+ this.isDeactivating = false;
this.activate(bundleContext, config);
}
@@ -464,7 +523,18 @@ public class ResourceResolverFactoryActivator {
*/
@Deactivate
protected void deactivate() {
- this.unregisterFactory();
+ this.isDeactivating = true;
+ this.factoryRegistrationTasks.add(this::unregisterFactory);
+ this.factoryRegistrationTasks.add(POISON_PILL_TASK);
+ this.factoryRegistrationWorker.shutdown();
+ try {
+ if (!this.factoryRegistrationWorker.awaitTermination(5,
TimeUnit.SECONDS)) {
+ this.factoryRegistrationWorker.shutdownNow();
+ this.unregisterFactory();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
this.bundleContext = null;
this.config = DEFAULT_CONFIG;
@@ -475,11 +545,6 @@ public class ResourceResolverFactoryActivator {
this.preconds.deactivate();
this.resourceDecoratorTracker.close();
-
- // this is just a sanity call to make sure that unregister
- // in the case that a registration happened again
- // while deactivation
- this.unregisterFactory();
}
/**
@@ -490,28 +555,13 @@ public class ResourceResolverFactoryActivator {
private void unregisterFactory() {
FactoryRegistration local = null;
synchronized ( this ) {
- if ( this.factoryRegistration != null ) {
+ if (this.factoryRegistration != null) {
local = this.factoryRegistration;
this.factoryRegistration = null;
}
}
- this.unregisterFactory(local);
- }
-
- /**
- * Unregister the provided factory
- */
- private void unregisterFactory(final FactoryRegistration local) {
- if ( local != null ) {
- if ( local.factoryRegistration != null ) {
- local.factoryRegistration.unregister();
- }
- if ( local.runtimeRegistration != null ) {
- local.runtimeRegistration.unregister();
- }
- if ( local.commonFactory != null ) {
- local.commonFactory.deactivate();
- }
+ if (local != null) {
+ local.unregister();
}
}
@@ -519,40 +569,12 @@ public class ResourceResolverFactoryActivator {
* Try to register the factory.
*/
private void registerFactory(final BundleContext localContext) {
- final FactoryRegistration local = new FactoryRegistration();
-
- if ( localContext != null ) {
- // activate and register factory
- final Dictionary<String, Object> serviceProps = new Hashtable<>();
- serviceProps.put(Constants.SERVICE_VENDOR, "The Apache Software
Foundation");
- serviceProps.put(Constants.SERVICE_DESCRIPTION, "Apache Sling
Resource Resolver Factory");
-
- local.commonFactory = new CommonResourceResolverFactoryImpl(this);
- local.commonFactory.activate(localContext);
- local.factoryRegistration = localContext.registerService(
- ResourceResolverFactory.class, new
ServiceFactory<ResourceResolverFactory>() {
-
- @Override
- public ResourceResolverFactory getService(final Bundle
bundle, final ServiceRegistration<ResourceResolverFactory> registration) {
- if (
ResourceResolverFactoryActivator.this.bundleContext == null ) {
- return null;
- }
- final ResourceResolverFactoryImpl r = new
ResourceResolverFactoryImpl(
- local.commonFactory, bundle,
-
ResourceResolverFactoryActivator.this.getServiceUserMapper());
- return r;
- }
-
- @Override
- public void ungetService(final Bundle bundle, final
ServiceRegistration<ResourceResolverFactory> registration, final
ResourceResolverFactory service) {
- // nothing to do
- }
- }, serviceProps);
-
- local.runtimeRegistration =
localContext.registerService(RuntimeService.class,
- this.getRuntimeService(), null);
-
- this.factoryRegistration = local;
+ if (!this.isDeactivating) {
+ synchronized (this) {
+ if (this.factoryRegistration == null) {
+ this.factoryRegistration = new
FactoryRegistration(localContext, this);
+ }
+ }
}
}
@@ -575,27 +597,17 @@ public class ResourceResolverFactoryActivator {
/**
* Check the preconditions and if it changed, either register factory or
unregister
*/
- private void checkFactoryPreconditions(final String unavailableName, final
String unavailableServicePid) {
- final BundleContext localContext = this.getBundleContext();
- if ( localContext != null ) {
- final boolean result =
this.preconds.checkPreconditions(unavailableName, unavailableServicePid);
- if ( result && this.factoryRegistration == null ) {
+ private void maybeRegisterFactory(final String unavailableName, final
String unavailableServicePid) {
+ if (!this.isDeactivating) {
+ final boolean preconditionsOk =
this.preconds.checkPreconditions(unavailableName, unavailableServicePid);
+ if ( preconditionsOk && this.factoryRegistration == null ) {
// check system bundle state - if stopping, don't register new
factory
+ final BundleContext localContext = this.getBundleContext();
final Bundle systemBundle =
localContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION);
if ( systemBundle != null && systemBundle.getState() !=
Bundle.STOPPING ) {
- boolean create = true;
- synchronized ( this ) {
- if ( this.factoryRegistration == null ) {
- this.factoryRegistration = new
FactoryRegistration();
- } else {
- create = false;
- }
- }
- if ( create ) {
- this.registerFactory(localContext);
- }
+ this.registerFactory(localContext);
}
- } else if ( !result && this.factoryRegistration != null ) {
+ } else if ( !preconditionsOk && this.factoryRegistration != null )
{
this.unregisterFactory();
}
}
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java
b/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java
index 453b5cf..7207b57 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java
@@ -52,7 +52,7 @@ public class AuthenticatedResourceProvider {
private static final Logger logger =
LoggerFactory.getLogger(ResourceResolverImpl.class);
- public static final AuthenticatedResourceProvider UNAUTHENTICATED_PROVIDER
= new AuthenticatedResourceProvider(null, false, null, null);
+ public static final AuthenticatedResourceProvider UNAUTHENTICATED_PROVIDER
= new AuthenticatedResourceProvider();
private final ResourceProviderHandler providerHandler;
@@ -79,6 +79,13 @@ public class AuthenticatedResourceProvider {
this.useRAS = useRAS;
}
+ private AuthenticatedResourceProvider() {
+ this.providerHandler = null;
+ this.resolveContext = null;
+ this.tracker = null;
+ this.useRAS = false;
+ }
+
/**
* Get the resolve context.
* @return The resolve context
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java
index 7b21023..fa764ae 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java
@@ -26,6 +26,7 @@ import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.same;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -40,6 +41,8 @@ import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
import javax.servlet.http.HttpServletRequest;
@@ -149,13 +152,20 @@ public class MockedResourceResolverImplTest {
@SuppressWarnings("unchecked")
@Before
- public void before() throws LoginException {
+ public void before() throws LoginException, InterruptedException {
activator = new ResourceResolverFactoryActivator();
// system bundle access
final Bundle systemBundle = mock(Bundle.class);
Mockito.when(systemBundle.getState()).thenReturn(Bundle.ACTIVE);
Mockito.when(bundleContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)).thenReturn(systemBundle);
+ CountDownLatch factoryRegistrationDone = new CountDownLatch(1);
+
Mockito.when(bundleContext.registerService(same(ResourceResolverFactory.class),
any(ServiceFactory.class), any(Dictionary.class)))
+ .thenAnswer(invocation -> {
+ factoryRegistrationDone.countDown();
+ return mock(ServiceReference.class);
+ });
+
activator.resourceAccessSecurityTracker = new
ResourceAccessSecurityTracker();
activator.resourceProviderTracker = resourceProviderTracker;
activator.changeListenerWhiteboard = resourceChangeListenerWhiteboard;
@@ -310,14 +320,17 @@ public class MockedResourceResolverImplTest {
Mockito.when(usingBundle.getBundleContext()).thenReturn(usingBundleContext);
Mockito.when(usingBundleContext.getBundle()).thenReturn(usingBundle);
+ factoryRegistrationDone.await(5, TimeUnit.SECONDS);
+
// extract any services that were registered into a map.
ArgumentCaptor<Class> classesCaptor =
ArgumentCaptor.forClass(Class.class);
ArgumentCaptor<ServiceFactory> serviceCaptor =
ArgumentCaptor.forClass(ServiceFactory.class);
@SuppressWarnings("rawtypes")
ArgumentCaptor<Dictionary> propertiesCaptor =
ArgumentCaptor.forClass(Dictionary.class);
Mockito.verify(bundleContext, Mockito.atLeastOnce()).registerService(
- (Class<ResourceResolverFactory>)classesCaptor.capture(),
(ServiceFactory<ResourceResolverFactory>)serviceCaptor.capture(),
- propertiesCaptor.capture());
+ (Class<ResourceResolverFactory>) classesCaptor.capture(),
+
(ServiceFactory<ResourceResolverFactory>)serviceCaptor.capture(),
+ propertiesCaptor.capture());
int si = 0;
List<ServiceFactory> serviceList = serviceCaptor.getAllValues();
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
index ddfd024..42bbfe0 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
@@ -32,6 +32,7 @@ import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
+import java.util.concurrent.TimeUnit;
import javax.servlet.http.HttpServletRequest;
@@ -53,6 +54,7 @@ import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
+import org.osgi.util.tracker.ServiceTracker;
/**
* Validates that the {@link ResourceMapperImpl} correctly queries all sources
of mappings
@@ -94,7 +96,7 @@ public class ResourceMapperImplTest {
}
@Before
- public void prepare() throws LoginException {
+ public void prepare() throws LoginException, InterruptedException {
ctx.registerInjectActivateService(new ServiceUserMapperImpl());
ctx.registerInjectActivateService(new ResourceAccessSecurityTracker());
@@ -139,7 +141,15 @@ public class ResourceMapperImplTest {
ctx.registerInjectActivateService(new
ResourceResolverFactoryActivator(),
"resource.resolver.optimize.alias.resolution",
optimiseAliasResolution);
- ResourceResolverFactory factory =
ctx.getService(ResourceResolverFactory.class);
+ final ResourceResolverFactory factory;
+ final ServiceTracker<ResourceResolverFactory, ResourceResolverFactory>
tracker =
+ new ServiceTracker<>(ctx.bundleContext(),
ResourceResolverFactory.class, null);
+ try {
+ tracker.open();
+ factory = tracker.waitForService(TimeUnit.SECONDS.toMillis(5));
+ } finally {
+ tracker.close();
+ }
assertNotNull(factory);
