Is my code a 'safe' singleton?

Question

I was wondering if my code will produce a true singleton. I am creating an Android app, and all activities should access my API through one instance of the SyncApi class.

public class Api {

    private static SyncApi api = null;

    static {
        synchronized (api) {
            if (api == null) {
                api = new ApiFActory().getSyncApi();
            }
        }
    }

    public static SyncApi getInstance() {
        return api;
    }
}

Use of the class would look like:

SyncApi api = Api.getInstance();
api.logOn("jjnguy", "pa55w0rd");

Answer 1

In Java there’s an established idiom for creating a thread-safe singleton, due to Bill Pugh:

public class Api {
    private Api() { }

    private static class SingletonHolder { 
        public static final SyncApi INSTANCE = new ApiFactory().getSyncApi();
    }

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

This implementation is both lazy and completely thread-safe, without the need to explicit synchronization and checking. This has the advantage that it eliminates the possibility of subtle race conditions and redundant locking.

Answer 2

The best and simple method was gived by Joshua Bloch in 《Effective Java 2th edition》

public enum Api {
    INSTANCE;
}

you can use it like this

Api api = Api.INSTANCE;

Answer 3

If you want thread-safe and lazy, I completely agree with the answer provided by Konrad Rudolph.

If you want just thread-safe without laziness (which is also very common), then you can initialize INSTANCE in the upper class, not from inner one.

Here is a good article with many tests and examples:
http://www.javaworld.com/javaworld/jw-04-2003/jw-0425-designpatterns.html

But both approaches don't give you an absolute singleton, since different ClassLoaders will create different instances. It might look like a nearly impossible case, but in my experience I faced a bug which was cause by such a situation.

Here is an article about absolute singletons:
http://surguy.net/articles/communication-across-classloaders.xml

Answer 4

Your setting your api to null did not help you spot your NullPointerException

synchronized (api) // api == null, you should use Api.class

As to the singleton please, please, please use an enum. It is the recommended pattern since java 5. Now, if you want lazy loading :

public class Api {

    private Api() {}

    private enum SingletonHolder {
        INSTANCE;

        private static final Api singleton = new Api();

        Api getSingleton() {
            return singleton;
        }
    }

    public Api api() {
        Api.SingletonHolder.INSTANCE.getSingleton();
    }
}

Call :

 api().logOn("jjnguy", "pa55w0rd");

You may dispose of getSingleton() and access singleton directly inside Api

public class Api {

    private Api() {/*your costly init here*/}

    private enum SingletonHolder {
        INSTANCE;
        private static final Api singleton = new Api();
    }

    public Api api() {
        return Api.SingletonHolder.singleton;
    }
}

Answer 5

Well, this is definitely not a Singleton, because this code is legal: Api.api=new Api();

You should at least define api variable as final and at least private constructor to avoid instantiation.

See @Konrad Rudolph's for proposed solution.

Answer 6

I agree with all the other answers but your problem, as you state it, is not to have a single API instance but to have a single SyncApi instance.

You will have no way to prevent a user from doing:

SyncApi api = new ApiFActory().getSyncApi();

You have to make SyncApi a Singleton or encapsulate it's usage in your Api class and not give access to it outside your Api...

Answer 7

public class Singleton {
      private static Singleton instance = null;

      public static Singleton getInstance(){
         if(null == instance){
          synchronized(Singleton.class){
             //double check
               if(null == instance){
                instance = new Singleton();
              }
           }  
         }
         return instance;
       }

}

This can be used to ensure only single instance is created. Or just use Enums.