Is this a thread-safe implementation of Singleton?

Question

I read the Wikipeida's Singleton introduction, and wrote this Singleton class. Could there be any thread safety issue?

 public class Engine {

    private static volatile Engine instance = null;
    private final Pipeline pipeline;

    private Engine() {
        pipeline = createEngine(SystemProperties.LANGUAGE);

    }

    public static Engine getInstance() {
        if (instance == null) {
            synchronized (Engine.class) {
                if (instance == null) {
                    instance = new Engine();
                }
            }
        }
        return instance;
    }

    private Pipeline createEngine(Constants.Language langauge) {
        Pipeline pipeline;
        if (langauge == Constants.Language.Chinese) {
            Properties props = IOUtils.readProperties("properties/chinese.properties");
            pipeline = new Pipeline(props);
        } else if (langauge == Constants.Language.English) {
            Properties props = IOUtils.readProperties("properties/english.properties");
            pipeline = new Pipeline(props);
        } else {
            throw new IllegalStateException("Unknow langauge not supported!");
        }
        return pipeline;
    }
}

Client code to use it:

    private List<String> segment(String str) {

            Engine ENGINE = Engine.getInstance();
            List<String> tokens = new ArrayList<>();
             ...
            try {
                ENGINE.annotate(tokens);
            }catch(Exception e){
                log.warn("can't annotate this sentence:" + str);
            }
            ...

            return tokens;
        }

Looks to be thread-safe, according to the (somewhat outdated) double checked locking pattern. — Kayaman, Nov 14 '17 at 06:06
[A `enum` can make a better implementation](https://dzone.com/articles/java-singletons-using-enum) — MadProgrammer, Nov 14 '17 at 06:09
I am having a very subtle thread safety issue, and the error message shows it's in the client code line "ENGINE.annotate(tokens);", so I suspect it's due to the Singleton implementation. — user697911, Nov 14 '17 at 06:10
@MadProgrammer, what about my client code's way to use it? Is it safe in that situation? — user697911, Nov 14 '17 at 06:10
BTW, http://www.yegor256.com/2016/06/27/singletons-must-die.html — eugene-nikolaev, Nov 14 '17 at 06:19
Looks like it is safe to get the singleton instance. But not sure about the other methods e.g. `Engine.annotate(..)` you used in Client code. From client that depends on the situation will raise! — Zico, Nov 14 '17 at 06:29
You mean the annotate must also be thread safe? If it's from an external library and how to make sure "ENGINE.annotate()" could be safely called in my client code? Isn't ENGINE a local variable so 'annotate()' doesn't have to be safe? — user697911, Nov 14 '17 at 06:33
if ENGINE.annotate sets some state variables - then local variable wouldn't help - and won't be thread safe. as the local variable ENGINE just holds same reference of global singleton instance of `getInstance()` — enator, Nov 14 '17 at 07:03
@enator, but my global singleton instance of getInstance() is a thread safe implementation as shown above. Right? So why does 'annotate' could be unsafe again? — user697911, Nov 14 '17 at 15:55
It's threadsafe. Better alternatives are listed in : https://stackoverflow.com/questions/7855700/why-is-volatile-used-in-this-example-of-double-checked-locking/36099644#36099644 — Ravindra babu, Nov 14 '17 at 17:14

nairavs · Answer 1 · 2017-11-14T13:59:12.030

YES. Globally : The easier way to create a thread-safe singleton class is to make the global access method synchronized Only one thread can execute this method at a time. It reduces the performance because of cost associated with the synchronized method . To avoid this extra overhead every time, double checked locking principle is used. In this approach, the synchronized block is used inside the if condition(how you do it) with an additional check to ensure that only one instance of singleton class is created. Also please note that the singleton can be destroyed via reflection. To avoid this problem Joshua Bloch suggests the use of Enum to implement Singleton design pattern. As Java ensures that any enum value is instantiated only once in a Java program .

public enum EnumSingleton {

INSTANCE;

public static void doSomething(){
    //do something
}

}

Just suggestion to understand fully the singleton pattern with all it will be better to read it in GO4 book or o'reilly headfirst design patterns also Joshua Bloch Effective Java 2nd edition . There is a chapter about singleton .

score 0 · Answer 2 · answered Nov 14 '17 at 13:32

It is thread-safe in Java 5.0+ as described in the Wikipedia article.

It is important to know, that the volatile keyword actually makes it thread-safe. Java allows other threads to use partially constructed classes, and threads work with local copies of other thread's variables, which - in combination - can result in a situation where Thread B keeps working with a not yet fully initialized class from Thread A. The volatile keyword guarantees that this cannot happen, at a slightly increased performance cost.

Aside from that this way of lazy initialization is excessively complicated when compared to the simple (and completely thread-safe) implementation below, that relies on the guarantees of class loading:

public enum Singleton {
  INSTANCE;

  private Singleton() {
    // initialization of Singleton
  }
}

Is this a thread-safe implementation of Singleton?

2 Answers2