Reputation: 21
Is the following code thread safe?
I think I have implemented the Double-checked locking pattern but not sure if it safe or it works as intended. Any other logic to implement the same would be really helpful.
public class OnProperties {
private static String dfltPropertyFile = "on.properties";
private static long refreshSecs = 120L;
private static Properties props;
private static long lastReadTimestamp = 0;
public static String getProperty(String propertyName, String dfltValue) {
long currentTimestamp = System.currentTimeMillis() / 1000L;
if (props == null
|| (refreshSecs > 0 && (currentTimestamp - lastReadTimestamp) > refreshSecs)) {
synchronized (props) {
if (props == null
|| (refreshSecs > 0 && (currentTimestamp - lastReadTimestamp) > refreshSecs)) {
lastReadTimestamp = currentTimestamp;
try {
loadProperties(dfltPropertyFile);
refreshSecs = getProperty("on.properties.refresh", 120L);
if (refreshSecs < 0L) {
refreshSecs = 0L;
}
} catch (Exception e) {
refreshSecs = 600L;
}
}
}
}
if (props == null) {
return dfltValue;
}
String propertyValue = props.getProperty(propertyName, dfltValue);
return propertyValue;
}
public static boolean getProperty(String propertyName, boolean dfltValue) {
boolean value = dfltValue;
String strValue = getProperty(propertyName, (String) null);
if (strValue != null) {
try {
value = Boolean.parseBoolean(strValue);
} catch (NumberFormatException e) {
// just keep the default
}
}
return value;
}
private static void loadProperties(String p_propertiesFile)
throws java.io.IOException, java.io.FileNotFoundException {
InputStream fileStream = new FileInputStream(p_propertiesFile);
props = new Properties();
props.load(fileStream);
fileStream.close();
}
}
Generally multiple threads running often access the "getProperty" method as follows:
extDebug = OnProperties.getProperty("on.extdebug", false);
Upvotes: 2
Views: 620
Answers (5)
Reputation: 6538
Atomic values guarantee to always return the complete latest value to all threads. This prevents a number of multi-threading issues in this case. A bit of synchronization is still required, but it can be limited to a minimum. See my implementation below:
import java.io.File;
import java.io.FileInputStream;
import java.util.Properties;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
public class OnProperties {
private static int refreshIntervalDefaultSecs;
private static int refreshIntervalOnErrorSecs;
static {
setRefreshInterval(120);
}
private static final AtomicReference<Properties> propsRef = new AtomicReference<Properties>(new Properties());
private static final AtomicLong nextPropsLoad = new AtomicLong(0L);
private static final Object loadLock = new Object();
private static String dfltPropertyFile = "on.properties";
public static String getProperty(String key, String defaultValue) {
String value = getProperty(key);
if (value == null) {
value = defaultValue;
}
return value;
}
private static String getProperty(String key) {
reloadWhenNeeded();
return propsRef.get().getProperty(key);
}
private static void reloadWhenNeeded() {
long now = System.currentTimeMillis();
if (now > nextPropsLoad.get()) {
boolean reload = false;
synchronized(loadLock) {
if (now > nextPropsLoad.get()) {
// need loadLock because there is time between previous get()
// and next set()
updateNextPropsLoad(now, refreshIntervalDefaultSecs);
reload = true;
}
}
if (reload) {
reloadProps(now);
}
}
}
private static void updateNextPropsLoad(long now, int nextRefreshSecs) {
nextPropsLoad.set(now + nextRefreshSecs * 1000);
}
private static void reloadProps(long now) {
Properties p = new Properties();
FileInputStream in = null;
System.out.println("Reloading from " + new File(dfltPropertyFile).getAbsolutePath());
try {
p.load(in = new FileInputStream(new File(dfltPropertyFile)));
propsRef.set(p);
setRefreshInterval(getProperty("on.properties.refresh", 120));
updateNextPropsLoad(now, refreshIntervalDefaultSecs);
} catch (Exception e) {
updateNextPropsLoad(now, refreshIntervalOnErrorSecs);
} finally {
try { if (in != null) in.close(); } catch (Exception e) {
updateNextPropsLoad(now, refreshIntervalOnErrorSecs);
}
}
}
private static void setRefreshInterval(int refreshSecs) {
if (refreshSecs < 1) {
refreshSecs = 120;
}
refreshIntervalDefaultSecs = refreshSecs;
refreshIntervalOnErrorSecs = 5 * refreshSecs;
}
public static boolean getProperty(String key, boolean defaultValue) {
boolean value = defaultValue;
String svalue = getProperty(key);
if (svalue != null) {
try {
value = Boolean.valueOf(svalue);
} catch (Exception ignored) {}
}
return value;
}
public static int getProperty(String key, int defaultValue) {
int value = defaultValue;
String svalue = getProperty(key);
if (svalue != null) {
try {
value = Integer.valueOf(svalue);
} catch (Exception ignored) {}
}
return value;
}
public static void main(String[] args) {
System.out.println("Refresh value from file: " + getProperty("on.properties.refresh", 120));
System.out.println("No reload " + getProperty("does.not.exist", true));
System.out.println("Next reload after " + ((nextPropsLoad.get() - System.currentTimeMillis()) / 1000) + " seconds.");
}
}
One drawback of the implementation is that one thread will get slowed down when it is selected to reload the properties from file. A better approach would be to create a 'watchdog' thread/scheduled task that checks every (for example) five seconds if the properties-file has a changed modification date and then trigger a reload (in which case the AtomicReference for the Properties still comes in handy).
Also keep in mind that there is a logical threading issue: if property values are interrelated (i.e. one value is only correct if another value is also updated), a reload could present a thread with old and new values that should not be mixed. The only way around that is to keep a reference to one set of properties in methods that use the interrelated values of the properties (and a class like this with static methods and variables is not handy in such a situation).
Upvotes: 1
Reputation: 24444
Please accept that the double-checked locking idiom is broken and does not work (i.e. does not synchronize properly). Even if you make it work using volatile (at the right place), it is far too complex for what you get.
So my suggestion: Simply synchronize everything. Then try and measure. If you find out that OnProperties is the bottleneck, consider more powerful/clever synchronization techniques and come back if necessary:
public class OnProperties {
/* some private fields here */
public static synchronized String getProperty(String propertyName, String dfltValue) {
reloadPropertiesIfNecessary();
return props.getProperty(propertyName, dfltValue);
}
/* other public methods using getProperty come here */
private static void reloadPropertiesIfNecessary() {
// check timestamp etc.
if (/* check timestamp etc. */) {
loadProperties(dfltPropertyFile);
// update timestamp etc.
}
}
private static void loadProperties(String filename) throws IOException {
try (InputStream stream = new FileInputStream(filename)) {
props = new Properties();
props.load(fileStream);
}
}
}
Upvotes: 0
Reputation: 329
- To reload the properties, you don't need to re-initialize the props variable. Initialize the properties during the declaration statement itself will do. This will solve the problem of synchronizing with null.
- Remove the initialization code in the loadProperties block.
- remove the prop==null check outside and inside the synchronized block.
Once that is done, your code will work exactly the way you want.
public class OnProperties {
private static String dfltPropertyFile = "on.properties"; private static long refreshSecs = 120L; private static Properties props = new Properties(); private static long lastReadTimestamp = 0; public static String getProperty(String propertyName, String dfltValue) { long currentTimestamp = System.currentTimeMillis() / 1000L; if (refreshSecs > 0 && (currentTimestamp - lastReadTimestamp) > refreshSecs) { synchronized (props) { if (refreshSecs > 0 && (currentTimestamp - lastReadTimestamp) > refreshSecs) { lastReadTimestamp = currentTimestamp; try { loadProperties(dfltPropertyFile); refreshSecs = getProperty("on.properties.refresh", 120L); if (refreshSecs < 0L) { refreshSecs = 0L; } } catch (Exception e) { refreshSecs = 600L; } } } } String propertyValue = props.getProperty(propertyName, dfltValue); return propertyValue; } public static boolean getProperty(String propertyName, boolean dfltValue) { boolean value = dfltValue; String strValue = getProperty(propertyName, (String) null); if (strValue != null) { try { value = Boolean.parseBoolean(strValue); } catch (NumberFormatException e) { // just keep the default } } return value; } private static void loadProperties(String p_propertiesFile) throws java.io.IOException, java.io.FileNotFoundException { InputStream fileStream = new FileInputStream(p_propertiesFile); props.load(fileStream); fileStream.close(); } }
Upvotes: 0
Reputation: 4057
To have this working correctly with double-checked locking you must do two things:
private static Properties propsmust be declared volatile;- as already mentioned, synchronised(props) won't work in case props are null - you need to declare a special lock object field:
.
private static final Object propsLockObject = new Object();
...
synchronized(propsLockObject) {
...
P.S. The lastReadTimestamp won't work also unless declared volatile. Though this is not about double-checked locking anymore.
Upvotes: 0
Reputation: 6306
It is not safe as you have multiple variables which are read in a way which is not thread safe (i.e. access is not synchronized and they are not volatile).
It appears the workflow is mostly reads with a few writes. I would suggest using a ReentrantReadWriteLock to synchronize access.
Upvotes: 0