Singleton using AtomicReference

Is it correct implementation of lazy-initializing singleton using AtomicReference? If no - what are the possible issues?

import java.io.ObjectStreamException;
import java.io.Serializable;
import java.util.concurrent.atomic.AtomicReference;

public class Singleton implements Serializable {

    private static final Singleton _instance = new Singleton();

    private static AtomicReference<Singleton> instance = new AtomicReference<Singleton>();

    private Singleton() {
    }

    public static Singleton getInstance() {
        if (instance.compareAndSet(null, _instance)) {
            synchronized (_instance) {
                _instance.init();
                instance.set(_instance);
    开发者_如何学JAVA        }
        }
        return instance.get();
    }

    private void init() {
        // do initialization
    }

    private Object readResolve() throws ObjectStreamException {
        return getInstance();
    }

}

No, this is bad:

public static Singleton getInstance() {
    // new "singleton" for every method call
    Singleton s = new Singleton();
                   ^^^^^^^^^^^^^^
    if (instance.compareAndSet(null, s)) {
        synchronized (s) {
            s.init();
        }
    }
    return instance.get();
}

Using an AtomicReference is a nice idea, but it won't work because Java doesn't have lazy evaluation.

The classic post 1.5 singleton methods are:

Eager Singleton:

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

Lazy Singleton with inner holder class:

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

Enum Singleton:

public enum Singleton{
    INSTANCE;
}

You should probably stick with one of these

You've got a race condition, in that you may return an instance of the Singleton before init is called on it. You could wrap singleton if you wanted a once-only init. However, we know how to implement singletons in a simple, efficiently manner and mutable singletons are pure evil..

You can prevent serialization scenario to make it singleton forcefully .

all your constructor should be private.

I don't see why you would need to use AtomicReference for a singleton: AtomicReference allows you to atomically change ther reference to the object -- and in the case of a singleton there should be one instance only and no one should be able to change that ever again for the execution of your app. Also your code is not synchronized, so concurrent requests will end up creating multiple instances of the Singleton class (as pointed out by @Sean Patrick Floyd for instance).

public static Singleton getInstance() {
    Singleton s=instance.get();
    if(s!=null)synchronized(s){return s;}//already initialised ->return it
    Singleton s = new Singleton();
    synchronized(s){
         if(instance.compareAndSet(null, s))//try to set
              s.init();
    }
    return instance.get();//use the one that is there after CaS (guaranteed not null)
}

The updated code adds deserialisation with readResolve.

Two obvious problems here.

• Back references can read the original object before readResolve is called.

• Even though the class has only a private constructor, it is not final. final is really important. A hand-crafted (or created with a jig Singleton implementation) octet sequence could deserialise into a subclass. (The no-args constructor of the most derived non-serialisable base class (which must be accessible to the most base serialisable class) is called by the deserialisation machinery.) An inaccessible readResolve is not called for subclasses.