Factory of singleton objects: is this code thread-safe?

Question

I have a common interface for a number of singleton implementations. Interface defines initialization method which can throw checked exception.

I need a factory which will return cached singleton implementations on demand, and wonder if following approach is thread-safe?

UPDATE1: Please don't suggest any 3rd partly libraries, as this will require to obtain legal clearance due to possible licensing issues :-)

UPDATE2: this code will likely to be used in EJB environment, so it's preferrable not to spawn additional threads or use stuff like that.

interface Singleton
{
    void init() throws SingletonException;
}

public class SingletonFactory
{
    private static ConcurrentMap<String, AtomicReference<? extends Singleton>> CACHE =
        new ConcurrentHashMap<String, AtomicReference<? extends Singleton>>();

    public static <T extends Singleton> T getSingletonInstance(Class<T> clazz)
        throws SingletonException
    {
        String key = clazz.getName();
        if (CACHE.containsKey(key))
        {
            return readEventually(key);
        }

        AtomicReference<T> ref = new AtomicReference<T>(null);
        if (CACHE.putIfAbsent(key, ref) == null)
        {
            try
            {
                T instance = clazz.newInstance();
                instance.init();
                ref.set(instance); // ----- (1) -----
                return instance;
            }
            catch (Exception e)
            {
                throw new SingletonException(e);
            }
        }

        return readEventually(key);
    }

    @SuppressWarnings("unchecked")
    private static <T extends Singleton> T readEventually(String key)
    {
        T instance = null;
        AtomicReference<T> ref = (AtomicReference<T>) CACHE.get(key);
        do
        {
            instance = ref.get(); // ----- (2) -----
        }
        while (instance == null);
        return instance;
    }
}

I'm not entirely sure about lines (1) and (2). I know that referenced object is declared as volatile field in AtomicReference, and hence changes made at line (1) should become immediately visible at line (2) - but still have some doubts...

Other than that - I think use of ConcurrentHashMap addresses atomicity of putting new key into a cache.

Do you guys see any concerns with this approach? Thanks!

P.S.: I know about static holder class idiom - and I don't use it due to ExceptionInInitializerError (which any exception thrown during singleton instantiation is wrapped into) and subsequent NoClassDefFoundError which are not something I want to catch. Instead, I'd like to leverage the advantage of dedicated checked exception by catching it and handling it gracefully rather than parse the stack trace of EIIR or NCDFE.

Answer 1

You have gone to a lot of work to avoid synchronization, and I assume the reason for doing this is for performance concerns. Have you tested to see if this actually improves performance vs a synchronized solution?

The reason I ask is that the Concurrent classes tend to be slower than the non-concurrent ones, not to mention the additional level of redirection with the atomic reference. Depending on your thread contention, a naive synchronized solution may actually be faster (and easier to verify for correctness).

Additionally, I think that you can possibly end up with an infinite loop when a SingletonException is thrown during a call to instance.init(). The reason being that a concurrent thread waiting in readEventually will never end up finding its instance (since an exception was thrown while another thread was initializing the instance). Maybe this is the correct behaviour for your case, or maybe you want to set some special value to the instance to trigger an exception to be thrown to the waiting thread.

Answer 2

Having all of these concurrent/atomic things would cause more lock issues than just putting

synchronized(clazz){}

blocks around the getter. Atomic references are for references that are UPDATED and you don't want collision. Here you have a single writer, so you do not care about that.

You could optimize it further by having a hashmap, and only if there is a miss, use the synchronized block:

public static <T> T get(Class<T> cls){
    // No lock try
    T ref = cache.get(cls);
    if(ref != null){
        return ref;
    }
    // Miss, so use create lock
    synchronized(cls){ // singletons are double created
        synchronized(cache){ // Prevent table rebuild/transfer contentions -- RARE
            // Double check create if lock backed up
            ref = cache.get(cls);
            if(ref == null){
                ref = cls.newInstance();
                cache.put(cls,ref);
            }
            return ref;
        }
    }
}