views:

167

answers:

6

Hello guys,

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 instanceHolder.get();
    }

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

Thanks

+3  A: 

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.

Joel
I was under the impression that they had fixed in in java 1.5
mR_fr0g
@mR_fr0g, no. A part of the problem with this anti-pattern is that it attempts to read the reference without locking. Proper synchronization requires locks on *all* reads *and* writes in order for threads to be guaranteed to see updates from other threads
matt b
You have rather old information. Java 1.5 came out in late 2004, and this has been fixed since then (so long as the field is marked `volatile`) In fact, Java 1.5 is so old that Sun stopped supporting it last November.
Daniel Martin
@matt b - No, this works and is perfectly safe with a `volatile` ref. See http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#dcl
Daniel Martin
+1  A: 

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.

Vineet Reynolds
A: 

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.

Alexander Pogrebnyak
+9  A: 

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?

Michael Borgwardt
thank you all guys for the answers. I think i totally misconcepted what i wanted to achieve.
iimuhin
You all helped me to target the real problem, which i posted at http://stackoverflow.com/questions/3636244/thread-safe-cache-of-one-object-in-java
iimuhin
In `class` sample `final static` is missing. I've corrected it. I want to add a link to this post from other question.
Alexander Pogrebnyak
A: 

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.

mR_fr0g
Nice idea, but you have to make the constuctor public in order to get it run.
iimuhin
Yeah Good point. You could use reflection to access the constructor and make it accessable. It would be a bit hacky though, so if you do do it make sure you document the hell out of it.
mR_fr0g
A: 
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.

irreputable