java - 单例类的公共(public)方法应该同步吗？

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...

对我来说，单例类是线程安全的，但我不确定如果多个线程开始执行单例类的相同方法会发生什么。这有副作用吗？

注意:我知道上面的单例类不是反射和序列化安全的。

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

如果您关心并发写入单个索引，那么就不要这样做 - 这是一个 ElasticSearch 任务来正确处理并发写入(相信我 ES 会顺利处理)

什么不是线程安全的(指出了 3 个地方)？具有并发 T1 和 T2:

1. checkIfElasticSearchIndexExists(index)如果 T1 和 T2 使用相同的索引名称，则两者都会通过此检查(我只假设这是一个休息调用 - 这是更糟糕的)
2. elasticConfig == null || elasticConfig.equals(BatchConstants.EMPTY_STRING)好吧，在第一个 runt T1 和 T2 都将通过此测试，并且都将从文件加载配置 - 可能不会产生影响，但它仍然是一个竞赛条件
3. if (elasticConfig != null && !elasticConfig.equals(""))与2+相同的场景，如果由于内存模型的原因，如果elasticConfig不是volatile它可以在 loadElasticConfigFromFile 之前读作“not null”将完全初始化它。

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

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

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