Ещё раз (надеюсь, последний) про double-checked locking

    Статей про double-checked locking на Хабре было столько, что казалось бы ещё одна — и Хабр лопнет. Вот только по Java неплохие публикации: Реализация Singleton в JAVA, Правильный Singleton в Java, А как же всё-таки работает многопоточность? Часть II: memory ordering или вот замечательный пост от TheShade (слава web-archive!). В наши дни, наверно, каждый Java-разработчик слышал, что если используешь DCL, будь добр объявить переменную volatile. Найти сегодня в коде известных опенсорсных проектов DCL без volatile довольно трудно, но оказалось, что проблемы ещё не полностью решены. Поэтому я добавлю небольшую заметку по теме с примерами из реальных проектов.

    Иногда складывается ощущение, что программисты не включают мозги и не пытаются понять, как что работает, а просто следуют простым и понятным правилам вроде «объяви переменную volatile, используй DCL, и всё будет хорошо». К сожалению, такой подход в программировании не всегда работает.

    Особенность DCL-паттерна в том, что момент публикации объекта — это операция volatile-записи, а не выход из секции синхронизации. Поэтому именно volatile-запись должна производиться после полной инициализации объекта.

    Вот, к примеру, такой код обнаружился в проекте ICU4J — TZDBTimeZoneNames#prepareFind:
        private static volatile TextTrieMap<TZDBNameInfo> TZDB_NAMES_TRIE = null;
    
        private static void prepareFind() {
            if (TZDB_NAMES_TRIE == null) {
                synchronized(TZDBTimeZoneNames.class) {
                    if (TZDB_NAMES_TRIE == null) {
                        // loading all names into trie
                        TZDB_NAMES_TRIE = new TextTrieMap<TZDBNameInfo>(true);
                        Set<String> mzIDs = TimeZoneNamesImpl._getAvailableMetaZoneIDs();
                        for (String mzID : mzIDs) {
                            ...
                            TZDB_NAMES_TRIE.put(std, stdInf);
                            ...
                        }
                    }
                }
            }
        }
    

    Разработчик написал volatile, потому что где-то слышал, что так надо, но, видимо, не понял, зачем. По факту публикация объекта TZDB_NAMES_TRIE состоялась в момент volatile-записи: после этого вызовы prepareFind в других потоках будут сразу выходить без синхронизации. При этом после публикации производится множество дополнительных шагов по инициализации.

    Данный метод используется при поиске часового пояса, и этот поиск вполне можно сломать. В нормальных условиях new TZDBTimeZoneNames(ULocale.ENGLISH).find("GMT", 0, EnumSet.allOf(NameType.class)) должен выдавать один результат. Выполним этот код в 1000 потоков:
    Код теста
    import java.util.ArrayList;
    import java.util.EnumSet;
    import java.util.List;
    import java.util.concurrent.atomic.AtomicInteger;
    
    import com.ibm.icu.impl.TZDBTimeZoneNames;
    import com.ibm.icu.text.TimeZoneNames.NameType;
    import com.ibm.icu.util.ULocale;
    
    public class ICU4JTest {
    	public static void main(String... args) throws InterruptedException {
    		final TZDBTimeZoneNames names = new TZDBTimeZoneNames(ULocale.ENGLISH);
    		final AtomicInteger notFound = new AtomicInteger();
    		final AtomicInteger found = new AtomicInteger();
    		List<Thread> threads = new ArrayList<>();
    		
    		for(int i=0; i<1000; i++) {
    			Thread thread = new Thread() {
    				@Override
    				public void run() {
    					int resultSize = names.find("GMT", 0, EnumSet.allOf(NameType.class)).size();
    					if(resultSize == 0)
    						notFound.incrementAndGet();
    					else if(resultSize == 1)
    						found.incrementAndGet();
    					else
    						throw new AssertionError();
    				}
    			};
    			thread.start();
    			threads.add(thread);
    		}
    		for(Thread thread : threads) thread.join();
    		System.out.println("Not found: "+notFound);
    		System.out.println("Found: "+found);
    	}
    }

    Результат всегда разный, но примерно такой:
    Exception in thread "Thread-383" java.util.ConcurrentModificationException
    	at java.util.LinkedList$ListItr.checkForComodification(LinkedList.java:953)
    	at java.util.LinkedList$ListItr.next(LinkedList.java:886)
    	at com.ibm.icu.impl.TextTrieMap$Node.findMatch(TextTrieMap.java:255)
    	at com.ibm.icu.impl.TextTrieMap.find(TextTrieMap.java:100)
    	at com.ibm.icu.impl.TextTrieMap.find(TextTrieMap.java:89)
    	at com.ibm.icu.impl.TZDBTimeZoneNames.find(TZDBTimeZoneNames.java:133)
    	at a.ICU4JTest$1.run(ICU4JTest.java:23)
    Exception in thread "Thread-447" java.lang.ArrayIndexOutOfBoundsException: 1
    	at com.ibm.icu.impl.TextTrieMap$Node.matchFollowing(TextTrieMap.java:316)
    	at com.ibm.icu.impl.TextTrieMap$Node.findMatch(TextTrieMap.java:260)
    	at com.ibm.icu.impl.TextTrieMap.find(TextTrieMap.java:100)
    	at com.ibm.icu.impl.TextTrieMap.find(TextTrieMap.java:89)
    	at com.ibm.icu.impl.TZDBTimeZoneNames.find(TZDBTimeZoneNames.java:133)
    	at a.ICU4JTest$1.run(ICU4JTest.java:23)
    Not found: 430
    Found: 568
    Почти половина потоков ничего не нашла, пара потоков вообще упала с исключением. Да, в реальном приложении такое маловероятно, но если сценарий с высокой конкуррентностью авторов не интересует, тогда можно вообще обойтись без volatile и DCL.

    Вот другой пример от IntelliJ IDEA — FileEditorManagerImpl#initUI:
      private final Object myInitLock = new Object();
      private volatile JPanel myPanels;
      private EditorsSplitters mySplitters;
    
      private void initUI() {
        if (myPanels == null) {
          synchronized (myInitLock) {
            if (myPanels == null) {
              myPanels = new JPanel(new BorderLayout());
              myPanels.setOpaque(false);
              myPanels.setBorder(new MyBorder());
              mySplitters = new EditorsSplitters(this, myDockManager, true);
              myPanels.add(mySplitters, BorderLayout.CENTER);
            }
          }
        }
      }
    
      public JComponent getComponent() {
        initUI();
        return myPanels;
      }
    
      public EditorsSplitters getMainSplitters() {
        initUI();
        return mySplitters;
      }

    Тут авторы даже сделали приватный lock-объект для надёжности, но код всё равно сломан. Конечно, может быть ничего страшного не произойдёт, если начать пользоваться myPanels с неустановленным border, но тут проблема серьёзнее: DCL выполняется на одной переменной (myPanels), а инициализируется две (ещё и mySplitters), причём опять же volatile-запись происходит перед полной инициализацией. В результате getMainSplitters() при конкуррентном доступе вполне может вернуть null.

    Исправляются такие вещи очень легко: надо сохранить объект в локальную переменную, с её помощью вызвать все необходимые методы для инициализации, а уже потом записать volatile-поле.

    Ещё пара подозрительных мест:
    Tomcat DBCP2 BasicDataSource: возможно увидеть объект dataSource, у которого не установлен logWriter.
    Apache Wicket Application#getSessionStore(): возможно увидеть sessionStore с незарегистрированным listener'ом.

    Здесь вряд ли возможны реальные проблемы, но всё равно стоит писать аккуратнее.

    Я добавил небольшую эвристическую проверку в FindBugs, которая предупреждает о таких ситуациях. Однако она может сработать не всегда, поэтому лучше полагайтесь на свою голову.
    Поделиться публикацией

    Похожие публикации

    Комментарии 116

      –22
      Какой посыл у вашей статьи?
      Код отвратителен, именования переменных тоже.
      Как относится prepareFind() с TZDBTimeZoneNames? В статье ни строки про это.
      Если TZDB_NAMES_TRIE не константа, зачем в ее имени капс? Почему не sSingleton?

      В тоже самое время, в Java отлично работает вот этот код (взято с википедии):
      // Работает с новой семантикой volatile
      // Не работает в Java 1.4 и более ранних версиях из-за семантики volatile
      class Foo {
          private volatile Helper helper = null;
          public Helper getHelper() {
              if (helper == null) {
                  synchronized(this) {
                      if (helper == null)
                          helper = new Helper();
                  }
              }
              return helper;
          }
       
          // и остальные члены класса…
      }
      
        +7
        Вы, наверно, в текст не вчитывались. В статье приведён не идеальный код вида «делайте так», а реальный код (вида «так не делайте») из реальных приложений, которыми пользуются люди. Идеального DCL-кода на Хабре уже было много, пройдите по ссылкам в начале статьи.
          –12
          Так какой же в итоге посыл статьи? «не надо так делать»?
          Везде есть разработчики, которые как разработчики так себе, это нормально.

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

          К примеру, я уверен, что вызвав метод prepareFind() в 1000 разных потоков я получу одинаковый результат, т.е. DCL реализован нормально, но вот как он связан с классом TZDBTimeZoneNames я не вижу, потому само место «как не нужно делать» отсутствует в коде.

          P.S.
          Посмотрел другие ваши статьи — видимо, вы часто встречаетесь с такого рода говнокодом и этот пост стал своеобразной точкой кипения.
            –8
            Правда, я не совсем понял, зачем там вообще нужен DCL, ведь метод ничего не возвращает.
              +2
              Да, посыл статьи — не надо так делать :-) Я бы не стал писать, если б так никто не делал, так делают ведь. Если вы всегда пишете код правильно, это замечательно.

              Весь контекст по ссылке на grepcode (привожу её ещё раз). Там сразу видно, что этот метод объявлен в этом классе. Место «как не нужно делать» в приведённом коде присутствует. Судя по тому, что минусанули статью пока только вы, другие читатели всё-таки поняли, что я имел в виду.
                –4
                Мы взрослые люди, какие минусы? не вижу смысла ставить ни плюс, ни минус, если честно (и тем более гадить в карму без оснований).
                Если честно, не ожидал, что вы примете мой комментарий вот так, «в штыки».

                По ссылке весь этот класс сплошной говнокод (и я понимаю, что его писали не вы), его очень сложно даже читать, не то, чтобы вникать в суть его работы.
                Все это конечно же мое имхо, но было бы лучше (если уж решили написать статью по этому поводу) разжевать проблему, а не приводить в примеры код и отправлять на ссылку к исходникам.
                «Вот есть реализация DCL, реализована правильно, но не нужна, так как сам метод ничего не возвращает, к тому же использование вот тут… неправильное, так как вот тут… возникнет проблема такая-то.»
                  0
                  В том же втором вашем примере, идеально описана проблема, причины возникновения и способ ее решения.
                    0
                    Окей, окей, перефразирую: «судя по текущему процентному отношению плюсов и минусов за статью…» Так лучше? :-) Если минус ставили не вы, прошу прощения, совпало по времени, значит :-) Карму вашу я не трогал, честно! Никаких штыков, мир-дружба-жвачка!
                      +3
                      … а не приводить в примеры код и отправлять на ссылку к исходникам


                      Какая ссылка, какие исходники? Приведеннего кода более, чем достаточно, для понимания проблемы и ее решения. Видимо, вы вообще не поняли сути.
                        0
                        Ну так ткните меня носом в место в статье, где DCL в первом примере реализован неправильно, пожалуйста. Или хотя бы укажите на строчку кода в примере-тесте, которая ломает работоспособность кода. Также объясните, зачем нужен DCL в методе, который ничего не возвращает.

                        Я утверждаю, что в 1 примере DCL реализован верно, правда метод должен возвращать хоть что-то, иначе смысл его использования в примере теряется. Само же использование переменной, для которой DCL и нужен (зачем ее использовать напрямую, если для этого DCL и был придуман?), которую код инициализирует должно быть обернуто блоком synchronized(TZDBTimeZoneNames.class) {… use TZDB_NAMES_TRIE here ...}. В коде этого я не вижу, по ссылкам пошел, а там говнокод. Собственно все мои комментарии именно об этом, в той или иной форме.

                        Я бы очень хотел, чтобы автор или любой другой пользователь подтвердил мою правоту или опроверг, но проще ведь поставить минус и пройти дальше. Считаю, что реализация DCL в 1 примере верна, использования я не вижу в статье.
                          +3
                          правда метод должен возвращать хоть что-то, иначе смысл его использования в примере теряется

                          В коде по ссылке значение глобальной переменной TZDB_NAMES_TRIE ипользуется после вызова prepareFind. Это плохой стиль, но работает это, как синглтон.
                          Достаточно очевидно, что паттерн синглтон в данном случае заинлайнен в метод
                          (TZDBTimeZoneNames.class) {… use TZDB_NAMES_TRIE here ...}

                          Зачем заворачивать использование? Смысл как раз в том, что бы был один объект, общий для всех потоков, и к которому потоки имели бы одновременный доступ.
                          Ну так ткните меня носом в место в статье, где DCL в первом примере реализован неправильно, пожалуйста.

                          Вот цитата из статьи:
                          По факту публикация объекта TZDB_NAMES_TRIE состоялась в момент volatile-записи: после этого вызовы prepareFind в других потоках будут сразу выходить без синхронизации. При этом после публикации производится множество дополнительных шагов по инициализации.

                          То есть, после создания экземпляра TextTrieMap в одном потоке, все остальные потоки при вызове prepareFind увидят его в TZDB_NAMES_TRIE. Однако они могут не увидеть (и не обязаны) всю инициализацию TZDB_NAMES_TRIE, которая происходит после создания объекта.
                          Дальше в статье написано, что это приводит к ArrayIndexOutOfBoundsException и ConcurrentModificationException.

                          Инициализацию потоки не видят, так как она происходит после записи в volatile поле. А действия с объектом после записи в volatile не обязаны happens before чтения из той же volatile

                            +2
                            Вот это место: TZDB_NAMES_TRIE = new TextTrieMap(true);
                            Присваивание TZDB_NAMES_TRIE должно быть последним оператором внутри синхронизированного блока, то есть, после полного завершения всей последовательности инициализации, иначе другие потоки сразу видят ненулевую TZDB_NAMES_TRIE, которая еще не полностью инициализирована, и сразу начинают ее использование. С плачевными последствиями.
                              0

                              Спасибо за полезный комментарий! Уже было совсем запутался в этой статье, но вы меня спасли)

                                0
                                Да не за что! В смысле, баксов 50 не помешали бы. В)
                      +3
                      Посыл статьи, похоже, такой:
                      Исправляются такие вещи очень легко: надо сохранить объект в локальную переменную, с её помощью вызвать все необходимые методы для инициализации, а уже потом записать volatile-поле.
                      Я это уже встречал при разборе получающегося байт-кода и кода, сгенерированного JIT-компилятором: сначала волатильному полю присваивается указатель на выделенный кусок памяти, а следующим шагом вызывается конструктор. Так что, если делать без посредства локальной переменной, существует такой промежуток времени, в течение которого значение волатильного поля уже заполнено, но объект ещё не инициализирован, что и приводит к некорректной работе.
                        –2
                        Это же явная ошибка JIT-компилятора. В Java же запись/чтение ссылок на объекты всегда атомарно.
                          +1
                          Ссылка будет видна, а внутренние структуры не инициализированы или инициализированы частично. И кому от этого легче?
                            –3
                            Нет. Ссылка должна быть видна только после успешной отработки конструктора.
                              +1
                              А там инициализация данных не в конструкторе, а в цикле после него.
                                0
                                Ну, так в данном примере ошибка — никто и не спорит.
                                +1
                                Надо глянуть в jls. В любом случае, первый пример из статьи сломан, т. к. после публикации объекта (volatile write) он модифицируется (т. е. с точки зрения приложения не инициализирован). А happens-before между выходом из syncronized и входом в неё уже не произойдёт (т. к. volotile read вернёт не null).
                                  +1
                                  В том то и дело что «успешная отработка конструктора» понятие довольно зыбкое. См.
                                  shipilev.net/blog/2014/all-fields-are-final/
                                    0
                                    И как это к обсуждаемой теме относится? Там я как понял как раз подтверждается атомарность создания объекта:
                                    C tc = // initialize object + metadata
                                    tc.f = 42; // initialize final field
                                    [LoadStore|StoreStore]
                                    c = tc; // publish
                                      +1
                                      Это если в объекте есть final поле. А если final поля нет, никаких барьеров не будет.
                                        0
                                        Вот откуда это следует я не понял. Насколько я понял, инициализация перед публикацией гарантируется в любом случае, а в случае final там начинаются заморочки с тем, что final-может зависеть от другого поля и потому требуется дополнительная синхронизация по чтению после инициализации метаданных (т.е. дополнительная синхронизация внутри конструктора, грубо говоря).
                    +4
                    Интересные примеры, спасибо! А то много раз слышал, как правильно делать, но практические примеры не правильного использования (и нюансы) не видел :)
                      0
                      Делайте, как нужно, и не будет проблем )
                      +3
                      Да, примеры «как делать не надо» хорошие. Я бы даже эту фразу сделал жирной :)
                      Особенность DCL-паттерна в том, что момент публикации объекта — это операция volatile-записи, а не выход из секции синхронизации. Поэтому именно volatile-запись должна производиться после полной инициализации объекта.
                        –4
                        В Java же запись и чтение ссылок атомарно — зачем делать volatile?
                          0
                          Для того, чтобы изменение увидели другие потоки, для этого он и предназначен. Без volatile велик риск того, что новое значение застрянет в кэше процессорного ядра.
                            0
                            А как тогда, по Вашему у объектов, существующих одновременно в нескольких потоках, ссылки подсчитывются? ;) Будет же «застревать в кэше» счётчик ссылок? :)
                            Кроме того — завершение секции synchronize приведёт к записи переменной, т.к. является барьером синхронизации в Java, разве нет?
                              0
                              Кроме того — завершение секции synchronize приведёт к записи переменной, т.к. является барьером синхронизации в Java, разве нет?
                              Вы меня этим озадачили. Так-то оно так, звучит разумно. Я поисследую вопрос.
                                +4
                                Собственно я исследовал :) Если присвоение непосредственно переменной будет только один раз внутри секции синхронизации, то всё должно работать. Единственный гипотетический случай, когда будут побочные эффекты — если JIT компилятор заинлайнит этот метод внутри цикла и вынесет проверку на null за пределы цикла (ни разу не видел и даже не смог искусственно такое поведение получить — сильно не уверен, что такое возможно). Но на сколько я помню раньше я находил в стандарте ограничения на подобную оптимизацию (давно было — не помню).
                                Поэтому, как я уже ниже писал, предпочитаю саму переменную объекта не делать volatile, а использовать отдельный volatile int флаг — признак инициализированности. Ибо однажды круто напоролся на то, что при активном использовании объекта, который был volatile производительность приложения упала в сорок (!!!) раз. Из-за того, что при каждом обращении отрабатывал атомарный код ссылки и её инкремента/декремента.
                                Т.е. я обычно пишу так.
                                volatile int dclFlag; // int в Java на всех существующих платформах атомарный, а boolean - нет.
                                MTObject mtObject; // Объект, с которым идёт работа в нескольких потоках.
                                MTObject2 mtObject2; // Ещё объекты.
                                
                                void doMultithreaded() {
                                  if (dclFlag==0) { // Один флаг на несколько объектов.
                                    synchronized (syncObj) {
                                      if (dclFlag==0) {      
                                        // Инициализация.
                                        mtObject = new MTOject();
                                        mtObject2 = new MTOject2();
                                        dclFlag = 1; // На большинстве платформ есть атомарная запись единицы. Например ~0 далеко не все платформы умеют атомарно одной командой писать
                                      };
                                    };
                                   // Код работы с переменными.
                                   for (int i=0; i!=1000; i++)
                                      mtObject.doSomething(); // В случае если mtObject volatile, то это превращается в ад.
                                };
                                
                                  +1
                                  Кстати, там ад наступал даже не из-за того, что была куча лишних атомарных обращений к переменной объекта (хотя это довольно тяжёлая операция), но из-за того, что чтение volatile-переменной тоже барьер и компилятор и JIT крайне не эффективно компилировали код (была куча лишних записей и чтений переменных).
                                    0
                                    Т.е. примерно такой код:
                                    volatile String o1="123";
                                    int counter=0;
                                    void test() {
                                      for( int i=0; i!=1000; i++ ) {
                                        if( o2.length()==3 ) // Опаньки! Кроме того, что на каждой итерации будет проверка счётчика ссылок, так volatile чтение ещё и барьер оптимизации!
                                          counter++; // Вот тут тоже будет неэффективный и неоптимизируемый код. 
                                      };
                                    }
                                    

                                    Как-то так. Извините — пишу по памяти.
                                      0
                                      Если вы потом собираетесь mtObject читать вообще в обход doMultithreaded(), то, собственно, нахрена тут DCL? DCL это _оптимизация_ кейса когда вы хотите _очень часто_ вызывать doMultithreaded(), но не хотите платить слишком много за лок. Если вы где-то наверху один раз вызываете doMultithreaded() и потом в цикле работаете с объектами, можно выкинуть весь DCL вместе со флагом и любыми волатайлами, делать все проверки под локом.

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

                                      Кстати в вашем случае, если 2 объекта, отдельный флаг ничем не лучше, а, точнее, даже хуже, чем сделать только один из инициализируемых объектов volatile, записывать его последним и сравнивать с null при входе в doMultithreaded() именно его.
                                        0
                                        От reordering не защитит, если потом они не будут читаться в обратном порядке. Т. к. happens before будет только по одному полю.
                                          0
                                          Ничего не понял, что не защитит? Надо в первом условии сравнивать именно volatile ссылку, и если она не null, читать все остальные. И все будет хорошо.
                                            0
                                            Object o1;
                                            volatile Object o2;
                                            
                                            private void m1() {
                                              o1 = ...;
                                              o2 = ...; // volatile write, happens before reading o2, 
                                              // so writting o1 happens before reading o2 (transitive)
                                            }
                                            
                                            private void m2() {
                                              // m1() in other thread
                                              o1.toString(); // unsafe, no happens before relation to o1 write
                                            }
                                            
                                            private void m3() {
                                              // m1() in other thread
                                              o1.toString(); // can be either visible or not
                                              o2.toString();
                                            }
                                            
                                            private void m4() {
                                              // m1() in other thread
                                              o2.toString(); // volatile read, o1 initialization is visible on next string
                                              o1.toString();
                                            }
                                            


                                            В функции m3 всё может выглядеть рабочим, а может ломаться. В m4 же всё ок. То, что эта перестановка (m4->m3) вносит гейзенбаг может быть неочевидно для модифицирующего код. Решит какой-нибудь junior поставить два независимых вызова по алфавиту, что в этом такого…

                                            Извините, что некорректно использовал термин reordering.
                                              +2
                                              Если m1() идет параллельно с m2/3/4, вы обязаны все обращения завернуть в if (o2 != null), иначе тупо будет NPE. вряд ли джуниор осмелится заменить проверку o2 != null на o1 != null.
                                                +1
                                                Тоже верно, если говорить о lazy инициализации.

                                                Если же объекты инициализированы ранее, а в m1 происходит идемпотентная операция, меняющая состояния o1 и o2, то ошибка проявится. Не вижу, правда ни одной причины не использовать в таком случае обычный лок.
                                          0
                                          Если volatile будет сам объект, то при частом к нему обращении будет знатная потеря производительности, как на самом обращении, там и на окружающем коде — второй пример с циклом (там, кстати у меня ошибка — вместо o2 должно быть o1).
                                            0
                                            Перед использованием достаточно сохранить ссылку на него в локальную переменную, как здесь уже упоминали, чтобы избежать многократных volatile read.
                                              0
                                              Можно ссылку сохранить, но тогда нужно помнить об этом каждый раз. И если забыть, то всё будет работать.
                                                0
                                                Так в вашем случае нужно не забыть прочитать флаг, иначе будет не просто медленно, а некорректно. А если вы флаг читать не собираетесь, то вам вообще волатайл не нужен тут.
                                                  0
                                                  Или я чего-то не понимаю. Или одно из двух. В общем случае, при DCL Вы тоже должны о синхронизации помнить, т.к. DCL лишь патерн.
                                                  Более того — у меня в примере самый что ни на есть чистый DCL! Double-checked locking! Вы по-моему не понимаете, что DCL — патерн именно синхронизации. А уже на чём он выполняется — на отдельном флаге или на самих данных (если язык программирования позволяет) — уже тонкости (разновидности) реализации этого патерна. И я говорю о том, что реализация патерна с полезными данными в качестве флага может иметь весьма не очевидные побочные эффекты в плане производительности.
                                                  А Вы не мне почему-то рассказываете про то, что нужно не забыть флаг проверить… Получается, что если мы делаем DCL, то нужно не забыть сделать DCL…
                                                    0
                                                    Определенная логика есть в флаге, если вы очень не любите выносить поля в локалы. Главная проблема в том, что забытие «бесполезного» флага не означает мгновенный фейл тестов (x86). А если у вас volatile на чем-то полезном, его невозможно забыть, потому что он-то вам, собственно, и нужен. + Меньше чтений из памяти, опять же.
                                                      0
                                                      Именно так. Но зато можно забыть, что с этим полем нельзя активно работать (например в циклах). И тоже никакого фейла тестов не будет. Более того на x86 может даже не быть катастрофической просадки производительности. А на каком-нибудь ARM'е оно не в 40 раз замедлится, а в тысячу. И кто его знает, когда это заметят и заметят ли.
                                                        +1
                                                        Если не заметят, то может и хрен с ним?! Корректно же
                                                          +1
                                                          Ну, тут уже программисту решать, где оно может работать и на сколько нужно оптимизировать.
                                              0
                                              Что мешает перед циклом сохранить ссылку в локальную переменную? Потому что для такого цикла вы в любом случае должны быть уверены, что инициализация уже прошла, иначе это тупо нерабочий код. Подумайте сами: для какой-либо синхронизации, вы в любом случае должны сделать хоть одно volatile чтение чего угодно. Если вы не читаете свой флаг, то значит вам синхронизация в принципе не нужна.
                                                0
                                                Ссылку можно сохранять, но тогда нужно об этом помнить и если забыть даже в критичном месте, то на тестах оно никак не проявится (кроме производительности). С флагом забыть нельзя (можно только вообще про синхронизацию забыть, но тогда уж ССЗБ).
                                                Ясен перец, что для синхронизации нужно сделать минимум одно volatile чтения. я говор о том, что применяя в качестве флага DCL полезные данные (а не отдельный флаг) можно легко забыть о том, что обращение к этим данным не просто относительно тяжёлая операция, но ещё и «ломает» оптимизацию окружающего кода.
                                                  0
                                                  С флагом забыть можно, и самое скверное, что на x86 у вас скорее всего все будет и так работать…
                                                  0
                                                  Подумайте сами: для какой-либо синхронизации, вы в любом случае должны сделать хоть одно volatile чтение чего угодно.
                                                  Строго говоря не чего угодно, а той же переменной. Если смотреть в новый jls8, 17.4.4-5, то там сказано
                                                  A write to a volatile variable v (§8.3.1.4) synchronizes-with all subsequent reads of v by any thread (where «subsequent» is defined according to the synchronization order).
                                                    0
                                                    Ну это я обобщил, «что угодно» флаг/не флаг в смысле. Хотя да, на HotSpot (и на других VM думаю тоже), есть такой эффект, что и впрямь можно читать писать что угодно volatile для синхронизации, чем можно иногда пользоваться, писать в заведомо thread-local volatile, чтобы компилятор поставил барьер, не создавая при этом contention по памяти.

                                                    Ну и в текущем JLS то же самое, естественно, строго говоря надо читать писать одно место.
                                                      0
                                                      Т. е. получаем костыль вместо Unsafe.storeFence()
                                                        0
                                                        Не тянуться же лишний раз за s.m.Unsafe ,)
                                                0
                                                Что мешает doMultithreaded() вызывать часто? Не говоря уже о том, что может быть несколько разных многопоточных функций работающих с этими переменными и достаточно лишней одной из них быть «тяжёлой», что круто повлиять на производительность.
                                                Я, как бы, когда пример писал, то считал, что очевидно, что раз мы говорим о DCL, то какая-то причина, почему DCL нужен, есть. Ясно же что doMultithreaded лишь одна конкретная функция демонстрирующая проблему с DCL, когда в качестве флага используются непосредственно сами полезные дынные.
                                                  0
                                                  Если у вас в любом случае под каждым doMultithreaded() 1000 итераций, экономия одного лока это ничто, скорее сыграет экономия байткодов на DCL, мож какой метод заинлайнится лишний раз. Если есть еще doMt2(), doMt3() и т. д., где нет такого толстого цикла, если вы в этих других методах не читаете свой флаг, они поломаны, а если читаете, вы делаете 1 volatile чтение + 2 не volatile, хотя могли бы 1 + 1. Флаг просто лишним чтением оказывается, если барьерно можно прочитать что-то полезное.
                                                    0
                                                    У меня может быть ещё 10 методов где используются эти переменные. Если у Вас 10 лёгких методов и 1 тяжёлый, то даже относительно редкий вызов тяжёлого, если он замедлится на два-три порядка может оказать заметное влияние на производительность. Более того, где гарантия что эти лёгкие методы в результате.
                                                    И разве я говорил о том, что DCL на полезных данных всегда плохо? Я лишь говорю, что при этом могут быть побочные эффекты в плане производительности.
                                                    Например, я видел код, в котором был DCL на полезный данных. Но по мере развития проекта эти данные использовались всё чаще и чаще. В конце концов начались проблемы с производительностью. Расследование показало, что виновато часто используемое volatile-поле. Ведь у Вас DCL может один раз отработать при запуске, например, какого-нибудь worker'а, который потом будет с этими данными активно работать (повторная синхронизация уже не нужна будет). И в этом worker'е volatile переменная запросто может использовать очень часто в любых циклах.
                                                    В конце-концов DCL не только для синглтонов используется. Это именно патерн синхронизации и использоваться он может для самых различных целей синхронизации.
                                                      0
                                                      Занятно что я всегда DCL воспринимал как чисто теоретическое упражнение, ни разу в жизни его не писал. Приведите пример реально полезного DCL?
                                                        0
                                                        Нагруженное многопоточное приложение, где DCL используется для передачи команд исполнительным потокам. Если исполнительных потоков много и обмен интенсивный, то синхронизация будет очень тяжёлой операцией. В Java8 для этого новые объекты синхронизации ввели и, в принципе, DCL не нужен, вроде как.
                                                        А как вы собираетесь сделать легковесную синхронизацию между несколькими десятками потоков иначе? Либо атомарные счётчки городить, либо DCL. Всё остальное тяжёлое получается.
                                                          0
                                                          То есть есть очередь команд и много потоков выгребают из нее наперегонки? Не очень понимаю при чем тут DCL
                                                            0
                                                            Затрудняюсь ответить :) Но однажды я использовал его именно в таком случае. Второй раз — в качестве флага/общих данных. При многопоточной работе и не такое случается. Особенно если нужно эффективно реализовать какие-нибудь очереди/списки при большом количестве параллельно работающих процессов.
                                                        +1
                                                        Приведите пример реально полезного DCL?
                                                        В Scala с помощью DCL сделаны lazy-значения ) Т.е., насколько я понимаю, компилятор разворачивает lazy val abc = ... в DCL с битовым флагом.
                                                        0
                                                        Ту же самую проблему можно было решить, просто повыносив volatile чтения в локальные переменные (или перекопируя в не-volatile поле объекта, работа с которым идет в одном потоке). Это было бы чуть-чуть быстрее, чем отдельный флаг. Да, есть возможность забыть про вынесение в новом коде и опять получить проблему. Зато теперь у вас есть возможность забыть про флаг и НЕ получить фейл тестов, зато получить баги гораздо позже на другой платформе/обновлении Java, которая поумнеет и станет что-то переставлять агрессивнее, чем раньше.
                                                          0
                                                          Несомненно. Но у вас же синхронизация может быть в одном-двух местах в коде. В остальных случаях те же данные будут использоваться без синхронизации. Получили некоторое задание для рабочего процесса и работаете. Синхронизация с данными ровно в двух местах — выдача задания и проверка задания, а работа с данными — в сотне. Естественно, это могут быть не только данные, а какие-то флаги, используемые многими исполнительными процессами или ещё что-то подобное. Очереди всякие, опять же — там вообще самые разнообразные варианты могут получаться.
                                                            0
                                                            DCL нужен только когда у вас гонка на публикацию _одного и того же_ задания, что выглядит немного как бред. В противном случае, просто спокойно подготовили задание, положили в AtomicReference, потом рабочий поток из AtomicReference задание достает и работает.
                                                              0
                                                              Это может быть не простая очередь FIFO, а какой-нибудь неблокирующий список. Или это может быть не задание а какие-то текущие общие данные для большого количества потоков. Это может быть какой-то интерфейс обработчика какого-то события для большого количества событий.
                                                              И я знаю, как делать очереди и вообще не плохо разбираюсь в многопоточном программировании :) И даже AtomicReferece, как правило не нужен — есть же синхронные очереди. Тем не менее, случаи бывают разные.
                                                  +1
                                                  Интересный способ, должен работать из-за read barrier для volatile. Но судя по stackoverflow.com/questions/10620680/why-volatile-in-java-5-doesnt-synchronize-cached-copies-of-variables-with-main не всегда работает корректно :)
                                                  А что бы не было каждый раз обращения к volatile полю в цикле, можно его присвоить не volatile полю и обращаться.
                                                    +1
                                                    Ну пугайте людей, это был баг в Java. С таким же успехом можно сказать, что память не всегда работает корректно, потому что раз в миллиард лет можно записать байтик и прочитать другой. Триггеры там может поплавились и неправильно сработали, я не знаю. Если на это закладываться ничего разрабатывать вообще нельзя.
                                                    0
                                                    Не заметил, что цикл сам располагается в doMultithreaded(). Таки да, DCL тут просто не нужен.
                                                      0
                                                      И как это эффективно сделать без DCL?
                                                        +1
                                                        MTObject mtObject;
                                                        MTObject2 mtObject2;
                                                        
                                                        void doMultithreaded() {
                                                            synchronized (syncObj) {
                                                              if (mtObject==null) {      
                                                                // Инициализация.
                                                               mtObject = new MTOject();
                                                                mtObject2 = new MTOject2();
                                                              }
                                                            }
                                                           // Код работы с переменными.
                                                           for (int i=0; i!=1000; i++)
                                                              m1.doSomething();
                                                        };
                                                        

                                                        DCL это типа же «продвинутый» наворот, не стоит забывать что существует «просто» locking! :)
                                                          0
                                                          Ну, раз мы говорим о DCL, то по какой-то причине он у нас уже появился :) Значит кто-то где-то с этими данными часто многопоточно работает на чтение (на самом деле это не единственная возможная причина использования DCL — например, может просто не быть «удобного» объекта синхронизации, если пишется какая-то универсальная библиотека).
                                                            0
                                                            И если этот код часто вызывают 50 процессов, то что случиться? Поляжет этот synchorize и положит производительность. Как раз, если тут больше десятка процессов и/или оно часто вызывается, то не нужно обычную синхронизацию делать.
                                                        +3
                                                        >> volatile int dclFlag; // int в Java на всех существующих платформах атомарный, а boolean — нет.

                                                        Откуда дровишки?

                                                        JVMS §2.3.4:

                                                        Instead, expressions in the Java programming language that operate on boolean values are compiled to use values of the Java Virtual Machine int data type.
                                                          –1
                                                          // В случае если mtObject volatile, то это превращается в ад.

                                                          Если при чтении volatile у вас действительно сильно проседает производительность, реализуйте такой код:
                                                          // это глобально видимая ссылка на объект
                                                          volatile MTObject mtObject;
                                                          ...
                                                          MTObject threadMTObject = mtObject;
                                                          ...
                                                          for (int i=0; i!=1000; i++)
                                                                threadMTObject.doSomething();
                                                          

                                                          В потоке вы можете единожды прочитать из volatile в локальную переменную, и уже ее использовать как угодно потоку.
                                                        0
                                                        Господа минусующие, не изволите указать на мою ошибку в этом рассуждении?
                                                          +3
                                                          Как сторонник аргументированных обсуждений не могу пройти мимо вашего комментария. Чтобы понять логику ваших рассуждений, позвольте уточнить несколько моментов:

                                                          А как тогда, по Вашему у объектов, существующих одновременно в нескольких потоках, ссылки подсчитывются? ;) Будет же «застревать в кэше» счётчик ссылок? :)

                                                          О каком счётчике ссылок идёт речь?

                                                          Кроме того — завершение секции synchronize приведёт к записи переменной, т.к. является барьером синхронизации в Java, разве нет?

                                                          С чего вы взяли, что «секция synchronize» является барьером синхронизации? Что вы вообще имеете в виду под «барьером синхронизации»? В терминах JMM, если можно.
                                                          +1
                                                          Так для взаимодействия двух потоков имеет значение happens before. А для него не достаточно одной синхронизации в пишущем потоке, нужна ещё синхронизация в читающем.
                                                          Грубо говоря: да, первый поток сбросит изменения в общую память при выходе из synchronize, но второй поток так и будет читать свой L3 кэш.
                                                            0
                                                            Ну, так я и думал. Правда, на когерентных кэшах работать будет :) Я так никогда не делаю ;)
                                                              +2
                                                              Это невозможно по семантике ассемблера, и вообще. Кто-то может читать свой кеш только пока кто-то другой еще НЕ сбросил изменения в общую память. Volatile чтение нужно для запрета переупорядочивания, а не для того, чтобы «прочистить» кеш (это работа процессора).
                                                                +2
                                                                Строго говоря, тут я ошибался. Это невозможно по семантике x86 ассемблера, но возможно по семантике некоторых других ассемблеров, на которых кеш действительно надо прочищать «руками» и volatile-чтения генерируют соотв. инструкции. Правда не L3, a L1/L2, потому что это L1 находится в ядре, а L3 это общий для всех ядер интерфейс к памяти, про него вообще можно никогда не думать.

                                                                См. habrahabr.ru/post/209128/, первую часть статьи и mechanical-sympathy.blogspot.com/2011/07/memory-barriersfences.html?showComment=1407914816407#c2882832980378499377
                                                                +1
                                                                В каком то смысле «кешем» можно считать, собственно, регистры с прочитанными значениями из памяти. Регистры-то никуда не сбрасываются сами, поэтому volatile чтение в каком-то смысле прочищает только их, а на деле просто не сохраняет значение volatile поля в регистре, а читает каждый раз по новой.
                                                                  –1
                                                                  А регистры будут сброшены по закрытию секции synchronize ;)
                                                                    +3
                                                                    Ага, особенно регистры другого ядра.
                                                                      –2
                                                                      А зачем нам тут другое ядро? Будут сброшены все изменения внутри секции.
                                                                +1
                                                                Только относительно входа в syncronized на тот же лок. Для остальных happens before не возникает.
                                                                  –2
                                                                  Кроме того — завершение секции synchronize приведёт к записи переменной, т.к. является барьером синхронизации в Java, разве нет?
                                                                  Я поизучал и поспрашивал на SO. Кратко: volatile тут таки необходим.

                                                                  Подробно
                                                                  Возьмем пример
                                                                  class Foo {
                                                                      private volatile Bar _barInstance;
                                                                      public Bar getBar() {
                                                                          if (_barInstance == null) {
                                                                              synchronized(this) { // or synchronized(someLock)
                                                                                  if (_barInstance == null) {
                                                                                      Bar newInstance = new Bar();
                                                                                      // possible additional initialization
                                                                                      _barInstance = newInstance;
                                                                                  }
                                                                              }
                                                                          }
                                                                          return _barInstance;
                                                                      }
                                                                  }
                                                                  

                                                                  Компилятор/рантайм могут переставить инструкции, связанные с инициализацией объекта. Ссылка _barInstance может быть присвоена раньше, чем выполнятся инструкции конструктора. Другой поток может увидеть, что _barInstance и, соответственно, не брать lock. А значит happens-before на этом блоке синхронизации не случится и появляется риск использования недоинициализированного объекта.
                                                                    +2
                                                                    Господа хабраюзеры, а за что минусы? Если я в чем-то фактически не прав, то напишите, а то ведь так и помру в неведении.
                                                                      +1
                                                                      Здесь, в комментариях уже несколько раз разжевали почему он необходим. Поэтому дополнительный комментарий про то же — не нужен.

                                                                      В этом топике у меня складывается впечатление, что почти никто из задававших вопросы не читал ответы на эти же вопросы, заданные другими комментаторами.
                                                                        0
                                                                        Да, согласен, виноват :) Слишком поспешил.
                                                                –3
                                                                Чего минусуете?
                                                                Если переменная для DCL одна, то на черта делать её volatile?
                                                                Пример:
                                                                DCLVarType dclVar;
                                                                DCLVarType getDCLVar() {
                                                                  if(dclVAR!=null)
                                                                    return dclVar;
                                                                  synchronized(syncVar) {
                                                                    if(dclVAR!=null)
                                                                      return dclVar;
                                                                    dclVar = new DCLVarType(); // Тут можно долго и сложно конструировать через промежуточную переменную.
                                                                    return dclVar;
                                                                  };
                                                                }
                                                                

                                                                В каком случае оно может не работать?
                                                                Сразу каюсь — я уже Java несколько подзабыл.
                                                                  0
                                                                  В Java, если вы не используете ключевое слово volatile, одна и таже переменная (как в вашем примере dclVar) в разных потокаж может быть или null или проинициализирована (именно для вашего примера, в общем случае «иметь разные значения»). Цель DCL в том, чтобы вернуть вам уже проинициализированную только 1 раз переменную.
                                                                    –2
                                                                    Ссылки на объекты в Java, на сколько я знаю, атомарны — нельзя два раза проинициализировать объект.
                                                                      +1
                                                                      Почитайте про кэширование переменных в разных потоках, а также про значение ключевого слова volatile в java.
                                                                      Без этого слова по факту потоки могут видеть разные значения одной и той же переменной.
                                                                        –2
                                                                        Т.е., получается, что в Java, можно устроить утечку памяти (либо, двойное освобождение объекта) через многопоточную работу с не-volatile объектом? Нельзя же.
                                                                          0
                                                                          Вы неверно трактуете значение ключевого слова volatile.
                                                                          Опишите ваше понимание того, как будет происходить утечка памяти, а сообщество объяснит вам, где вы ошиблись.
                                                                    +3
                                                                    Сколько можно это обсасывать… читайте все предыдущие статьи по DCL, объясняющие потенциальный баг в этом коде
                                                                  0
                                                                  Сразу скажу (чтобы не закидали какашками), что лично я, как правило, если использую DCL, то предпочитаю иметь отдельный volatile флаг, т.к. пометка самое переменной как volatile может снизить производительность. Т.к. мало того, что каждый раз будет производиться чтение переменной при частом использовании, так чтение volatile ещё и является барьером в Java и сильно снижает общую оптимизацию программы — т.е. все оптимизации будут ограничены участками между использованием volatile переменной, что бывало, производительность на порядок роняло.
                                                                    0
                                                                    пример кода, пожалуйста?
                                                                    +3
                                                                    Спасибо автору! Обязательно посмотрим, поправим.
                                                                        +1
                                                                        Спасибо за внимание к моим публикациям, оставайтесь на нашем канале :-)
                                                                          0
                                                                          видимо внутри идеи многопоточность не используется так интенсивно, раз у ее разработчиков проскакивают такие ошибки.
                                                                          Но все равно странно, если используется кодревью, то такое обычно выявляется и не доходит до прода.
                                                                            0
                                                                            Да в любом большом проекте такие косяки есть. Вы знаете, что в idea.jar 50000 классов? Вы ревьювить это годами будете, а кастомеры новых фич хотят тем временем.
                                                                        +1
                                                                        Заведите, пожалуйста, тикеты в соответствующих проектах, если их ещё нет.
                                                                          +5
                                                                          Ну для IDEA, как я понял, публикации на Хабре достаточно, чтобы они обратили внимание :-) В ICU4J, так и быть, завёл, раз уж у меня готовый тест-кейс был. Хотя в этом проекте вообще куча серьёзных багов находится статическим анализом, там можно долго заводить.

                                                                          Вообще это часто гиблое дело, которое съедает много времени. Мало того, что надо зарегистрироваться в куче разнородных баг-трекеров и разобраться с разной вики-разметкой, так ещё приходится убеждать людей, что баг, найденный статическим анализом, действительно может повлечь реальные проблемы. Некоторые требуют конкретного воспроизведения бага, им недостаточно указания на подозрительное место в коде. Некоторые говорят «это было написано давно человеком, который уже с нами не работает, поэтому мы лучше не будем трогать, а то ещё что-нибудь сломаем». В общем, после нескольких попыток я бросил репортить баги каждый раз. В конце концов, FindBugs бесплатен, скачайте, поставьте и пользуйтесь :-)
                                                                            +1
                                                                            О, пофиксали оказывается. Ну значит, всё не зря :-)
                                                                            +1
                                                                            Автор, вы баги-то зафайлили? Народ дело спрашивает.
                                                                              +3
                                                                              Выше ответил.
                                                                                –1
                                                                                нене, рекламировать FindBugs хорошо конечно, но баги нужно файлить! Потому что хабр, опенсорс, карма, уважение пацанов и все такое.
                                                                                  +1
                                                                                  Вы вполне можете взять на себя эту почётную обязанность :-) Все мы добровольцы и жертвуем своим временем, которого и так не очень много.

                                                                            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                                                            Самое читаемое