jsedding commented on code in PR #104:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/104#discussion_r1327220032
##########
src/main/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandler.java:
##########
@@ -30,45 +30,85 @@
import java.util.Dictionary;
import java.util.Hashtable;
+import java.util.Objects;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
public class FactoryRegistrationHandler implements AutoCloseable {
private static final Logger LOG =
LoggerFactory.getLogger(FactoryRegistrationHandler.class);
- private final ResourceResolverFactoryActivator activator;
+ private final ExecutorService factoryRegistrationWorker;
- private final FactoryPreconditions factoryPreconditions;
+ /**
+ * The configurationLock serializes access to the fields {@code activator}
+ * and {@code factoryPreconditions}. Each access to these fields must be
+ * guarded by this lock.
+ */
+ private final ReentrantLock configurationLock = new ReentrantLock();
- private final ExecutorService factoryRegistrationWorker;
+ /** Access only when holding configurationLock */
+ private ResourceResolverFactoryActivator activator;
+
+ /** Access only when holding configurationLock */
+ private FactoryPreconditions factoryPreconditions;
+
+ /**
+ * The registrationLock serializes access to the field {@code
factoryRegistration}.
+ * Each access to these field must be guarded by this lock.
+ */
+ private final ReentrantLock registrationLock = new ReentrantLock();
- @SuppressWarnings("java:S3077") // The field is only ever set to null or
to a new FactoryRegistration instance, which is safe.
- private volatile FactoryRegistration factoryRegistration;
+ /** Access only when holding registrationLock */
+ private FactoryRegistration factoryRegistration;
- public FactoryRegistrationHandler(
- ResourceResolverFactoryActivator activator, FactoryPreconditions
factoryPreconditions) {
- this.factoryPreconditions = factoryPreconditions;
- this.activator = activator;
+ public FactoryRegistrationHandler() {
this.factoryRegistrationWorker = Executors.newSingleThreadExecutor(
r -> new Thread(r,
ResourceResolverFactory.class.getSimpleName() + "
registration/deregistration"));
}
+ public void configure(ResourceResolverFactoryActivator activator,
FactoryPreconditions factoryPreconditions) {
+ checkClosed();
+
+ boolean reRegister;
+ try {
+ configurationLock.lock();
+ reRegister = this.activator != activator ||
!Objects.equals(this.factoryPreconditions, factoryPreconditions);
+ LOG.debug("activator differs = {}, factoryPreconditions differ =
{}", this.activator != activator, !Objects.equals(this.factoryPreconditions,
factoryPreconditions));
+ LOG.debug("factoryPreconditions {} vs {}",
this.factoryPreconditions, factoryPreconditions);
+ this.factoryPreconditions = factoryPreconditions;
+ this.activator = activator;
+ } finally {
+ configurationLock.unlock();
+ }
+
+ if (reRegister) {
+ if (this.factoryRegistration != null) {
Review Comment:
You are right to spot the missing lock. But the `registrationLock` is
required when accessing `factoryRegistration`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
