I've been working in the conversion system lately, and discovered a
problem with it's DCL anti-pattern usage. This is not meant to single
out the conversion framework, it's just a convient example.
In Converters.getConverter(Class, Class), there is this pattern:
===
Converter result = convertMap.get(key);
if (result == null) {
if (!noConversions.contains(key)) {
synchronized (converterMap) {
....
if (condition) {
converterMap.put(key, converter);
}
...
noConversions.add(key);
...
}
}
}
===
Here are the issues with this code.
No protection on the first get. This can break if thread has hit
asking for this particular key first, enters the synchronized block,
calls put, and the map starts to rehash itself. In the middle of this
rehashing, a second comes in, and tries to find the key. It's
possible that the get could go into a never-ending loop.
There is no protection at all on the noConversions set. That is just
plain bad.
Now, it must be said, that there are ways to protect against this.
First, place everything inside synchronization. This has issues which
we are all aware of. Second, use a concurrent algorithm. I am not
aware how much FastMap/FastList/FastSet obey this. ConcurrentHashMap
gets around it by having 32 internal maps; each internal map *does*
have synchronization around it.
The other main thing to keep in mind, that if the value placed into
the map is a read-only kind of thing, doesn't have any mutative state,
and the creation of the value has no side affects, then you don't have
to have any protection to make certain there is only one instance of
value created. However, that still doesn't change the fact that the
above anti-pattern *can* dead-lock.
