Get rid of synchronized maps and sets
diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java
index 427cce6..24a9580 100644
--- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java
+++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java
@@ -1,8 +1,7 @@
package org.apache.commons.jcs.auxiliary.disk.jdbc;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.Iterator;
+import java.util.concurrent.CopyOnWriteArraySet;
/*
* Licensed to the Apache Software Foundation (ASF) under one
@@ -39,8 +38,8 @@
private static final Log log = LogManager.getLog( ShrinkerThread.class );
/** A set of JDBCDiskCache objects to call deleteExpired on. */
- private final Set<JDBCDiskCache<?, ?>> shrinkSet =
- Collections.synchronizedSet( new HashSet<>() );
+ private final CopyOnWriteArraySet<JDBCDiskCache<?, ?>> shrinkSet =
+ new CopyOnWriteArraySet<>();
/** Default time period to use. */
private static final long DEFAULT_PAUSE_BETWEEN_REGION_CALLS_MILLIS = 5000;
@@ -98,40 +97,29 @@
log.info( "Running JDBC disk cache shrinker. Number of regions [{0}]",
() -> shrinkSet.size() );
- Object[] caches = null;
-
- synchronized ( shrinkSet )
+ for (Iterator<JDBCDiskCache<?, ?>> i = shrinkSet.iterator(); i.hasNext();)
{
- caches = this.shrinkSet.toArray();
- }
+ JDBCDiskCache<?, ?> cache = i.next();
+ long start = System.currentTimeMillis();
+ int deleted = cache.deleteExpired();
+ long end = System.currentTimeMillis();
- if ( caches != null )
- {
- for ( int i = 0; i < caches.length; i++ )
+ log.info( "Deleted [{0}] expired for region [{1}] for table [{2}] in {3} ms.",
+ deleted, cache.getCacheName(), cache.getTableName(), end - start );
+
+ // don't pause after the last call to delete expired.
+ if ( i.hasNext() )
{
- JDBCDiskCache<?, ?> cache = (JDBCDiskCache<?, ?>) caches[i];
+ log.info( "Pausing for [{0}] ms before shrinking the next region.",
+ this.getPauseBetweenRegionCallsMillis() );
- long start = System.currentTimeMillis();
- int deleted = cache.deleteExpired();
- long end = System.currentTimeMillis();
-
- log.info( "Deleted [{0}] expired for region [{1}] for table [{2}] in {3} ms.",
- deleted, cache.getCacheName(), cache.getTableName(), end - start );
-
- // don't pause after the last call to delete expired.
- if ( i < caches.length - 1 )
+ try
{
- log.info( "Pausing for [{0}] ms before shrinking the next region.",
- this.getPauseBetweenRegionCallsMillis() );
-
- try
- {
- Thread.sleep( this.getPauseBetweenRegionCallsMillis() );
- }
- catch ( InterruptedException e )
- {
- log.warn( "Interrupted while waiting to delete expired for the next region." );
- }
+ Thread.sleep( this.getPauseBetweenRegionCallsMillis() );
+ }
+ catch ( InterruptedException e )
+ {
+ log.warn( "Interrupted while waiting to delete expired for the next region." );
}
}
}
diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java
index e2e964d..186e46b 100644
--- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java
+++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java
@@ -23,9 +23,7 @@
import java.sql.DriverManager;
import java.sql.SQLException;
import java.sql.Statement;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.concurrent.CopyOnWriteArraySet;
import org.apache.commons.jcs.auxiliary.AuxiliaryCacheAttributes;
import org.apache.commons.jcs.auxiliary.disk.jdbc.JDBCDiskCache;
@@ -49,7 +47,7 @@
private static final Log log = LogManager.getLog( HSQLDiskCacheFactory.class );
/** The databases. */
- private Set<String> databases;
+ private CopyOnWriteArraySet<String> databases;
/**
* This factory method should create an instance of the hsqlcache.
@@ -79,7 +77,7 @@
public void initialize()
{
super.initialize();
- this.databases = Collections.synchronizedSet( new HashSet<>() );
+ this.databases = new CopyOnWriteArraySet<>();
}
/**
@@ -105,31 +103,34 @@
return;
}
- // TODO get this from the attributes.
- System.setProperty( "hsqldb.cache_scale", "8" );
-
- // "org.hsqldb.jdbcDriver"
- String driver = attributes.getDriverClassName();
- // "sa"
- String user = attributes.getUserName();
- // ""
- String password = attributes.getPassword();
-
- try
+ synchronized (databases)
{
- Class.forName( driver ).newInstance();
+ // TODO get this from the attributes.
+ System.setProperty( "hsqldb.cache_scale", "8" );
+
+ // "org.hsqldb.jdbcDriver"
+ String driver = attributes.getDriverClassName();
+ // "sa"
+ String user = attributes.getUserName();
+ // ""
+ String password = attributes.getPassword();
+
+ try
+ {
+ Class.forName( driver ).newInstance();
+ }
+ catch (Exception e)
+ {
+ throw new SQLException( "Could not initialize driver " + driver, e );
+ }
+
+ Connection cConn = DriverManager.getConnection( database, user, password );
+ setupTable( cConn, attributes.getTableName() );
+
+ log.info( "Finished setting up database [{0}]", database);
+
+ databases.add( database );
}
- catch (Exception e)
- {
- throw new SQLException( "Could not initialize driver " + driver, e );
- }
-
- Connection cConn = DriverManager.getConnection( database, user, password );
- setupTable( cConn, attributes.getTableName() );
-
- log.info( "Finished setting up database [{0}]", database);
-
- databases.add( database );
}
/**
diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java
index 94d37d8..6816ab2 100644
--- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java
+++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java
@@ -1,11 +1,9 @@
package org.apache.commons.jcs.auxiliary.lateral.socket.tcp;
import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.CopyOnWriteArrayList;
/*
* Licensed to the Apache Software Foundation (ASF) under one
@@ -53,15 +51,15 @@
* Map of no wait facades. these are used to determine which regions are locally configured to
* use laterals.
*/
- private final Map<String, LateralCacheNoWaitFacade<?, ?>> facades =
- Collections.synchronizedMap( new HashMap<>() );
+ private final ConcurrentMap<String, LateralCacheNoWaitFacade<?, ?>> facades =
+ new ConcurrentHashMap<>();
/**
* List of regions that are configured differently here than on another server. We keep track of
* this to limit the amount of info logging.
*/
- private final Set<String> knownDifferentlyConfiguredRegions =
- Collections.synchronizedSet( new HashSet<>() );
+ private final CopyOnWriteArrayList<String> knownDifferentlyConfiguredRegions =
+ new CopyOnWriteArrayList<>();
/** The name of the cache factory */
private String factoryName;
@@ -91,7 +89,7 @@
* @param facade - facade (for region) => multiple lateral clients.
* @return true if the facade was not already registered.
*/
- public synchronized boolean addNoWaitFacade( String cacheName, LateralCacheNoWaitFacade<?, ?> facade )
+ public boolean addNoWaitFacade( String cacheName, LateralCacheNoWaitFacade<?, ?> facade )
{
boolean isNew = !containsNoWaitFacade( cacheName );
@@ -123,7 +121,9 @@
public <K, V> boolean containsNoWait( String cacheName, LateralCacheNoWait<K, V> noWait )
{
@SuppressWarnings("unchecked") // Need to cast because of common map for all facades
- LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
+ LateralCacheNoWaitFacade<K, V> facade =
+ (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
+
if ( facade == null )
{
return false;
@@ -141,13 +141,14 @@
* services.
* <p>
* @param noWait
- * @return true if we found the no wait and added it. False if the no wait was not present or it
+ * @return true if we found the no wait and added it. False if the no wait was not present or if
* we already had it.
*/
protected <K, V> boolean addNoWait( LateralCacheNoWait<K, V> noWait )
{
@SuppressWarnings("unchecked") // Need to cast because of common map for all facades
- LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
+ LateralCacheNoWaitFacade<K, V> facade =
+ (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
log.debug( "addNoWait > Got facade for {0} = {1}", noWait.getCacheName(), facade );
if ( facade != null )
@@ -158,12 +159,11 @@
}
else
{
- if ( !knownDifferentlyConfiguredRegions.contains( noWait.getCacheName() ) )
+ if ( knownDifferentlyConfiguredRegions.addIfAbsent( noWait.getCacheName() ) )
{
log.info( "addNoWait > Different nodes are configured differently "
+ "or region [{0}] is not yet used on this side.",
() -> noWait.getCacheName() );
- knownDifferentlyConfiguredRegions.add( noWait.getCacheName() );
}
return false;
}
@@ -179,7 +179,8 @@
protected <K, V> boolean removeNoWait( LateralCacheNoWait<K, V> noWait )
{
@SuppressWarnings("unchecked") // Need to cast because of common map for all facades
- LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
+ LateralCacheNoWaitFacade<K, V> facade =
+ (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() );
log.debug( "removeNoWait > Got facade for {0} = {1}", noWait.getCacheName(), facade);
if ( facade != null )
@@ -190,12 +191,11 @@
}
else
{
- if ( !knownDifferentlyConfiguredRegions.contains( noWait.getCacheName() ) )
+ if ( knownDifferentlyConfiguredRegions.addIfAbsent( noWait.getCacheName() ) )
{
- log.info( "removeNoWait > Different nodes are configured differently "
+ log.info( "addNoWait > Different nodes are configured differently "
+ "or region [{0}] is not yet used on this side.",
() -> noWait.getCacheName() );
- knownDifferentlyConfiguredRegions.add( noWait.getCacheName() );
}
return false;
}
diff --git a/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java b/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java
index 0775fc0..48932cb 100644
--- a/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java
+++ b/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java
@@ -19,16 +19,15 @@
* under the License.
*/
-import java.util.ArrayList;
-import java.util.Collections;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicInteger;
-import junit.framework.TestCase;
-
import org.apache.commons.jcs.access.GroupCacheAccess;
import org.apache.commons.jcs.access.exception.CacheException;
+import junit.framework.TestCase;
+
/**
* Test Case for JCS-73, modeled after the Groovy code by Alexander Kleymenov
*
@@ -67,7 +66,7 @@
JCS.setConfigFilename( "/TestJCS-73.ccf" );
cache = JCS.getGroupCacheInstance( "cache" );
errcount = new AtomicInteger(0);
- valueMismatchList = Collections.synchronizedList(new ArrayList<>());
+ valueMismatchList = new CopyOnWriteArrayList<>();
}
@Override
