java应该同步单例类公共方法吗？

Question

我有一个为我的应用程序抽象elasticsearch API的单例包装类

public class ElasticSearchClient {    

    private static volatile ElasticSearchClient elasticSearchClientInstance;

    private static final Object lock = new Object();

    private static elasticConfig ; 

    /*
    ** Private constructor to make this class singleton
    */
    private ElasticSearchClient() {
    }

    /*
    ** This method does a lazy initialization and returns the singleton instance of ElasticSearchClient
    */
    public static ElasticSearchClient getInstance() {
        ElasticSearchClient elasticSearchClientInstanceToReturn = elasticSearchClientInstance;
        if (elasticSearchClientInstanceToReturn == null) {
            synchronized(lock) {
                elasticSearchClientInstanceToReturn = elasticSearchClientInstance;
                if (elasticSearchClientInstanceToReturn == null) {
                    // While this thread was waiting for the lock, another thread may have instantiated the clinet.
                    elasticSearchClientInstanceToReturn = new ElasticSearchClient();
                    elasticSearchClientInstance = elasticSearchClientInstanceToReturn;
                }
            }
        }
        return elasticSearchClientInstanceToReturn;
    }

    /*
    ** This method creates a new elastic index with the name as the paramater, if if does not already exists.
    *  Returns true if the index creation is successful, false otherwise.
     */
    public boolean createElasticIndex(String index) {
        if (checkIfElasticSearchIndexExists(index)) {
            LOG.error("Cannot recreate already existing index: " + index);
            return false;
        }
        if (elasticConfig == null || elasticConfig.equals(BatchConstants.EMPTY_STRING)) {
            loadElasticConfigFromFile(ELASTIC_CONFIG_FILE_NAME);
        }
        if (elasticConfig != null && !elasticConfig.equals("")) {
            try {
                HttpURLConnection elasticSearchHttpURLConnection = performHttpRequest(
                    ELASTIC_SEARCH_URL + "/" + index,
                    "PUT",
                    elasticConfig,
                    "Create index: " + index
                );

                return elasticSearchHttpURLConnection != null &&
                       elasticSearchHttpURLConnection.getResponseCode() == HttpURLConnection.HTTP_OK;
            } catch (Exception e) {
             LOG.error("Unable to access Elastic Search API. Following exception occurred:\n" + e.getMessage());
          }
        } else {
            LOG.error("Found empty config file");
        }
        return false;
    }

private void loadElasticConfigFromFile(String filename) {

    try {
        Object obj = jsonParser.parse(new FileReader(filename);
        JSONObject jsonObject = (JSONObject) obj;
        LOG.info("Successfully parsed elastic config file: "+ filename);
        elasticConfig = jsonObject.toString();
        return;
    } catch (Exception e) {
        LOG.error("Cannot read elastic config from  " + filename + "\n" + e.getMessage());
        elasticConfig = "";
    }
}

}

我有多个线程使用ElasticSearchClient，如下所述

Thread1
ElasticSearchClient elasticSearchClient = ElasticSearchClient.getInstance()
elasticSearchClient.createElasticIndex("firstindex");

Thread2
ElasticSearchClient elasticSearchClient = ElasticSearchClient.getInstance()
elasticSearchClient.createElasticIndex("secondindex");

Thread3...

在我看来，Singleton类是线程安全的，但我不确定如果多个线程开始执行Singleton类的同一个方法，会发生什么。这有副作用吗

注意：我知道上面的singleton类不是反射和序列化安全的

Answer 1

在您的特定实现中

if (checkIfElasticSearchIndexExists(index)) { //NOT THREAD SAFE
            LOG.error("Cannot recreate already existing index: " + index);
            return false;
        }
        if (elasticConfig == null || elasticConfig.equals(BatchConstants.EMPTY_STRING)) { //NOT THREAD SAFE
            loadElasticConfigFromFile(ELASTIC_CONFIG_FILE_NAME);
        }
        if (elasticConfig != null && !elasticConfig.equals("")) { //NOT THREAD SAFE

有3点可能会导致赛车状况

所以本质上

Should singleton class public methods be synchronized?

没有这样的规则——如果这些规则是线程安全的，那么就不需要额外的同步。在你的情况下，这些不是线程安全的，因此你必须使它们安全

public synchronized boolean createElasticIndex

如果你担心并发写入单个索引，那么就不要这样做——正确处理并发写入是一项弹性搜索任务（相信我，ES会顺利处理）

什么是线程不安全的（指出3处）？同时具有T1和T2：

1. checkIfElasticSearchIndexExists(index)如果T1和T2使用相同的索引名，它们都将通过这个检查（我只假设这是一个rest调用——这甚至是最糟糕的）
2. elasticConfig == null || elasticConfig.equals(BatchConstants.EMPTY_STRING)在第一次运行时，T1和T2都将通过此测试，并且都将从文件中加载配置-可能不会产生影响，但仍然是一种赛车状态
3. if (elasticConfig != null && !elasticConfig.equals(""))与2+相同的场景，如果由于内存模型，如果elasticConfig不是volatile，则在loadElasticConfigFromFile将其完全初始化之前，可以将其读取为“非空”

2和3可以通过双重检查锁定来修复（就像你在getInstance()中所做的那样），或者我更愿意将其移动到实例初始化块——我认为构造函数是最好的

至于更好地理解这种现象，你可以查看why a==1 && a==2 can evaluate to true

然而，更大的问题是，由于调用和响应之间的延迟，您得到了一个很宽的窗口，其中两个线程可以查询相同的索引并获得完全相同的响应——该索引不存在，并尝试创建一个索引