I'm a novice at developing thread-safe methods. I have a configuration service, implemented as a Singleton class, that needs to be thread-safe. When the service is started, a collection of configuration files are read and stored in a map. This only needs to happen once. I have used AtomicBoolean for the isStarted state field, but I am not sure if I have done this properly:
public class ConfigServiceImpl implements ConfigService {
public static final URL PROFILE_DIR_URL =
ConfigServiceImpl.class.getClassLoader().getResource("./pageobject_config/");
private AtomicBoolean isStarted;
private Map<String,ConcurrentHashMap<String,LoadableConfig>> profiles = new ConcurrentHashMap<>();
private static final class Loader {
private static final ConfigServiceImpl INSTANCE = new ConfigServiceImpl();
}
private ConfigServiceImpl() { }
public static ConfigServiceImpl getInstance() {
return Loader.INSTANCE;
}
@Override
public void start() {
if(!isStarted()) {
try {
if (PROFILE_DIR_URL != null) {
URI resourceDirUri = PROFILE_DIR_URL.toURI();
File resourceDir = new File(resourceDirUri);
@SuppressWarnings("ConstantConditions")
List<File> files = resourceDir.listFiles() != null ?
Arrays.asList(resourceDir.listFiles()) : new ArrayList<>();
files.forEach(this::addProfile);
isStarted.compareAndSet(false, true);
}
} catch (URISyntaxException e) {
throw new IllegalStateException("Could not generate a valid URI for " + PROFILE_DIR_URL);
}
}
}
@Override
public boolean isStarted() {
return isStarted.get();
}
....
}
I wasn't sure if I should set isStarted to true before populating the map, or even if this matters at all. Would this implementation be reasonably safe in a multi-threaded environment?
UPDATE:
Using zapl's suggestion to perform all initialization in the private constructor and JB Nizet's suggestion to use getResourceAsStream():
public class ConfigServiceImpl implements ConfigService {
private static final InputStream PROFILE_DIR_STREAM =
ConfigServiceImpl.class.getClassLoader().getResourceAsStream("./pageobject_config/");
private Map<String,HashMap<String,LoadableConfig>> profiles = new HashMap<>();
private static final class Loader {
private static final ConfigServiceImpl INSTANCE = new ConfigServiceImpl();
}
private ConfigServiceImpl() {
if(PROFILE_DIR_STREAM != null) {
BufferedReader reader = new BufferedReader(new InputStreamReader(PROFILE_DIR_STREAM));
String line;
try {
while ((line = reader.readLine()) != null) {
File file = new File(line);
ObjectMapper mapper = new ObjectMapper().registerModule(new Jdk8Module());
MapType mapType = mapper.getTypeFactory()
.constructMapType(HashMap.class, String.class, LoadableConfigImpl.class);
try {
//noinspection ConstantConditions
profiles.put(file.getName(), mapper.readValue(file, mapType));
} catch (IOException e) {
throw new IllegalStateException("Could not read and process profile " + file);
}
}
reader.close();
} catch(IOException e) {
throw new IllegalStateException("Could not read file list from profile directory");
}
}
}
public static ConfigServiceImpl getInstance() {
return Loader.INSTANCE;
}
...
}
The simplest threadsafe singleton is
public class ConfigServiceImpl implements ConfigService {
private static final ConfigServiceImpl INSTANCE = new ConfigServiceImpl();
private ConfigServiceImpl() {
// all the init code here.
URI resourceDirUri = PROFILE_FIR_URL.toURI();
File resourceDir = new File(resourceDirUri);
...
}
// not synchronized because final field
public static ConfigService getInstance() { return INSTANCE; }
}
The hidden constructor contains all the initialization and since INSTANCE is a final field you're guaranteed by the Java language guarantees that only ever 1 instance is created. And since instance creation means execution of init code in the constructor you're also guaranteed that initialization is done only once. No need for isStarted()/start(). It's basically bad practice to have classes that are complicated to use correctly. Without having to start it you can't forget it.
The "problem" with this code is that the initialization happens as soon as the class is loaded. You sometime want to delay that so it happens only once someone calles getInstance(). For that you can introduce a subclass to hold INSTANCE. That subclass is only loaded by the first call of getInstance.
It's often not even necessary to enforce later loading since typically, the first time you call getInstance is the time your class is loaded anyways. It becomes relevant if you use that class for something else. Like to hold some constants. Reading those loads the class even though you didn't want to initialize all the configs.
Btw a "correct" way to use AtomicBoolean to 1-time init something goes along the lines of:
AtomicBoolean initStarted = new AtomicBoolean();
volatile boolean initDone = false;
Thing thing = null;
public Thing getThing() {
// only the 1st ever call will do this
if (initStarted.compareAndSet(false, true)) {
thing = init();
initDone = true;
return thing;
}
// all other calls will go here
if (initDone) {
return thing;
} else {
// you're stuck in a pretty undefined state
return null;
}
}
public boolean isInit() {
return initDone;
}
public boolean needsInit() {
return !initStarted.get();
}
The big problem is that in practice you want to wait until initialization is done and not return null so you'll probably never see code like that.