Eliminate ThreadLocals

We can nicely co-locate services with their users, eliminating one
instance of centralization.

Change-Id: Ibad16eb8241b5baa584782a2027ba0bd87b775ba
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java
index 094ec29a03ba272353c4ca60cd2cdc0c1ac3e513..4f6656e2eb4eb91b8dcdf483f1ee74179820865a 100644 (file)
--- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java
+++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilter.java
@@ -8,6 +8,7 @@
 package org.opendaylight.aaa.shiro.realm;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Verify.verifyNotNull;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.Iterables;
@@ -25,7 +26,6 @@ import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import org.apache.shiro.subject.Subject;
 import org.apache.shiro.web.filter.authz.AuthorizationFilter;
-import org.opendaylight.aaa.shiro.web.env.ThreadLocals;
 import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener;
 import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
@@ -36,6 +36,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev1
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.authorization.policies.Policies;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.http.permission.Permissions;
 import org.opendaylight.yangtools.concepts.ListenerRegistration;
+import org.opendaylight.yangtools.concepts.Registration;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -57,13 +58,24 @@ public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter
     private static final DataTreeIdentifier<HttpAuthorization> AUTHZ_CONTAINER = DataTreeIdentifier.create(
             LogicalDatastoreType.CONFIGURATION, InstanceIdentifier.create(HttpAuthorization.class));
 
+    private static final ThreadLocal<DataBroker> DATABROKER_TL = new ThreadLocal<>();
+
     private final DataBroker dataBroker;
 
     private ListenerRegistration<?> reg;
     private volatile ListenableFuture<Optional<HttpAuthorization>> authContainer;
 
     public MDSALDynamicAuthorizationFilter() {
-        dataBroker = requireNonNull(ThreadLocals.DATABROKER_TL.get());
+        this(verifyNotNull(DATABROKER_TL.get(), "MDSALDynamicAuthorizationFilter loading not prepared"));
+    }
+
+    public MDSALDynamicAuthorizationFilter(final DataBroker dataBroker) {
+        this.dataBroker = requireNonNull(dataBroker);
+    }
+
+    public static Registration prepareForLoad(final DataBroker dataBroker) {
+        DATABROKER_TL.set(requireNonNull(dataBroker));
+        return DATABROKER_TL::remove;
     }
 
     @Override
@@ -71,7 +83,7 @@ public class MDSALDynamicAuthorizationFilter extends AuthorizationFilter
         try (ReadTransaction tx = dataBroker.newReadOnlyTransaction()) {
             authContainer = tx.read(AUTHZ_CONTAINER.getDatastoreType(), AUTHZ_CONTAINER.getRootIdentifier());
         }
-        this.reg = dataBroker.registerDataTreeChangeListener(AUTHZ_CONTAINER, this);
+        reg = dataBroker.registerDataTreeChangeListener(AUTHZ_CONTAINER, this);
         return super.processPathConfig(path, config);
     }
 

diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java
index b5cfabba76e257f90c2ae9a111bfcd86bb659975..a160d80244731177e2acd7164042ade22de25add 100644 (file)
--- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java
+++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/realm/MdsalRealm.java
@@ -7,6 +7,7 @@
  */
 package org.opendaylight.aaa.shiro.realm;
 
+import static com.google.common.base.Verify.verifyNotNull;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.Iterables;
@@ -31,7 +32,6 @@ import org.opendaylight.aaa.api.shiro.principal.ODLPrincipal;
 import org.opendaylight.aaa.shiro.principal.ODLPrincipalImpl;
 import org.opendaylight.aaa.shiro.realm.util.TokenUtils;
 import org.opendaylight.aaa.shiro.realm.util.http.header.HeaderUtils;
-import org.opendaylight.aaa.shiro.web.env.ThreadLocals;
 import org.opendaylight.mdsal.binding.api.ClusteredDataTreeChangeListener;
 import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
@@ -44,6 +44,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev1
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.authentication.Roles;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.aaa.rev161214.authentication.Users;
 import org.opendaylight.yangtools.concepts.ListenerRegistration;
