Can the following Singleton be unsafe in multithreaded environment

Question

I want to be sure that my Singleton instance is available safely and with minimum synchronization but I have doubt about the first if clause outside the synchronized block. Is it possible for the INSTANCE to have a not-null value when it isn't completely constructed? If so how can I solve the issue.

I think that including the whole get() block will reduce the efficiency because there will be so many configuration variables that must be read thousands of times per second from different part of program via this get() method.

public class ConfsDBLoader {

    private static ConfsDBLoader INSTANCE = null;
    private static final Object lock = new Object();

    private ConfsDBLoader() { //Codes loading the db objects
    }

    public static ConfsDBLoader get(){
        if(INSTANCE != null){
            return INSTANCE;
        } else {
            synchronized(lock){
                if(INSTANCE == null){
                    INSTANCE = new ConfsDBLoader();
                }
                return INSTANCE;
            }
        }
    }

}

NOTE: I cant use static initialization because my hibernate sessionFactory is initialized statically and I want to have complex static structures that need each other. In fact I already have it and I'm not interested to make it more and more complex and investigate where these these static attributes try to use each other.

I assume you know there are simpler alternatives which have been around for about ten years. Can you let us know what you considered and why they were not appropriate? — Peter Lawrey, Sep 01 '14 at 16:46
You are using a very old and very well-known anti-pattern. It's not thread safe. A second caller can find a non-null INSTANCE while the first caller is still initialising it. — gnasher729, Sep 01 '14 at 16:47
You mean that I should nest two synchronized? But it is not efficient! — Johnny, Sep 01 '14 at 16:48
I am suggesting you use a SingletonHolder or an `enum`, neither use synchronization at all. Can you say why you don't use those? — Peter Lawrey, Sep 01 '14 at 16:49
possible duplicate of [Java double checked locking](http://stackoverflow.com/questions/1625118/java-double-checked-locking) — Holger, Sep 01 '14 at 16:51
But SingletonHolder and enum are both eager! I'm trying to let this part of code be run on the first call in the non static body of the program. — Johnny, Sep 01 '14 at 16:53
@Johnny: None of them is eager. You are trying to “optimize” a problem that simply doesn’t exist. — Holger, Sep 01 '14 at 16:55
@Johnny Classes are lazily loaded. What do you mean by eager? — Peter Lawrey, Sep 01 '14 at 17:01
possible duplicate of [What is an efficient way to implement a singleton pattern in Java?](http://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java) — fredoverflow, Sep 01 '14 at 18:32

score 2 · Answer 1 · answered Sep 01 '14 at 16:47

2

No. There is not enough synchronization to make sure that you are seeing the correct value on INSTANCE. You may see a non-null, but corrupt instance if your ConfsDBLoader because it may not be properly constructed by the time another thread calls getInstance().

You have 3 choices:

Eager initialize and make final
Synchronize whole method
Make INSTANCE volatile

answered Sep 01 '14 at 16:47

I reject the first two ones but will it work only with volatile keyword?! That's what I doubt. And will it be enough efficient? – Johnny Sep 01 '14 at 16:50
@Johnny Define what you mean by "efficient enough"? – Peter Lawrey Sep 01 '14 at 16:52
@Johnny synchronizing the whole method has some overhead, but how often are you going to call the get method? At some point, you will cache a local instance in your class, right? If I had to choose between the two, and loading the object eagerly is not an issue, I'd just choose option 1. – coffeeaddict Sep 01 '14 at 18:03
@Peter Lawrey ... are the sync between threads atomic when INSTANCE is being set? I've never really tried to find ways to break volatile, but that was my first thought since I've only really known it as a way to keep the value in sync in general, but not from context of atomicity. Would be good to know! – coffeeaddict Sep 01 '14 at 18:05
@coffeeaddict `volatile` has the memory barriers you need. – Peter Lawrey Sep 01 '14 at 18:37

Can the following Singleton be unsafe in multithreaded environment

1 Answers1