Is this Singleton class in Java acceptable?

Question

I used the concepts from a published tutorial on Singleton creation but would like some feedback. Is there a preferred method?

public class MyDoubleLockSingleton {

    private volatile static Object _mySingleton;

//private constructor
    private MyDoubleLockSingleton() {
        if (_mySingleton == null) {
            MyDoubleLockSingleton.getSingleton();
        }
    }

//  double lock singleton approach
    public static Object getSingleton() {
        synchronized (MyDoubleLockSingleton.class) {
            if (_mySingleton == null) {
            _mySingleton = new Object();
        }
        return (Object)_mySingleton;
        }
    }
}

Answer 1

It would look more like this:

public class MyDoubleLockSingleton {

    private static final Object _lock = new Object(); // synchronize on this instead of MyDoubleLockSingleton.class
    private volatile static MyDoubleLockSingleton _mySingleton;

    //private constructor
    private MyDoubleLockSingleton() {
        // perform expensive initialization here.
    }

    //  double lock singleton approach
    public static MyDoubleLockSingleton getSingleton() {
        if (_mySingleton == null) {
            synchronized (_lock) {
                if (_mySingleton == null) {
                    _mySingleton = new Object();
                }
            }
        }
        return _mySingleton;
    }
}

• The private constructor is where you perform your expensive initialization.
• You are better off synchronizing on a private member.
• And you probably wanted to add the null check outside the synchronized block for performance. Otherwise, there isn't much point to this pattern.

Now, if your private constructor's code is not that expensive, then there really isn't any point in trying to perform lazy initialization. In that case, keep it very simple like this:

public class MyDoubleLockSingleton {

    private static final MyDoubleLockSingleton _mySingleton = new MyDoubleLockSingleton();

    //private constructor
    private MyDoubleLockSingleton() {
        // perform initialization here.
    }

    //  double lock singleton approach
    public static Object getSingleton() {
        return _mySingleton;
    }
}

Answer 2

Your biggest flaw is synchronizing on every getSingleton() call.

The idea of "double checked" locking is to first perform an unsynchronized check. This is for cases where the singleton is already initialized. If the singleton already exists, there is no reason to synchronize

If the singleton is null when you perform the unsynchronized check, you THEN synchronize:

public Singleton getSingleton() {
    if(singleton == null) {
        synchronized(lock) {

        }
    }
}

Now we need to make sure no other threads may have initialized the singleton from the time we leave the null check to the time we enter the synchronized block. If the singleton has been created in that time, we don't want to create a new singleton. That's why we perform a second null-check:

public Singleton getSingleton() {
    if(singleton == null) {
        synchronized(lock) {
            if(singleton == null) {
                //create
            }
        }
    }
}

An easier way to avoid this would to use the Initialize-On-Demand idiom:

class Singleton {
    private static final Singleton SINGLETON = new Singleton();

    public static Singleton getSingleton() {
        return SINGLETON;
    }
}

The idea is to let the mechanism that handles static initialization (which is already synchronized) to handle the creation for you.

An even easier alternative would be an enum:

public enum Singleton {
    INSTANCE;
}

To reduce verbosity, I usually use GET instead of INSTANCE. That's assuming you aren't using static imports, which you should be using a more suitable name if that were the case.

Answer 3

Besides synchronizing like Vince mentioned, and returning object in get singleton it is good. Usually in singleton method is called instance or getInstance. Remove body of constructor. Singleton pattern can be used however try to avoid singletons if you can, otherwise initialize it as fast as possible, when you application has only one thread. General rule - the less singletons you have the better. See this article http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html

From article see difference

class SomeClass {
  private Resource resource = null;
  public Resource getResource() {
    if (resource == null) {
      synchronized {
        if (resource == null) 
          resource = new Resource();
      }
    }
    return resource;
  }
}

class A {
  static private A a = null;
  public Resource instance() {
    if ( a == null ) {
      synchronized {
        if (a == null ) 
           a= new A();
      }
    }
    return a;
  }
}

Answer 4

Actually there is a preferred method, also the easiest one to implement. Enums.

• They are guaranteed to to have only 1 instance across JVM
• They are serializable-safe
• They are thread-safe

Answer 5

You don't mention your version of java, but this is what I've used in the past. I had some code that was supposed to be something like a singleton that was using single check locking and under a load was garbling reports as the report threads collided with each other. I used this solution called: The Intialize-On-Demand-Holder Class Idiom:

public class Singleton {
        // Private constructor. Prevents instantiation from other classes.
        private Singleton() { }

        /**
         * Initializes singleton.
         *
         * {@link SingletonHolder} is loaded on the first execution of {@link Singleton#getInstance()} or the first access to
         * {@link SingletonHolder#INSTANCE}, not before.
         */
        private static class SingletonHolder {
                private static final Singleton INSTANCE = new Singleton();
        }

        public static Singleton getInstance() {
                return SingletonHolder.INSTANCE;
        }
}

and it effectively creates a singleton that is thread safe.