Should singleton class public methods be synchronized?

Question

I have a singleton wrapper class abstracting elasticsearch APIs for my application.

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

}

I have multiple threads that use ElasticSearchClient as mentioned below

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

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

Thread3...

As per me the Singleton class is thread safe but I am not sure what will happen if more than one thread starts executing the same method of a singleton class. Does this have any side-effect?

Note: I am aware of that the singleton class above is not reflection and serialization safe.

What might happen if multiple threads called `createElasticIndex("sameIndex")` concurrently? Well, quite a lot of things since there is no synchronization but one thing could be that multiple put requests are generated. — Thomas, Aug 07 '18 at 15:27
Multiple put requests are expected. Because they are writing to a different indexes in the elastic search. My whole concern is will one thread create any sort of data corruption for data of some other thread? — ThinkGeek, Aug 07 '18 at 15:29

Antoniossss · Accepted Answer · 2018-08-07T15:43:56.613

In your particular implementation

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

There are 3 points that could cause racing conditions.

So per se

Should singleton class public methods be synchronized?

There is no such rule - if those would be thread safe, then no additional synchronization is needed. In your case, those are not thread safe thus you must make them threas safe eg. making

public synchronized boolean createElasticIndex

If you are concenr eg about writing concurrently to single index, then don't - it is an ElasticSearch task to handle concurent writes properly (and trust me ES will handle that smoothly)

Whats not thread safe (pointed out 3 places)? Having concurrent T1 and T2:

checkIfElasticSearchIndexExists(index) If T1 and T2 will use the same index name, both will pass this check (I only assume it is a rest call - that is even worst)
elasticConfig == null || elasticConfig.equals(BatchConstants.EMPTY_STRING) Well on first runt T1 and T2 will both pass this test and both will load config from file - probably wont have impact, but still it is a racing condition
if (elasticConfig != null && !elasticConfig.equals("")) The same scenario as 2 + if due to memory model, if elasticConfig is not volatile it can be read as "not null" before loadElasticConfigFromFile will inilatize it fully.

2 and 3 can be fixed with either double-check-locking (like you do in getInstance() or I would rather move it to instance initialization block - constructor would be the best for that I think.

As for better undestanding of that phenomenon you can check why a==1 && a==2 can evaluate to true

1 however is bigger problem due to delay between call and response you got a wide window where 2 threads can query for the same index and get exact same response - that index does not exist and try to create one.

Thanks for the insights. This makes sense. – ThinkGeek Aug 07 '18 at 15:42 — ThinkGeek, Aug 07 '18 at 15:42

Should singleton class public methods be synchronized?

1 Answers1