16

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
  • 7
    My biggest problem is with the pattern itself. From Wikipedia: Some consider it an anti-pattern, judging that it is overused, introduces unnecessary limitations in situations where a sole instance of a class is not actually required, and introduces global state into an application. Consider using dependency injection instead. Sep 3, 2010 at 11:51
  • 3
    I would highly suggest avoiding something like this. Use a DI framework instead.
    – matt b
    Sep 3, 2010 at 13:07
  • possible duplicate of singleton pattern in java- lazy Intialization Sep 4, 2010 at 2:56

6 Answers 6

56

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?

12
  • thank you all guys for the answers. I think i totally misconcepted what i wanted to achieve. Sep 3, 2010 at 13:18
  • You all helped me to target the real problem, which i posted at stackoverflow.com/questions/3636244/… Sep 3, 2010 at 13:37
  • In class sample final static is missing. I've corrected it. I want to add a link to this post from other question. Sep 8, 2010 at 16:37
  • 3
    @savinos: because that makes it automatically threadsafe and serializable while preserving the singleton property. Especially the latter would take a lot of work to get right manually. Oct 6, 2011 at 15:53
  • Maybe it's a stupid question, but what appends if the constructor throws an exception ?
    – Pith
    Jul 24, 2013 at 19:20
5

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.

3

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.

3

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.

4
  • 1
    I was under the impression that they had fixed in in java 1.5
    – mR_fr0g
    Sep 3, 2010 at 12:25
  • @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
    Sep 3, 2010 at 13:07
  • 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. Sep 3, 2010 at 13:13
  • @matt b - No, this works and is perfectly safe with a volatile ref. See cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#dcl Sep 3, 2010 at 13:16
0

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.

2
  • 1
    Nice idea, but you have to make the constuctor public in order to get it run. Sep 3, 2010 at 13:35
  • 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
    Sep 3, 2010 at 15:13
0
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.

1
  • overkill as for me. not the simplest way.
    – ses
    Feb 20, 2013 at 19:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.