DCL(double-checked locking) is bad, mkay?

[Adam Heath <do...@brainfood.com>]

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.

Re: DCL(double-checked locking) is bad, mkay?

[Adam Heath <do...@brainfood.com>]

Adrian Crum wrote:
> I agree that DCL is a bad pattern to use, and coincidentally, I was
> researching the subject last weekend - hoping to find a good alternative
> to propose to the community.

Ofbiz has generally been very luck with it's use of DCL.  Most cases
are hit at startup, and from then on, the maps are fully populated.

You can run into errors, however, when you start clearing out the
caches, while the server is under heavy load.

> Some time ago I had proposed a multi-threaded "Best Practices"
> discussion, but it didn't attract much interest.

Sure.  Just wait until I get my commons-vfs virtual file system
committed.  It has tons of non-blocking programming.

Re: DCL(double-checked locking) is bad, mkay?

[Adrian Crum <ad...@hlmksw.com>]

Adam Heath wrote:
> 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.

I agree that DCL is a bad pattern to use, and coincidentally, I was 
researching the subject last weekend - hoping to find a good alternative 
to propose to the community.

Some time ago I had proposed a multi-threaded "Best Practices" 
discussion, but it didn't attract much interest.

The absence of protection on the set is an oversight. You may lash me 
with a wet noodle.