Occasionally there is a need to reload or change a collection while another thread may be using it. Poorly written code tends to produce a ConcurrentModificationException. The following example shows how:
public class Config
{
private File propertyFile;
private long lastRefresh = 0l;
private static final long REFRESH_MSECS = 30000l;
// Map of property => value
private static Map<String,String> properties;
public Config(File propertyFile)
{
this.propertyFile = propertyFile;
properties = new HashMap<String,String>();
}
public Map<String,String> getLatestProperties()
{
if (System.currentTimeMillis() > lastRefresh + REFRESH_MSECS)
{
// reload the properties from disk
properties = new HashMap<String,String>();
BufferedReader reader = null;
try
{
reader = new BufferedReader(new FileReader(propertyFile));
for(;;)
{
String line = reader.readLine();
if (line == null)
{
break;
}
String property = getLineProperty(line);
String value = getLineValue(line);
properties.put(property,value);
}
}
catch (Exception e)
{
:
}
finally
{
if (reader != null) try{reader.close();}catch (IOException e){}
}
lastRefresh = System.currentTimeMillis();
}
return properties;
}
:
}
The key collection in the class is the properties map but it's not protected by a synchronized block so when it is reloaded while other threads happen to be trying to read it, a ConcurrentModificationException will be thrown.
Perhaps getLatestProperties() should be put in a synchronized block so only one caller can update properties at a time? Perhaps also return a copy of the map in case others try to change it.
:
public synchronized Map<String,String> getLatestProperties()
{
if (System.currentTimeMillis() > lastRefresh + REFRESH_MSECS)
{
// reload the properties from disk
:
lastRefresh = System.currentTimeMillis();
}
// return a copy so we don't care if others change it
Map<String,String> ret = new HashMap<String,String>();
ret.putAll(properties);
return ret;
}
:
But this solution is flawed too. The getLatestProperties() call is used throughout the code and causes a performance bottleneck if only one thread can call it at a time. It's not that important a method (only reading properties), so there is no reason why a possibility for a bottleneck should exist.
One solution is to do no synchronized locking at all, instead relying on the lastRefresh variable to tell other threads not to try and reload configuration at the same instant. This works because changes to primitive variables and references to objects are guaranteed atomic. A change in the lastRefresh variable can be used to communicate with other threads that might be calling the code, so they don't also attempt to reload the configuration. There is a small window of opportunity for a second thread to check the if() statement before lastRefresh is called, but two threads can reload configuration no problem. The method also uses a temporary collection tempProperties which is the key to the code being thread safe. The properties collection is never changed, just switched over to a new reference when the collection has been loaded as a temporary collection.
:
public Map<String,String> getLatestProperties()
{
if (System.currentTimeMillis() > lastRefresh + REFRESH_MSECS)
{
// reload the properties from disk
lastRefresh = System.currentTimeMillis();
Map<String,String> tempProperties = new HashMap<String,String>();
BufferedReader reader = null;
try
{
reader = new BufferedReader(new FileReader(propertyFile));
for(;;)
{
String line = reader.readLine();
if (line == null)
{
break;
}
String property = getLineProperty(line);
String value = getLineValue(line);
tempProperties.put(property,value);
}
// Switch over the reference at the last moment
properties = tempProperties;
}
catch (Exception e)
{
// :
}
finally
{
if (reader != null) try{reader.close();}catch (IOException e){}
}
}
// return a copy
Map<String,String> ret = new HashMap<String,String>();
ret.putAll(properties);
return ret;
}
:
I call this technique the collection switcheroo – nothing special about it but it can often replace complex synchronized blocks.
The switcheroo also works well where there are many reading threads but very few writes or updates (also known as a Reader/Writer Lock for those who want to do more reading). The reads have to be fast, but the writes, because there are so few of them, can be slow. The writes can update collections or data structures in the background and there's no rule to say they can't be in synchronized blocks to prevent data corruption.
A full implementation of a Reader/Writer Lock involves upgrading reads to writes and all sorts of fancy stuff; plenty of examples online that do this.
This simpler example shows how to do the reads and writes using a collection switcheroo. It shows a RulesEngine class that is really just a wrapper around a Map of <rulename,Rule>, designed to be used as a singleton across the entire application. Rules are rarely updated, perhaps once an hour, but can be read thousands of times per second (read the comments for more clarity).
public class RulesEngine
{
private Map<String,Rule> rules = new HashMap<String,Rule>();
/**
* Updates the rule if it is new or is different from the one already stored.
* This is the 'gatekeeper' for the real method that updates the rule, and since it
* does no direct updating, does not need to be synchronized.
*
* @param rule The rule that has changed
*/
public void updateRule(Rule rule)
{
if (isRuleNewOrUpdated(rule))
{
doUpdateRule(rule);
}
}
/**
* Does a heavyweight update of the rule.
* This method is synchronized to ensure that no two threads are replacing the rules map
* at the same instant (otherwise one rule would be lost)
*
* @param rule The rule that has changed
*/
private synchronized void doUpdateRule(Rule rule)
{
// do all our work on a temporary rule map:
Map<String,Rule> temp = new HashMap<String,Rule>();
temp.putAll(rules);
temp.put(rule.getName(),rule);
// switch the temporary over to the real rule map - none of the readers will notice
rules = temp;
}
/**
* Note the lack of synchronized block around this getter.
* The data may be slightly out of date which is fine in this model
*
* @param ruleName Name of rule to lookup.
* @return Rule that ruleName belongs to
*/
public Rule getRule(String ruleName)
{
return rules.get(ruleName);
}
:
}
