This is an automated email from the ASF dual-hosted git repository.
gosullivan pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 81f0497 GEODE-6353: remove the ThreadLocal from SecurityConfig.
(#3154)
81f0497 is described below
commit 81f0497efd2ea299e17a16bb4355a0defe61ced0
Author: Galen O'Sullivan <[email protected]>
AuthorDate: Mon Feb 4 09:23:29 2019 -0800
GEODE-6353: remove the ThreadLocal from SecurityConfig. (#3154)
* GEODE-6353: remove the ThreadLocal from SecurityConfig.
* Extract `DistributedSystem.connect()` to
`InternalDistributedSystem.connectInternal()` so that parameters can be passed
in.
* Make `DistributedSystem.existingSystems` a
`List<InternalDistributedSystem>.
* Add a comment making it clear that users should never override
`DistributedSystem`.
* Remove commented code
---
.../java/org/apache/geode/cache/CacheFactory.java | 11 +--
.../geode/distributed/DistributedSystem.java | 106 ++-------------------
.../internal/InternalDistributedSystem.java | 81 ++++++++++++++--
.../geode/distributed/internal/SecurityConfig.java | 15 ---
4 files changed, 85 insertions(+), 128 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java
b/geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java
index 38c5265..e824769 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java
@@ -211,14 +211,9 @@ public class CacheFactory {
validateUsabilityOfSecurityCallbacks(ds);
}
if (ds == null) {
- // use ThreadLocal to avoid exposing new User API in DistributedSystem
- SecurityConfig.set(this.cacheConfig.getSecurityManager(),
- this.cacheConfig.getPostProcessor());
- try {
- ds = DistributedSystem.connect(this.dsProps);
- } finally {
- SecurityConfig.remove();
- }
+ ds = InternalDistributedSystem.connectInternal(dsProps, new
SecurityConfig(
+ this.cacheConfig.getSecurityManager(),
+ this.cacheConfig.getPostProcessor()));
}
return create(ds, true, this.cacheConfig);
}
diff --git
a/geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java
b/geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java
index ac5f266..f33f8f7 100644
---
a/geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java
+++
b/geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java
@@ -15,7 +15,6 @@
package org.apache.geode.distributed;
import static
org.apache.geode.distributed.ConfigurationProperties.CONSERVE_SOCKETS;
-import static
org.apache.geode.distributed.internal.InternalDistributedSystem.ALLOW_MULTIPLE_SYSTEMS;
import java.io.File;
import java.net.InetAddress;
@@ -23,7 +22,6 @@ import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
-import java.util.Iterator;
import java.util.List;
import java.util.Properties;
import java.util.Set;
@@ -77,6 +75,10 @@ import org.apache.geode.internal.util.IOUtils;
* program will no longer access it. Disconnecting frees up certain resources
and allows your
* application to connect to a different distributed system, if desirable.
*
+ * <P>
+ *
+ * Users should never subclass this class.
+ *
* @since GemFire 3.0
*/
public abstract class DistributedSystem implements StatisticsFactory {
@@ -86,7 +88,8 @@ public abstract class DistributedSystem implements
StatisticsFactory {
* to a distributed system is allowed in a VM. This set is never modified in
place (it is always
* read only) but the reference can be updated by holders of {@link
#existingSystemsLock}.
*/
- protected static volatile List existingSystems = Collections.EMPTY_LIST;
+ protected static volatile List<InternalDistributedSystem> existingSystems =
+ Collections.EMPTY_LIST;
/**
* This lock must be changed to add or remove a system. It is notified when
a system is removed.
*
@@ -156,66 +159,7 @@ public abstract class DistributedSystem implements
StatisticsFactory {
*
*/
public static DistributedSystem connect(Properties config) {
- if (config == null) {
- // fix for bug 33992
- config = new Properties();
- }
-
- if (ALLOW_MULTIPLE_SYSTEMS) {
- return InternalDistributedSystem.newInstance(config);
- }
-
- synchronized (existingSystemsLock) {
- if (ClusterDistributionManager.isDedicatedAdminVM()) {
- // For a dedicated admin VM, check to see if there is already
- // a connect that will suit our purposes.
- DistributedSystem existingSystem = getConnection(config);
- if (existingSystem != null) {
- return existingSystem;
- }
-
- } else {
- boolean existingSystemDisconnecting = true;
- boolean isReconnecting = false;
- while (!existingSystems.isEmpty() && existingSystemDisconnecting &&
!isReconnecting) {
- Assert.assertTrue(existingSystems.size() == 1);
-
- InternalDistributedSystem existingSystem =
- (InternalDistributedSystem) existingSystems.get(0);
- existingSystemDisconnecting = existingSystem.isDisconnecting();
- // a reconnecting DS will block on GemFireCache.class and a
ReconnectThread
- // holds that lock and invokes this method, so we break out of the
loop
- // if we detect this condition
- isReconnecting = existingSystem.isReconnectingDS();
- if (existingSystemDisconnecting) {
- boolean interrupted = Thread.interrupted();
- try {
- // no notify for existingSystemsLock, just to release the sync
- existingSystemsLock.wait(50);
- } catch (InterruptedException ex) {
- interrupted = true;
- } finally {
- if (interrupted) {
- Thread.currentThread().interrupt();
- }
- }
- } else if (existingSystem.isConnected()) {
- existingSystem.validateSameProperties(config,
existingSystem.isConnected());
- return existingSystem;
- } else {
- // This should not happen: existingSystem.isConnected()==false &&
- // existingSystem.isDisconnecting()==false
- throw new AssertionError(
- "system should not have both disconnecting==false and
isConnected==false");
- }
- }
- }
-
- // Make a new connection to the distributed system
- InternalDistributedSystem newSystem =
InternalDistributedSystem.newInstance(config);
- addSystem(newSystem);
- return newSystem;
- }
+ return InternalDistributedSystem.connectInternal(config, null);
}
protected static void addSystem(InternalDistributedSystem newSystem) {
@@ -234,7 +178,7 @@ public abstract class DistributedSystem implements
StatisticsFactory {
protected static boolean removeSystem(InternalDistributedSystem oldSystem) {
synchronized (existingSystemsLock) {
- List listOfSystems = new ArrayList(existingSystems);
+ List<InternalDistributedSystem> listOfSystems = new
ArrayList<>(existingSystems);
boolean result = listOfSystems.remove(oldSystem);
if (result) {
int size = listOfSystems.size();
@@ -282,13 +226,11 @@ public abstract class DistributedSystem implements
StatisticsFactory {
*
* @since GemFire 4.0
*/
- private static DistributedSystem getConnection(Properties config) {
+ protected static DistributedSystem getConnection(Properties config) {
// In an admin VM you can have a connection to more than one
// distributed system. If we are already connected to the desired
// distributed system, return that connection.
- List l = existingSystems;
- for (Iterator iter = l.iterator(); iter.hasNext();) {
- InternalDistributedSystem existingSystem = (InternalDistributedSystem)
iter.next();
+ for (InternalDistributedSystem existingSystem : existingSystems) {
if (existingSystem.sameSystemAs(config)) {
Assert.assertTrue(existingSystem.isConnected());
return existingSystem;
@@ -310,13 +252,7 @@ public abstract class DistributedSystem implements
StatisticsFactory {
return existing;
} else {
- // logger.info("creating new distributed system for admin");
- // for (java.util.Enumeration en=props.propertyNames();
en.hasMoreElements(); ) {
- // String prop=(String)en.nextElement();
- // logger.info(prop + "=" + props.getProperty(prop));
- // }
props.setProperty(CONSERVE_SOCKETS, "true");
- // LOG: no longer using the LogWriter that was passed in
return connect(props);
}
}
@@ -337,28 +273,6 @@ public abstract class DistributedSystem implements
StatisticsFactory {
}
}
- // /**
- // * Connects to a GemFire distributed system with a configuration
- // * supplemented by the given properties.
- // *
- // * @param config
- // * The <a href="#configuration">configuration properties</a>
- // * used when connecting to the distributed system
- // * @param callback
- // * A user-specified object that is delivered with the {@link
- // * org.apache.geode.admin.SystemMembershipEvent}
- // * triggered by connecting.
- // *
- // * @see #connect(Properties)
- // * @see org.apache.geode.admin.SystemMembershipListener#memberJoined
- // *
- // * @since GemFire 4.0
- // */
- // public static DistributedSystem connect(Properties config,
- // Object callback) {
- // throw new UnsupportedOperationException("Not implemented yet");
- // }
-
////////////////////// Constructors //////////////////////
/**
diff --git
a/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
b/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
index 25dda88..00d6851 100644
---
a/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
+++
b/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
@@ -166,12 +166,8 @@ public class InternalDistributedSystem extends
DistributedSystem
* disconnectListeners. {@link #addDisconnectListener} will not throw
ShutdownException if the
* value is Boolean.TRUE.
*/
- final ThreadLocal<Boolean> isDisconnectThread = new ThreadLocal() {
- @Override
- public Boolean initialValue() {
- return Boolean.FALSE;
- }
- };
+ private final ThreadLocal<Boolean> isDisconnectThread =
+ ThreadLocal.withInitial(() -> Boolean.FALSE);
private final StatisticsManager statisticsManager;
@@ -185,6 +181,73 @@ public class InternalDistributedSystem extends
DistributedSystem
/** services provided by other modules */
private Map<Class, DistributedSystemService> services = new HashMap<>();
+ /**
+ * If the experimental multiple-system feature is enabled, always create a
new system.
+ *
+ * Otherwise, create a new InternalDistributedSystem with the given
properties, or connect to an
+ * existing one with the same properties.
+ */
+ public static DistributedSystem connectInternal(Properties config,
+ SecurityConfig securityConfig) {
+ if (config == null) {
+ config = new Properties();
+ }
+
+ if (ALLOW_MULTIPLE_SYSTEMS) {
+ return InternalDistributedSystem.newInstance(config);
+ }
+
+ synchronized (existingSystemsLock) {
+ if (ClusterDistributionManager.isDedicatedAdminVM()) {
+ // For a dedicated admin VM, check to see if there is already
+ // a connect that will suit our purposes.
+ DistributedSystem existingSystem = getConnection(config);
+ if (existingSystem != null) {
+ return existingSystem;
+ }
+
+ } else {
+ boolean existingSystemDisconnecting = true;
+ boolean isReconnecting = false;
+ while (!existingSystems.isEmpty() && existingSystemDisconnecting &&
!isReconnecting) {
+ Assert.assertTrue(existingSystems.size() == 1);
+
+ InternalDistributedSystem existingSystem = existingSystems.get(0);
+ existingSystemDisconnecting = existingSystem.isDisconnecting();
+ // a reconnecting DS will block on GemFireCache.class and a
ReconnectThread
+ // holds that lock and invokes this method, so we break out of the
loop
+ // if we detect this condition
+ isReconnecting = existingSystem.isReconnectingDS();
+ if (existingSystemDisconnecting) {
+ boolean interrupted = Thread.interrupted();
+ try {
+ // no notify for existingSystemsLock, just to release the sync
+ existingSystemsLock.wait(50);
+ } catch (InterruptedException ex) {
+ interrupted = true;
+ } finally {
+ if (interrupted) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ } else if (existingSystem.isConnected()) {
+ existingSystem.validateSameProperties(config,
existingSystem.isConnected());
+ return existingSystem;
+ } else {
+ throw new AssertionError(
+ "system should not have both disconnecting==false and
isConnected==false");
+ }
+ }
+ }
+
+ // Make a new connection to the distributed system
+ InternalDistributedSystem newSystem =
+ InternalDistributedSystem.newInstance(config, securityConfig);
+ addSystem(newSystem);
+ return newSystem;
+ }
+ }
+
public GrantorRequestProcessor.GrantorRequestContext
getGrantorRequestContext() {
return grc;
}
@@ -252,7 +315,7 @@ public class InternalDistributedSystem extends
DistributedSystem
/**
* auto-reconnect listeners
*/
- private static List<ReconnectListener> reconnectListeners = new
ArrayList<ReconnectListener>();
+ private static List<ReconnectListener> reconnectListeners = new
ArrayList<>();
/**
* whether this DS is one created to reconnect to the distributed system
after a
@@ -299,7 +362,7 @@ public class InternalDistributedSystem extends
DistributedSystem
*/
private DistributionConfig config;
- private volatile boolean shareSockets =
DistributionConfig.DEFAULT_CONSERVE_SOCKETS;
+ private volatile boolean shareSockets;
/**
* if this distributed system starts a locator, it is stored here
@@ -344,7 +407,7 @@ public class InternalDistributedSystem extends
DistributedSystem
* Creates a new instance of <code>InternalDistributedSystem</code> with the
given configuration.
*/
public static InternalDistributedSystem newInstance(Properties config) {
- return newInstance(config, SecurityConfig.get());
+ return newInstance(config, null);
}
public static InternalDistributedSystem newInstance(Properties config,
diff --git
a/geode-core/src/main/java/org/apache/geode/distributed/internal/SecurityConfig.java
b/geode-core/src/main/java/org/apache/geode/distributed/internal/SecurityConfig.java
index deea55f..da5c6ac 100644
---
a/geode-core/src/main/java/org/apache/geode/distributed/internal/SecurityConfig.java
+++
b/geode-core/src/main/java/org/apache/geode/distributed/internal/SecurityConfig.java
@@ -18,21 +18,6 @@ import org.apache.geode.security.PostProcessor;
import org.apache.geode.security.SecurityManager;
public class SecurityConfig {
-
- private static final ThreadLocal<SecurityConfig> THREAD_LOCAL = new
ThreadLocal<>();
-
- public static void set(SecurityManager securityManager, PostProcessor
postProcessor) {
- THREAD_LOCAL.set(new SecurityConfig(securityManager, postProcessor));
- }
-
- public static SecurityConfig get() {
- return THREAD_LOCAL.get();
- }
-
- public static void remove() {
- THREAD_LOCAL.remove();
- }
-
private final SecurityManager securityManager;
private final PostProcessor postProcessor;
