You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by oh...@apache.org on 2013/06/01 21:54:28 UTC
svn commit: r1488570 - in /commons/proper/configuration/trunk/src:
main/java/org/apache/commons/configuration/
test/java/org/apache/commons/configuration/
Author: oheger
Date: Sat Jun 1 19:54:27 2013
New Revision: 1488570
URL: http://svn.apache.org/r1488570
Log:
Reworked DynamicCombinedConfiguration regarding thread-safety.
The class now uses its Synchronizer to protect its internal state against
concurrent access. All tests could be enabled again (after configuring a
fully functional Synchronizer).
Modified:
commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/DynamicCombinedConfiguration.java
commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/SynchronizerTestImpl.java
commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/TestDynamicCombinedConfiguration.java
Modified: commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/DynamicCombinedConfiguration.java
URL: http://svn.apache.org/viewvc/commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/DynamicCombinedConfiguration.java?rev=1488570&r1=1488569&r2=1488570&view=diff
==============================================================================
--- commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/DynamicCombinedConfiguration.java (original)
+++ commons/proper/configuration/trunk/src/main/java/org/apache/commons/configuration/DynamicCombinedConfiguration.java Sat Jun 1 19:54:27 2013
@@ -41,13 +41,28 @@ import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
/**
- * DynamicCombinedConfiguration allows a set of CombinedConfigurations to be used. Each CombinedConfiguration
- * is referenced by a key that is dynamically constructed from a key pattern on each call. The key pattern
- * will be resolved using the configured ConfigurationInterpolator.
+ * <p>
+ * DynamicCombinedConfiguration allows a set of CombinedConfigurations to be
+ * used.
+ * </p>
+ * <p>
+ * Each CombinedConfiguration is referenced by a key that is dynamically
+ * constructed from a key pattern on each call. The key pattern will be resolved
+ * using the configured ConfigurationInterpolator.
+ * </p>
+ * <p>
+ * This Configuration implementation uses the configured {@code Synchronizer} to
+ * guard itself against concurrent access. If there are multiple threads
+ * accessing an instance concurrently, a fully functional {@code Synchronizer}
+ * implementation (e.g. {@code ReadWriteSynchronizer}) has to be used to ensure
+ * consistency and to avoid exceptions. The {@code Synchronizer} assigned to an
+ * instance is also passed to child configuration objects when they are created.
+ * </p>
+ *
* @since 1.6
* @author <a
- * href="http://commons.apache.org/configuration/team-list.html">Commons
- * Configuration team</a>
+ * href="http://commons.apache.org/configuration/team-list.html">Commons
+ * Configuration team</a>
* @version $Id$
*/
public class DynamicCombinedConfiguration extends CombinedConfiguration
@@ -186,11 +201,19 @@ public class DynamicCombinedConfiguratio
public void addConfiguration(Configuration config, String name,
String at)
{
- ConfigData cd = new ConfigData(config, name, at);
- configurations.add(cd);
- if (name != null)
+ beginWrite();
+ try
{
- namedConfigurations.put(name, config);
+ ConfigData cd = new ConfigData(config, name, at);
+ configurations.add(cd);
+ if (name != null)
+ {
+ namedConfigurations.put(name, config);
+ }
+ }
+ finally
+ {
+ endWrite();
}
}
/**
@@ -202,7 +225,15 @@ public class DynamicCombinedConfiguratio
@Override
public int getNumberOfConfigurations()
{
- return configurations.size();
+ beginRead();
+ try
+ {
+ return configurations.size();
+ }
+ finally
+ {
+ endRead();
+ }
}
/**
@@ -216,8 +247,16 @@ public class DynamicCombinedConfiguratio
@Override
public Configuration getConfiguration(int index)
{
- ConfigData cd = configurations.get(index);
- return cd.getConfiguration();
+ beginRead();
+ try
+ {
+ ConfigData cd = configurations.get(index);
+ return cd.getConfiguration();
+ }
+ finally
+ {
+ endRead();
+ }
}
/**
@@ -230,7 +269,15 @@ public class DynamicCombinedConfiguratio
@Override
public Configuration getConfiguration(String name)
{
- return namedConfigurations.get(name);
+ beginRead();
+ try
+ {
+ return namedConfigurations.get(name);
+ }
+ finally
+ {
+ endRead();
+ }
}
/**
@@ -244,7 +291,15 @@ public class DynamicCombinedConfiguratio
@Override
public Set<String> getConfigurationNames()
{
- return namedConfigurations.keySet();
+ beginRead();
+ try
+ {
+ return namedConfigurations.keySet();
+ }
+ finally
+ {
+ endRead();
+ }
}
/**
@@ -274,16 +329,24 @@ public class DynamicCombinedConfiguratio
@Override
public boolean removeConfiguration(Configuration config)
{
- for (int index = 0; index < getNumberOfConfigurations(); index++)
+ beginWrite();
+ try
{
- if (configurations.get(index).getConfiguration() == config)
+ for (int index = 0; index < getNumberOfConfigurations(); index++)
{
- removeConfigurationAt(index);
-
+ if (configurations.get(index).getConfiguration() == config)
+ {
+ removeConfigurationAt(index);
+ return true;
+ }
}
- }
- return super.removeConfiguration(config);
+ return false;
+ }
+ finally
+ {
+ endWrite();
+ }
}
/**
@@ -295,13 +358,22 @@ public class DynamicCombinedConfiguratio
@Override
public Configuration removeConfigurationAt(int index)
{
- ConfigData cd = configurations.remove(index);
- if (cd.getName() != null)
+ beginWrite();
+ try
+ {
+ ConfigData cd = configurations.remove(index);
+ if (cd.getName() != null)
+ {
+ namedConfigurations.remove(cd.getName());
+ }
+ return cd.getConfiguration();
+ }
+ finally
{
- namedConfigurations.remove(cd.getName());
+ endWrite();
}
- return super.removeConfigurationAt(index);
}
+
/**
* Returns the configuration root node of this combined configuration. This
* method will construct a combined node structure using the current node
@@ -812,39 +884,20 @@ public class DynamicCombinedConfiguratio
// The double-checked works here due to the Thread guarantees of ConcurrentMap.
if (config == null)
{
- synchronized (configs)
+ beginWrite();
+ try
{
config = configs.get(key);
if (config == null)
{
- config = new CombinedConfiguration(getNodeCombiner());
- if (loggerName != null)
- {
- Log log = LogFactory.getLog(loggerName);
- if (log != null)
- {
- config.setLogger(log);
- }
- }
- config.setExpressionEngine(this.getExpressionEngine());
- config.setDelimiterParsingDisabled(isDelimiterParsingDisabled());
- config.setConversionExpressionEngine(getConversionExpressionEngine());
- config.setListDelimiter(getListDelimiter());
- for (ConfigurationErrorListener listener : getErrorListeners())
- {
- config.addErrorListener(listener);
- }
- for (ConfigurationListener listener : getConfigurationListeners())
- {
- config.addConfigurationListener(listener);
- }
- for (ConfigData data : configurations)
- {
- config.addConfiguration(data.getConfiguration(), data.getName(), data.getAt());
- }
+ config = createChildConfiguration();
configs.put(key, config);
}
}
+ finally
+ {
+ endWrite();
+ }
}
if (getLogger().isDebugEnabled())
{
@@ -854,6 +907,46 @@ public class DynamicCombinedConfiguratio
}
/**
+ * Creates a new child configuration. This method creates a new
+ * {@code CombinedConfiguration} object and initializes it with properties
+ * from this instance.
+ *
+ * @return the new child configuration
+ */
+ private CombinedConfiguration createChildConfiguration()
+ {
+ CombinedConfiguration config;
+ config = new CombinedConfiguration(getNodeCombiner());
+ if (loggerName != null)
+ {
+ Log log = LogFactory.getLog(loggerName);
+ if (log != null)
+ {
+ config.setLogger(log);
+ }
+ }
+ config.setExpressionEngine(this.getExpressionEngine());
+ config.setDelimiterParsingDisabled(isDelimiterParsingDisabled());
+ config.setConversionExpressionEngine(getConversionExpressionEngine());
+ config.setListDelimiter(getListDelimiter());
+ for (ConfigurationErrorListener listener : getErrorListeners())
+ {
+ config.addErrorListener(listener);
+ }
+ for (ConfigurationListener listener : getConfigurationListeners())
+ {
+ config.addConfigurationListener(listener);
+ }
+ for (ConfigData data : configurations)
+ {
+ config.addConfiguration(data.getConfiguration(), data.getName(),
+ data.getAt());
+ }
+ config.setSynchronizer(getSynchronizer());
+ return config;
+ }
+
+ /**
* Creates a {@code ConfigurationInterpolator} instance for performing local
* variable substitutions. This implementation returns an object which
* shares the prefix lookups from this configuration's
Modified: commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/SynchronizerTestImpl.java
URL: http://svn.apache.org/viewvc/commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/SynchronizerTestImpl.java?rev=1488570&r1=1488569&r2=1488570&view=diff
==============================================================================
--- commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/SynchronizerTestImpl.java (original)
+++ commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/SynchronizerTestImpl.java Sat Jun 1 19:54:27 2013
@@ -100,6 +100,18 @@ public class SynchronizerTestImpl implem
}
/**
+ * Verifies that the specified sequence of methods was called somewhere in
+ * the interaction with the synchronizer.
+ *
+ * @param expMethods the expected methods
+ */
+ public void verifyContains(Methods... expMethods)
+ {
+ assertTrue("Expected methods not found: " + methods, methods.toString()
+ .indexOf(constructExpectedMethods(expMethods)) >= 0);
+ }
+
+ /**
* Generates a string with expected methods from the given array.
*
* @param expMethods the array with expected methods
Modified: commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/TestDynamicCombinedConfiguration.java
URL: http://svn.apache.org/viewvc/commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/TestDynamicCombinedConfiguration.java?rev=1488570&r1=1488569&r2=1488570&view=diff
==============================================================================
--- commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/TestDynamicCombinedConfiguration.java (original)
+++ commons/proper/configuration/trunk/src/test/java/org/apache/commons/configuration/TestDynamicCombinedConfiguration.java Sat Jun 1 19:54:27 2013
@@ -20,6 +20,8 @@ package org.apache.commons.configuration
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import java.io.File;
@@ -30,6 +32,7 @@ import java.io.Reader;
import java.io.Writer;
import java.util.Random;
+import org.apache.commons.configuration.SynchronizerTestImpl.Methods;
import org.apache.commons.configuration.builder.BuilderConfigurationWrapperFactory;
import org.apache.commons.configuration.builder.ConfigurationBuilder;
import org.apache.commons.configuration.builder.FileBasedBuilderParametersImpl;
@@ -40,8 +43,9 @@ import org.apache.commons.configuration.
import org.apache.commons.configuration.interpol.ConfigurationInterpolator;
import org.apache.commons.configuration.interpol.Lookup;
import org.apache.commons.configuration.io.FileHandler;
+import org.apache.commons.configuration.sync.LockMode;
+import org.apache.commons.configuration.sync.ReadWriteSynchronizer;
import org.apache.commons.configuration.tree.xpath.XPathExpressionEngine;
-import org.junit.Ignore;
import org.junit.Test;
public class TestDynamicCombinedConfiguration
@@ -102,12 +106,111 @@ public class TestDynamicCombinedConfigur
assertEquals("a\\,b\\,c", config.getString("split/list2"));
}
- @Test @Ignore
+ /**
+ * Prepares a test for calling the Synchronizer. This method creates a test
+ * Synchronizer, installs it at the configuration and returns it.
+ *
+ * @param config the configuration
+ * @return the test Synchronizer
+ */
+ private SynchronizerTestImpl prepareSynchronizerTest(Configuration config)
+ {
+ config.lock(LockMode.READ);
+ config.unlock(LockMode.READ); // ensure that root node is constructed
+ SynchronizerTestImpl sync = new SynchronizerTestImpl();
+ config.setSynchronizer(sync);
+ return sync;
+ }
+
+ /**
+ * Tests whether adding a configuration is synchronized.
+ */
+ @Test
+ public void testAddConfigurationSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ config.addConfiguration(new PropertiesConfiguration());
+ sync.verify(Methods.BEGIN_WRITE, Methods.END_WRITE);
+ }
+
+ /**
+ * Tests whether querying the number of configurations is synchronized.
+ */
+ @Test
+ public void testGetNumberOfConfigurationsSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ config.getNumberOfConfigurations();
+ sync.verify(Methods.BEGIN_READ, Methods.END_READ);
+ }
+
+ /**
+ * Tests whether querying a configuration by index is synchronized.
+ */
+ @Test
+ public void testGetConfigurationByIdxSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ Configuration child = new PropertiesConfiguration();
+ config.addConfiguration(child);
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ assertSame("Wrong configuration", child, config.getConfiguration(0));
+ sync.verify(Methods.BEGIN_READ, Methods.END_READ);
+ }
+
+ /**
+ * Tests whether querying a configuration by name is synchronized.
+ */
+ @Test
+ public void testGetConfigurationByNameSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ assertNull("Wrong result", config.getConfiguration("unknown config"));
+ sync.verify(Methods.BEGIN_READ, Methods.END_READ);
+ }
+
+ /**
+ * Tests whether querying the set of configuration names is synchronized.
+ */
+ @Test
+ public void testGetConfigurationNamesSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ config.getConfigurationNames();
+ sync.verify(Methods.BEGIN_READ, Methods.END_READ);
+ }
+
+ /**
+ * Tests whether removing a child configuration is synchronized.
+ */
+ @Test
+ public void testRemoveConfigurationSynchronized()
+ {
+ DynamicCombinedConfiguration config =
+ new DynamicCombinedConfiguration();
+ String configName = "testConfig";
+ config.addConfiguration(new PropertiesConfiguration(), configName);
+ SynchronizerTestImpl sync = prepareSynchronizerTest(config);
+ config.removeConfiguration(configName);
+ sync.verifyContains(Methods.BEGIN_WRITE);
+ }
+
+ @Test
public void testConcurrentGetAndReload() throws Exception
{
System.getProperties().remove("Id");
CombinedConfigurationBuilder builder = new CombinedConfigurationBuilder();
- builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE));
+ builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE)
+ .setSynchronizer(new ReadWriteSynchronizer()));
CombinedConfiguration config = builder.getConfiguration();
assertEquals("Wrong value", "50", config.getString("rowsPerPage"));
@@ -129,12 +232,13 @@ public class TestDynamicCombinedConfigur
assertEquals(totalFailures + " failures Occurred", 0, totalFailures);
}
- @Test @Ignore
+ @Test
public void testConcurrentGetAndReload2() throws Exception
{
System.getProperties().remove("Id");
CombinedConfigurationBuilder builder = new CombinedConfigurationBuilder();
- builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE));
+ builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE)
+ .setSynchronizer(new ReadWriteSynchronizer()));
CombinedConfiguration config = builder.getConfiguration();
assertEquals(config.getString("rowsPerPage"), "50");
@@ -159,12 +263,13 @@ public class TestDynamicCombinedConfigur
assertEquals(totalFailures + " failures Occurred", 0, totalFailures);
}
- @Test @Ignore
+ @Test
public void testConcurrentGetAndReloadMultipleClients() throws Exception
{
System.getProperties().remove("Id");
CombinedConfigurationBuilder builder = new CombinedConfigurationBuilder();
- builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE));
+ builder.configure(Parameters.fileBased().setFile(MULTI_TENENT_FILE)
+ .setSynchronizer(new ReadWriteSynchronizer()));
CombinedConfiguration config = builder.getConfiguration();
assertEquals(config.getString("rowsPerPage"), "50");
@@ -213,6 +318,7 @@ public class TestDynamicCombinedConfigur
new ReloadingCombinedConfigurationBuilder();
builder.configure(Parameters
.combined()
+ .setSynchronizer(new ReadWriteSynchronizer())
.setDefinitionBuilderParameters(
new FileBasedBuilderParametersImpl()
.setFile(MULTI_DYNAMIC_FILE))