Безопасный однопользовательский сервисный класс с однократной инициализацией поля карты

Question

Я новичок в разработке потокобезопасных методов. У меня есть служба конфигурации, реализованная как класс Singleton, которая должна быть потокобезопасной. Когда служба запускается, коллекция конфигурационных файлов считывается и сохраняется на карте. Это нужно только однажды. Я использовал AtomicBoolean для isStarted поля состояния, но я не уверен, что если бы я сделал это правильно:

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

    .... 
} 

Я не был уверен, должен ли я установить isStarted к true перед тем заселению карты, или даже если это вообще. Будет ли эта реализация достаточно надежной в многопоточной среде?

UPDATE:

Используя предложение zapl, чтобы выполнить все инициализации в частном конструктору и предложение JB Nizet на использование 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

Самого простой поточно Синглтон

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

Скрытый Конструктор содержит все инициализации и так INSTANCE является final поля вы гарантированы языковыми гарантиями Java, что создается только когда-либо один экземпляр. И поскольку создание экземпляра означает выполнение кода init в конструкторе, вы также гарантируете, что инициализация выполняется только один раз. Нет необходимости в isStarted()/start(). Практически плохая практика - это классы, которые сложно использовать правильно. Без его начала вы не можете забыть об этом.

«Проблема» с этим кодом заключается в том, что инициализация происходит, как только класс загружается. Вы когда-нибудь хотите отложить это, так что это происходит только один раз, когда кто-то звонит getInstance(). Для этого вы можете ввести подкласс для хранения INSTANCE. Этот подкласс загружается только первым вызовом getInstance.

Часто бывает необязательно выполнять последующую загрузку, поскольку, как правило, при первом вызове getInstance это время, когда ваш класс загружен. Это становится актуальным, если вы используете этот класс для чего-то другого. Как держать некоторые константы. Чтение этих нагрузок класса, даже если вы не хотели инициализировать все конфиги.

---

Btw «правильный» способ использовать AtomicBoolean к 1-время инициализации что-то идет по линии:

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

Большая проблема заключается в том, что на практике вы хотите ждать, пока инициализация не выполняется и не возвращать null, поэтому вы, вероятно, никогда не увидите такой код.

Answer 2

Ваш код действительно не поточно-, так как два потока могли бы назвать start() одновременно и одновременно читать файл и заполнять карту.

Это также неприятно для использования, потому что вызывающим абонентам вашего синглтона придется постоянно проверять (или, возможно, ложно предположить), что синглтон уже запущен, или вызвать start() перед вызовом любого другого метода, чтобы убедиться, что он запущен.

Я бы разработал его так, чтобы getInstance() всегда возвращал инициализированный экземпляр. Убедитесь, что getInstance() синхронизирован, чтобы избежать одновременного инициализации экземпляра двумя потоками. Или используйте initialization on demand holder idiom. Или даже лучше, не используйте singleton anti-pattern, что делает код сложным для модульного тестирования и вместо этого использует инфраструктуру инъекций зависимостей.