rombert commented on code in PR #104:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/104#discussion_r1327152813
##########
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:
Should this check be done inside the critical region protected by the
`configurationLock`?
##########
src/main/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandler.java:
##########
@@ -77,15 +117,24 @@ public void close() {
void maybeRegisterFactory(final String unavailableName, final String
unavailableServicePid) {
if (!factoryRegistrationWorker.isShutdown()) {
LOG.debug("submitting maybeRegisterFactory");
- factoryRegistrationWorker.submit(() -> {
- final boolean preconditionsOk =
factoryPreconditions.checkPreconditions(unavailableName, unavailableServicePid);
- if ( preconditionsOk && this.factoryRegistration == null ) {
+ final boolean preconditionsOk;
+ final ResourceResolverFactoryActivator localActivator;
+ try {
+ configurationLock.lock();
+ preconditionsOk =
factoryPreconditions.checkPreconditions(unavailableName, unavailableServicePid);
+ localActivator = this.activator;
+ } finally {
+ configurationLock.unlock();
+ }
+
+ factoryRegistrationWorker.execute(() -> {
+ if (preconditionsOk && this.factoryRegistration == null) {
Review Comment:
What are the consequences of making an "unsychronized" read of
`this.factoryRegistration` in this method?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
