Фабрика одноэлементных объектов: является ли этот код потокобезопасным? - PullRequest
Новая
       37

Фабрика одноэлементных объектов: является ли этот код потокобезопасным?

6 голосов
/ 30 ноября 2011

У меня есть общий интерфейс для ряда одноэлементных реализаций.Интерфейс определяет метод инициализации, который может генерировать проверенное исключение.

Мне нужна фабрика, которая будет возвращать кэшированные одноэлементные реализации по требованию, и интересно, является ли следующий подход поточно-ориентированным?

UPDATE1: Пожалуйста, не предлагайте какие-либо третьи частично библиотеки, так как для этого потребуется получить юридическое разрешение из-за возможных проблем с лицензированием: -)

ОБНОВЛЕНИЕ2: этот код, вероятно, будет использоватьсяв среде EJB, поэтому предпочтительнее не создавать дополнительные потоки или использовать подобные вещи. 

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;
    }
}

Я не совсем уверен в строках (1) и (2).Я знаю, что указанный объект объявлен как изменчивое поле в AtomicReference, и, следовательно, изменения, сделанные в строке (1), должны сразу же стать видимыми в строке (2) - но все же есть некоторые сомнения ...

Кромеэто - я думаю, что использование ConcurrentHashMap решает проблему атомарности помещения нового ключа в кеш.

Ребята, видите ли вы какие-либо проблемы с этим подходом?Спасибо!

PS: Я знаю об идиоме класса статического держателя - и я не использую его из-за ExceptionInInitializerError (в который включается любое исключение, возникающее при создании экземпляра singleton) и последующие NoClassDefFoundError, которые я не хочу поймать.Вместо этого я хотел бы использовать преимущество выделенного проверенного исключения, перехватывая его и обрабатывая его изящно, а не анализируя трассировку стека EIIR или NCDFE.

Ответы [ 6 ]

3 голосов
/ 30 ноября 2011

Вы проделали большую работу, чтобы избежать синхронизации, и я предполагаю, что причина этого заключается в проблемах производительности. Вы проверяли, действительно ли это улучшает производительность по сравнению с синхронизированным решением?

Причина, по которой я спрашиваю, состоит в том, что классы Concurrent имеют тенденцию быть медленнее, чем неконкурентные классы, не говоря уже о дополнительном уровне перенаправления с атомарной ссылкой. В зависимости от вашего конфликта потоков наивное синхронизированное решение может на самом деле быть быстрее (и его легче проверить на правильность).

Кроме того, я думаю, что вы можете закончить бесконечным циклом, когда выдается исключение SingletonException во время вызова instance.init (). Причина в том, что параллельный поток, ожидающий в readEventually, никогда не будет в конечном итоге находить свой экземпляр (так как исключение было сгенерировано, когда другой поток инициализировал экземпляр). Возможно, это правильное поведение для вашего случая, или вы хотите установить какое-то специальное значение для экземпляра, чтобы вызвать исключение, которое будет выдано ожидающему потоку.

3 голосов
/ 30 ноября 2011

Наличие всех этих параллельных / атомарных вещей вызовет больше проблем с блокировкой, чем просто установка 

synchronized(clazz){}

блоки вокруг геттера. Атомные ссылки предназначены для ссылок, которые ОБНОВЛЕНЫ, и вы не хотите столкновения. Здесь у вас есть один писатель, так что вам все равно.

Вы можете оптимизировать его дальше, имея хэш-карту, и только в случае пропуска используйте синхронизированный блок: 

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;
        }
    }
}
0 голосов
/ 01 декабря 2011

гугл "Memoizer".в основном вместо AtomicReference используйте Future.

0 голосов
/ 30 ноября 2011

Похоже, что это будет работать, хотя я мог бы рассмотреть какой-то сон, если даже наносекунду или что-то в этом роде при тестировании на заданную ссылку.Цикл спин-теста будет очень дорогим.

Кроме того, я бы рассмотрел возможность улучшения кода путем передачи AtomicReference в readEventually(), чтобы вы могли избежать условия гонки containsKey() и putIfAbsent().,Таким образом, код будет: 

AtomicReference<T> ref = (AtomicReference<T>) CACHE.get(key);
if (ref != null) {
    return readEventually(ref);
}

AtomicReference<T> newRef = new AtomicReference<T>(null);
AtomicReference<T> oldRef = CACHE.putIfAbsent(key, newRef);
if (oldRef != null) {
    return readEventually(oldRef);
}
...
0 голосов
/ 30 ноября 2011

Код обычно не является потокобезопасным, поскольку между проверкой CACHE.containsKey(key) и вызовом CACHE.putIfAbsent(key, ref) существует разрыв. Два потока могут одновременно вызывать метод (особенно в многоядерных / процессорных системах), и оба выполняют проверку containsKey(), затем оба пытаются выполнить операции размещения и создания.

Я бы защитил это выполнение метода getSingletonInstnace(), используя либо блокировку, либо синхронизацию на каком-либо мониторе.

0 голосов
/ 30 ноября 2011

Рассмотрите возможность использования Guava's CacheBuilder.Например: 

private static Cache<Class<? extends Singleton>, Singleton> singletons = CacheBuilder.newBuilder()
   .build(
       new CacheLoader<Class<? extends Singleton>, Singleton>() {
         public Singleton load(Class<? extends Singleton> key) throws SingletonException {
           try {
             Singleton singleton = key.newInstance();
             singleton.init();
             return singleton;
           }
           catch (SingletonException se) {
             throw se;
           }
           catch (Exception e) {
             throw new SingletonException(e);
           }
         }
       });

public static <T extends Singleton> T getSingletonInstance(Class<T> clazz) {
  return (T)singletons.get(clazz);
}

Примечание: этот пример не протестирован и не скомпилирован.

Базовая реализация Cache в Guava будет обрабатывать всю логику кэширования и параллелизма за вас.

...