Once again (hopefully the last) about double-checked locking

There were so many articles about double-checked locking on Habré that one more would seem - and Habr will burst. Here are just some good publications on Java: Implementing Singleton in JAVA , Correct Singleton in Java , But how does multithreading work? Part II: memory ordering or here is a wonderful post from TheShade ( glory web-archive! ). Nowadays, probably every Java developer has heard that if you use DCL, be so kind as to declare a variable volatile. It’s quite difficult to find DCL without volatile in the code of well-known open source projects of DCL, but it turned out that the problems have not yet been completely solved. Therefore, I will add a small note on the topic with examples from real projects.

Sometimes you get the feeling that programmers don’t turn on their brains and do not try to understand how it works, but simply follow simple and understandable rules like “declare a volatile variable, use DCL, and everything will be fine”. Unfortunately, this approach to programming does not always work.

The peculiarity of the DCL pattern is that the moment the object is published is a volatile write operation, and not an exit from the synchronization section. Therefore, it is volatile-record should be made after the complete initialization of the object.

Here, for example, such code was found in the ICU4J project - TZDBTimeZoneNames # prepareFind :

    private static volatile TextTrieMap 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(true);
                    Set mzIDs = TimeZoneNamesImpl._getAvailableMetaZoneIDs();
                    for (String mzID : mzIDs) {
                        ...
                        TZDB_NAMES_TRIE.put(std, stdInf);
                        ...
                    }
                }
            }
        }
    }

The developer wrote volatile, because somewhere he heard that it was necessary, but apparently did not understand why. In fact, the publication of the TZDB_NAMES_TRIE object took place at the time of the volatile record: after this, prepareFind calls in other threads will immediately exit without synchronization. Moreover, after publication, many additional steps are taken to initialize.

This method is used when searching for a time zone, and this search may well be broken. Under normal conditions, it new TZDBTimeZoneNames(ULocale.ENGLISH).find("GMT", 0, EnumSet.allOf(NameType.class))should produce one result. Let's execute this code in 1000 threads:

The result is always different, but something like this:

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

Almost half of the threads did not find anything, a couple of threads generally fell with the exception. Yes, in a real application this is unlikely, but if a script with high competitiveness is not of interest to the authors, then you can do without volatile and DCL at all.

Here is another example from 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;
  }

Here, the authors even made a private lock object for reliability, but the code is still broken. Of course, it can be okay if you start using myPanels with the border not set, but the problem is more serious: DCL is executed on one variable (myPanels), and two are initialized (also mySplitters), and again, volatile recording occurs before full initialization . As a result, getMainSplitters () with competitive access may well return null.

These things can be fixed very easily: you need to save the object to a local variable, use it to call all the necessary methods for initialization, and only then write the volatile field.

Another couple of suspicious places:
Tomcat DBCP2 BasicDataSource : it is possible to see a dataSource object that does not have logWriter installed.
Apache Wicket Application # getSessionStore () : it is possible to see a sessionStore with an unregistered listener.

It is unlikely that real problems are possible, but you should still write more accurately.

I added a small heuristic check in FindBugs, which warns of such situations. However, it may not always work, so better rely on your head.