Implement the singleton pattern with a twist

Viewed 916

This is a job interview question.

Implement the singleton pattern with a twist. First, instead of storing one instance, store two instances. And in every even call of getInstance(), return the first instance and in every odd call of getInstance(), return the second instance.

My implementation is as follows:

public final class Singleton implements Cloneable, Serializable {
    private static final long serialVersionUID = 42L;
    private static Singleton evenInstance;
    private static Singleton oddInstance;
    private static AtomicInteger counter = new AtomicInteger(1);

    private Singleton() {
        // Safeguard against reflection
        if (evenInstance != null || oddInstance != null) {
            throw new RuntimeException("Use getInstance() instead");
        }
    }

    public static Singleton getInstance() {
        boolean even = counter.getAndIncrement() % 2 == 0;
        // Make thread safe
        if (even && evenInstance == null) {
            synchronized (Singleton.class) {
                if (evenInstance == null) {
                    evenInstance = new Singleton();
                }
            }
        } else if (!even && oddInstance == null) {
            synchronized (Singleton.class) {
                if (oddInstance == null) {
                    oddInstance = new Singleton();
                }
            }
        }

        return even ? evenInstance : oddInstance;
    }

    // Make singleton from deserializaion
    protected Singleton readResolve() {
        return getInstance();
    }

    @Override
    protected Object clone() throws CloneNotSupportedException {
        throw new CloneNotSupportedException("Use getInstance() instead");
    }
}

Do you see a problem? The first call may enter getInstance and the thread get preempted. The second call may then enter getInstance but will get the oddInstance instead of the evenInstance.

Obviously, this can be prevented by making getInstance synchronized, but it's unnecessary. The synchronization is only required twice in the lifecycle of the singleton, not for every single getInstance call.

Ideas?

3 Answers

Most importantly, the evenInstance and oddInstance variables need to be declared volatile. See the famous "Double-Checked Locking is Broken" declaration: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Also, you should really use different objects in the synchronization blocks for the even and odd instances so they can be constructed simultaneously.

Finally, the check in the Singleton constructor is broken and will throw an exception in the second call to getInstance()

Other than that it's fine, but it's better if you don't do the concurrency work yourself:

public final class Singleton implements Cloneable, Serializable {
    private static AtomicInteger counter = new AtomicInteger(1);


    public static Singleton getInstance() {
        if (counter.getAndIncrement() % 2 == 0) {
            return EvenHelper.instance;
        } else {
            return OddHelper.instance;
        }
    }

    private static class EvenHelper {
        //not initialized until the class is used in getInstance()
        static Singleton instance = new Singleton();
    }

    private static class OddHelper {
        //not initialized until the class is used in getInstance()
        static Singleton instance = new Singleton();
    } 
}

You don't say the singleton must be lazily initialized, so I'll assume not...

You could be over-thinking it. Try this:

public final class Singleton implements Cloneable, Serializable {
    private static Singleton[] instances = new Singleton[]{new Singleton(), new Singleton()};
    private static AtomicInteger counter = new AtomicInteger();

    private Singleton() {} // further protection not necessary

    public static Singleton getInstance() {
        return instances[counter.getAndIncrement() % 2];
    }

    // Make singleton from deserializaion
    protected Singleton readResolve() {
        return getInstance();
    }

    @Override
    protected Object clone() throws CloneNotSupportedException {
        throw new CloneNotSupportedException("Use getInstance() instead");
    }
}

If you're worried about reflection attacks, just use an enum, which is bullet-proof, something like:

public final class Singleton implements Cloneable, Serializable {
    private static AtomicInteger counter = new AtomicInteger();
    private enum SingletonInstance implements Cloneable, Serializable {
        ODD, EVEN;
        private Singleton instance = new Singleton();
    }

    private Singleton() {} // further protection not necessary

    public static Singleton getInstance() {
        return SingletonInstance.values()[counter.getAndIncrement() % 2].instance;
    }

    // Make singleton from deserializaion
    protected Singleton readResolve() {
        return getInstance();
    }
}

Do you see a problem? The first call may enter getInstance and the thread get preempted. The second call may then enter getInstance but will get the oddInstance instead of the evenInstance.

Obviously, this can be prevented by making getInstance synchronized, but it's unnecessary. The synchronization is only required twice in the lifecycle of the singleton, not for every single getInstance call.

If you really want to "fix" this "problem", your only option is to synchronize getInstance. But how would one really see this problem? What if the first thread is preempted right after getInstance?

In multithreading, the absolute order of events is not completely deterministic. So you always have the risk that actions seem to be out-of-order.


btw: The against the "reflection attack" has a serious flaw! It prevents the construction of evenInstance! I guess you should change || to &&. But that still doesn't give you any guarantees, because the "reflection attack" could be between the first and second call. You have to preconstruct both instances at class loading time to be 99% sure.

And if you're worried about it, you should definitely implement neither Cloneable nor Serializable!

Related