Thread-safe singleton service class with one-time initialization of Map field

Question

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;
    }

    ...
}

Answer 1

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.

Answer 2

Your code is not really thread-safe, since two threads might call start() concurrently and concurrently read the file and populate the map.

It's also unpleasant to use, because callers of your singleton will constantly have to check (or potentially falsely assume) that the singleton has been started already, or call start() before calling any other method to make sure it's started.

I would design it so that getInstance() always returns an initialized instance. Make sure getInstance() is synchronized to avoid two threads concurrently initializing the instance. Or use the initialization on demand holder idiom. Or even better, don't use the singleton anti-pattern, which makes the code hard to unit-test, and use a dependency injection framework instead.