Pattern for lazy thread-safe singleton instantiation in java

the lazy thread-safe singleton instantion is kinda not easy to understand to every coder, so i wanted to create a class in our enterprise framework that would do the job.

What do you think about it? Do you see something bad about it? Is there something similar like in Apache Commons? How can i make it better?

Supplier.java

public interface Supplier<T> {
    public T get();
}

LazyThreadSafeInstantiator.java

public class LazyThreadSafeInstantiator<T> implements Supplier<T> {
    private final Supplier<T> instanceSupplier;

    private volatile T obj;

    public LazyThreadSafeInstantiator(Supplier<T> instanceSupplier) {
        this.instanceSupplier = instanceSupplier;
    }

    @Override
    // http://en.wikipedia.org/wiki/Double-checked_locking
    public T get() {
        T result = obj;  // Wikipedia: Note the usage of the local variable result which seems unnecessary. For some versions of the Java VM, it will make the code 25% faster and for others, it won't hurt.
        if (result == null) {
            synchronized(this) {
                result = obj;
                if (result == null) {
                    result = instanceSupplier.get();
                    obj = result;
                }
            }
        }
        return result;
    }
}

Example usage:

public class Singleton1 {
    private static final Supplier<Singleton1> instanceHolder =
        new LazyThreadSafeInstantiator<Singleton1>(new Supplier<Singleton1>() {
            @Override
            public Singleton1 get() {
                return new Singleton1();
            }
        });

    public Singleton1 instance() {
        return instanceHo开发者_运维知识库lder.get();
    }

    private Singleton1() {
        System.out.println("Singleton1 instantiated");
    }
}

Thanks

---

the lazy thread-safe singleton instantion is kinda not easy to understand to every coder

No, it's actually very, very easy:

public class Singleton{
    private final static Singleton instance = new Singleton();
    private Singleton(){ ... }
    public static Singleton getInstance(){ return instance; }
}

Better yet, make it an enum:

public enum Singleton{
    INSTANCE;
    private Singleton(){ ... }
}

It's threadsafe, and it's lazy (initialization happens at class loading time, and Java does not load classes until they are are first referred).

Fact is, 99% of the time you don't need lazy loading at all. And out of the remaining 1%, in 0.9% the above is perfectly lazy enough.

Have you run a profiler and determined that your app belings to the 0.01% that really needs lazy-loading-at-first-access? Didn't think so. Then why are you wasting your time concocting these Rube Goldbergesque code abominations to solve a non-existing problem?

---

For a version that is more readable (in my opinion) than the one presented in the question, one can refer to the Initialization on Demand Holder idiom, introduced by Bill Pugh. Not only is it thread-safe considering the Java 5 memory model, the singleton is also lazily initialized.

---

Looks overengineered to me.

I really don't see how having helper class helps.

First of all, it's using double-locking idiom, and it has been proved once and again broken.

Second, if you HAVE TO use singleton, why not initialize static final instance.

public class Singleton1 {
    private static final Singleton1 instanceHolder =
        new Singletong1( );

    public Singleton1 instance() {
        return instanceHolder;
    }

    private Singleton1() {
        System.out.println("Singleton1 instantiated");
    }
}

This code is thread-safe and has been proven to work.

Check Vineet Reynolds' answer for when you need to initialize singleton instance on a first get. In many cases I think that approach is an overkill as well.

---

Isn't the double checked locking pattern and use of volatile broken on JIT compilers and multi-core/processor systems due to the Java Memory Model & possibility of out of order execution?

More generally, it seems that a framework for singletons is overkill for what is essentially a pretty straightforward pattern to implement correctly.

---

I would agree with other posters and say that this does seem like overkill, but have said that i do think that this is something that a junior developer is likely to get wrong. I think that because the behaviour of the supplier that constructs the singleton (shown below) is going to be the same in nearly all cases, i would be tempted to put this as default behaviour in the LazyThreadSafeInstantiator. The use of the annonomous inner class every time you want to use a singleton is really messy.

        @Override
        public Singleton1 get() {
            return new Singleton1();
        }

This could be done by providing an overloaded constructor that takes the Class to the singleton required.

public class LazyThreadSafeInstantiator<T> implements Supplier<T> {
    private final Supplier<T> instanceSupplier;

    private Class<T> toConstruct;

    private volatile T obj;

    public LazyThreadSafeInstantiator(Supplier<T> instanceSupplier) {
        this.instanceSupplier = instanceSupplier;
    }

    public LazyThreadSafeInstantiator(Class<t> toConstruct) {
        this.toConstruct = toConstruct;
    }

    @Override
    // http://en.wikipedia.org/wiki/Double-checked_locking
    public T get() {
        T result = obj;  // Wikipedia: Note the usage of the local variable result which seems unnecessary. For some versions of the Java VM, it will make the code 25% faster and for others, it won't hurt.
        if (result == null) {
            synchronized(this) {
                result = obj;
                if (result == null) {
                    if (instanceSupplier == null) {
                      try {
                        Constructor[] c = toConstruct.getDeclaredConstructors();
                        c[0].setAccessible(true);
                        result = c[0].newInstance(new Object[] {});
                      } catch (Exception e) {
                        //handle
                      }
                      result = 
                    } else {
                      result = instanceSupplier.get();
                    }
                    obj = result;
                }
            }
        }
        return result;
    }
}

This would then be used like so.

private static final Supplier<Singleton1> instanceHolder =
    new LazyThreadSafeInstantiator<Singleton1>(Singleton1.getClass());

This is my opinion is a bit cleaner. You could alos extend this further to use constructor arguments.

---

Lazy<X> lazyX= new Lazy<X>(){
    protected X create(){
        return new X();
    }};

X x = lazyX.get();

---

abstract public class Lazy<T>
{
    abstract protected T create();

    static class FinalRef<S>
    {
        final S value;
        FinalRef(S value){ this.value =value; }
    }

    FinalRef<T> ref = null;

    public T get()
    {
        FinalRef<T> result = ref;
        if(result==null)
        {
            synchronized(this)
            {
                if(ref==null)
                    ref = new FinalRef<T>( create() );
                result = ref;
            }
        }
        return result.value;
    }
}

except maybe the first get() in a thread, all get() calls require no synchronization or volatile read. the original goal of double checked locking is achieved.