+import org.opendaylight.yangtools.concepts.Registration;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -60,14 +61,24 @@ public class MdsalRealm extends AuthorizingRealm implements Destroyable {
     private static final DataTreeIdentifier<Authentication> AUTH_TREE_ID = DataTreeIdentifier.create(
             LogicalDatastoreType.CONFIGURATION, InstanceIdentifier.create(Authentication.class));
 
+    private static final ThreadLocal<PasswordHashService> PASSWORD_HASH_SERVICE_TL = new ThreadLocal<>();
+    private static final ThreadLocal<DataBroker> DATABROKER_TL = new ThreadLocal<>();
+
     private final PasswordHashService passwordHashService;
     private final ListenerRegistration<?> reg;
 
     private volatile ListenableFuture<Optional<Authentication>> authentication;
 
     public MdsalRealm() {
-        this.passwordHashService = requireNonNull(ThreadLocals.PASSWORD_HASH_SERVICE_TL.get());
-        final DataBroker dataBroker = requireNonNull(ThreadLocals.DATABROKER_TL.get());
+        this(verifyLoad(PASSWORD_HASH_SERVICE_TL), verifyLoad(DATABROKER_TL));
+    }
+
+    private static <T> T verifyLoad(final ThreadLocal<T> threadLocal) {
+        return verifyNotNull(threadLocal.get(), "MdsalRealm not prepared for loading");
+    }
+
+    public MdsalRealm(final PasswordHashService passwordHashService, final DataBroker dataBroker) {
+        this.passwordHashService = requireNonNull(passwordHashService);
 
         try (ReadTransaction tx = dataBroker.newReadOnlyTransaction()) {
             authentication = tx.read(AUTH_TREE_ID.getDatastoreType(), AUTH_TREE_ID.getRootIdentifier());
@@ -79,6 +90,16 @@ public class MdsalRealm extends AuthorizingRealm implements Destroyable {
         LOG.info("MdsalRealm created");
     }
 
+    public static Registration prepareForLoad(final PasswordHashService passwordHashService,
+            final DataBroker dataBroker) {
+        PASSWORD_HASH_SERVICE_TL.set(requireNonNull(passwordHashService));
+        DATABROKER_TL.set(requireNonNull(dataBroker));
+        return () -> {
+            PASSWORD_HASH_SERVICE_TL.remove();
+            DATABROKER_TL.remove();
+        };
+    }
+
     private void onAuthenticationChanged(final Collection<DataTreeModification<Authentication>> changes) {
         final Authentication newVal = Iterables.getLast(changes).getRootNode().getDataAfter();
         LOG.debug("Updating authentication information to {}", newVal);

diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java
index f085e569430174470d40fe617534976988328e16..a5337cba7dfecde6f4994e764f1b5a328b9b89e2 100644 (file)
--- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java
+++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/AAAIniWebEnvironment.java
@@ -19,6 +19,8 @@ import org.opendaylight.aaa.api.TokenStore;
 import org.opendaylight.aaa.api.password.service.PasswordHashService;
 import org.opendaylight.aaa.cert.api.ICertificateManager;
 import org.opendaylight.aaa.shiro.realm.KeystoneAuthRealm;
+import org.opendaylight.aaa.shiro.realm.MDSALDynamicAuthorizationFilter;
+import org.opendaylight.aaa.shiro.realm.MdsalRealm;
 import org.opendaylight.aaa.shiro.realm.MoonRealm;
 import org.opendaylight.aaa.shiro.realm.TokenAuthRealm;
 import org.opendaylight.aaa.tokenauthrealm.auth.TokenAuthenticators;
@@ -94,13 +96,12 @@ class AAAIniWebEnvironment extends IniWebEnvironment {
 
     @Override
     public void init() {
-        ThreadLocals.DATABROKER_TL.set(dataBroker);
-        ThreadLocals.PASSWORD_HASH_SERVICE_TL.set(passwordHashService);
         try (
+            var filterLoad = MDSALDynamicAuthorizationFilter.prepareForLoad(dataBroker);
             var keyStoneLoad = KeystoneAuthRealm.prepareForLoad(certificateManager);
+            var mdsalLoad = MdsalRealm.prepareForLoad(passwordHashService, dataBroker);
             var moonLoad = MoonRealm.prepareForLoad(servletSupport);
             var tokenAuthLoad = TokenAuthRealm.prepareForLoad(authenticationService, tokenAuthenticators, tokenStore)) {
-
             // Initialize the Shiro environment from clustered-app-config
             final Ini ini = createIniFromClusteredAppConfig(shiroConfiguration);
             setIni(ini);
@@ -108,9 +109,6 @@ class AAAIniWebEnvironment extends IniWebEnvironment {
                 super.init();
                 return null;
             });
-        } finally {
-            ThreadLocals.DATABROKER_TL.remove();
-            ThreadLocals.PASSWORD_HASH_SERVICE_TL.remove();
         }
     }
 }

diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java
deleted file mode 100644 (file)
index 469c7fb..0000000
--- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ThreadLocals.java
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2018 Inocybe Technologies and others.  All rights reserved.
- *
- * This program and the accompanying materials are made available under the
- * terms of the Eclipse Public License v1.0 which accompanies this distribution,
- * and is available at http://www.eclipse.org/legal/epl-v10.html
- */
-package org.opendaylight.aaa.shiro.web.env;
-
-import org.opendaylight.aaa.api.password.service.PasswordHashService;
-import org.opendaylight.mdsal.binding.api.DataBroker;
-
-/**
- * Holds ThreadLocal variables used to indirectly inject instances into classes that are instantiated by the Shiro
- * lib. Not ideal but a necessary evil to avoid static instances.
- *
- * @author Thomas Pantelis
- */
-public final class ThreadLocals {
-    public static final ThreadLocal<DataBroker> DATABROKER_TL = new ThreadLocal<>();
-
-    public static final ThreadLocal<PasswordHashService> PASSWORD_HASH_SERVICE_TL = new ThreadLocal<>();
-
-    private ThreadLocals() {
-        // Hidden on purpose
-    }
-}

diff --git a/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java b/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java
index 4edb06ba7106377764fb46bce8559ac7d4f04e14..c662fa08c458b893a047c442a18f2c7f3a2cdef5 100644 (file)
--- a/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java
+++ b/aaa-shiro/impl/src/test/java/org/opendaylight/aaa/shiro/realm/MDSALDynamicAuthorizationFilterTest.java
@@ -24,9 +24,7 @@ import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import org.apache.shiro.subject.Subject;
-import org.junit.Before;
 import org.junit.Test;
-import org.opendaylight.aaa.shiro.web.env.ThreadLocals;
 import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.binding.api.ReadTransaction;
 import org.opendaylight.mdsal.common.api.ReadFailedException;
@@ -42,10 +40,6 @@ import org.opendaylight.yangtools.yang.common.Uint32;
 @SuppressWarnings("checkstyle:AbbreviationAsWordInName")
 public class MDSALDynamicAuthorizationFilterTest {
 
-    @Before
-    public void setup() {
-    }
-
     private static DataBroker mockDataBroker(final Object readData) {
         final ReadTransaction readOnlyTransaction = mock(ReadTransaction.class);
 
@@ -65,18 +59,12 @@ public class MDSALDynamicAuthorizationFilterTest {
 
     private static MDSALDynamicAuthorizationFilter newFilter(final Subject subject, final DataBroker dataBroker)
             throws ServletException {
-        ThreadLocals.DATABROKER_TL.set(dataBroker);
-        MDSALDynamicAuthorizationFilter ret;
-        try {
-            ret = new MDSALDynamicAuthorizationFilter() {
-                @Override
-                protected Subject getSubject(final ServletRequest request, final ServletResponse servletResponse) {
-                    return subject;
-                }
-            };
-        } finally {
-            ThreadLocals.DATABROKER_TL.remove();
-        }
+        final var ret = new MDSALDynamicAuthorizationFilter(dataBroker) {
+            @Override
+            protected Subject getSubject(final ServletRequest request, final ServletResponse servletResponse) {
+                return subject;
+            }
+        };
 
         ret.processPathConfig("test-path","test-config");
         return ret;