Once again (I hope the last one) about double-checked locking
There were so many articles about double-checked locking on Habré that it would seem another one - and Habr would burst. Here are some good publications on Java: The implementation of Singleton in JAVA , The correct Singleton in Java , But how does multithreading work? Part II: memory ordering or here's a great post from TheShade ( thank web-archive! ). Nowadays, probably every Java developer has heard that if you use DCL, kindly declare a variable volatile. It is rather difficult to find today in the code of well-known open source DCL projects without volatile, but it turned out that the problems have not been completely solved. Therefore, I will add a short note on the topic with examples from real projects.
Sometimes it seems that programmers do not include brains and do not try to understand how things work, but simply follow simple and understandable rules like “declare a variable volatile, 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 of publication of an object is a volatile-record operation, and not an exit from the synchronization section. Therefore, it is the volatile-record that should be made after the object is fully initialized.
Here, for example, such code was found in the ICU4J project - TZDBTimeZoneNames # prepareFind :
The developer wrote volatile, because somewhere I 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 that, the prepareFind calls in other threads will immediately exit without synchronization. At the same time, after the publication, many additional steps for initialization are performed.
')
This method is used when searching for a time zone, and this search is quite possible to break. Under normal conditions,
The result is always different, but something like this:
Here is another example from IntelliJ IDEA - FileEditorManagerImpl # initUI :
Here the authors even made a private lock-object for reliability, but the code is still broken. Of course, nothing terrible will happen if you start using myPanels with an unset border, but the problem is more serious: DCL runs on one variable (myPanels), and two are initialized (also mySplitters), and again volatile-writing occurs before full initialization . As a result, getMainSplitters () with competitive access may well return null.
Such things are fixed very easily: it is necessary to save the object to a local variable, use it to call all the necessary methods for initialization, and only then write a volatile field.
Another couple of suspicious places:
Tomcat DBCP2 BasicDataSource : it is possible to see a dataSource object that does not have a logWriter installed.
Apache Wicket Application # getSessionStore () : it is possible to see a sessionStore with an unregistered listener.
Real problems are hardly possible here, but you still need to write carefully.
I added a small heuristic check in FindBugs, which warns of such situations. However, it may not always work, so it's better to rely on your head.
Sometimes it seems that programmers do not include brains and do not try to understand how things work, but simply follow simple and understandable rules like “declare a variable volatile, 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 of publication of an object is a volatile-record operation, and not an exit from the synchronization section. Therefore, it is the volatile-record that should be made after the object is fully initialized.
Here, for example, such code was found in the ICU4J project - 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); ... } } } } } The developer wrote volatile, because somewhere I 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 that, the prepareFind calls in other threads will immediately exit without synchronization. At the same time, after the publication, many additional steps for initialization are performed.
')
This method is used when searching for a time zone, and this search is quite possible to break. Under normal conditions,
new TZDBTimeZoneNames(ULocale.ENGLISH).find("GMT", 0, EnumSet.allOf(NameType.class)) should produce one result. Run this code in 1000 threads:Test code
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); } } 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 streams did not find anything, a couple of streams generally fell with the exception. Yes, in a real application, this is unlikely, but if the authors are not interested in a scenario with high competitiveness, then you can do without volatile and DCL.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, nothing terrible will happen if you start using myPanels with an unset border, but the problem is more serious: DCL runs on one variable (myPanels), and two are initialized (also mySplitters), and again volatile-writing occurs before full initialization . As a result, getMainSplitters () with competitive access may well return null.
Such things are fixed very easily: it is necessary to save the object to a local variable, use it to call all the necessary methods for initialization, and only then write a volatile field.
Another couple of suspicious places:
Tomcat DBCP2 BasicDataSource : it is possible to see a dataSource object that does not have a logWriter installed.
Apache Wicket Application # getSessionStore () : it is possible to see a sessionStore with an unregistered listener.
Real problems are hardly possible here, but you still need to write carefully.
I added a small heuristic check in FindBugs, which warns of such situations. However, it may not always work, so it's better to rely on your head.
Source: https://habr.com/ru/post/248041/
All Articles