开发者

Correct using double-check idiom

开发者 https://www.devze.com 2023-01-24 07:37 出处:网络
public class Generator { private static final Map<byte[], byte[]> cache = new HashMap<byte[], byte[]>();
public class Generator {
private static final Map<byte[], byte[]> cache = new HashMap<byte[], byte[]>();

public static byte[] generate(byte[] src) {
    by开发者_开发技巧te[] generated = cache.get(src);
    if (generated == null) {
        synchronized (cache) {
            generated = cache.get(src);
            if (generated == null) {
                generated = doGenerate(src);
                cache.put(src, generated);
            }
        }
    }
    return generated;
}

private static byte[] doGenerate(byte[] src) {...}

}

Can anybody answer, what's wrong in this code? Maybe generate() method can return partially constructed array, isn't it?


Replace your HashMap with ConcurrentHashMap.

Your synchronization looks not useful. I would rewrite the whole thing in this way:

private static final Map<byte[], byte[]> cache = new ConcurrentHashMap<byte[], byte[]>();

public static byte[] generate(byte[] src, int counter) {
    byte[] generated = cache.get(src);
    if (generated == null) {
        generated = doGenerate(src);
        cache.put(src, generated);
    }        
    return generated;
}

Edit: The try/catch was bizarre and not necessary. For the OP's purposes, it suffices to use a ConcurrentHashMap. If the OP really doesn't want to suffer the costs of calling doGenerate twice for the same dataset, then they will have to continue with the DCL pattern, but use of a ConcurrentHashMap is still recommended.


For a start, you are using the HashMap unsynchronised. This can lead to, say, infinite loops (this is real - it has been observed in production environments).

It also suffers from the general double-checked locking bug of not having a happens-before relationship from writing the contents of the array to reading from it (I am assuming there will be some data).

(Note, you are comparing object identities here, not array contents.)

Another problem is that you have mutable statics, which is poor design that causes serious problems.


While one thread is executing line 11, another thread may be executing line 5. This is why you need a synchronized map here, and why when using an unsynchronized HashMap your results will be indeterminate.

The solution here is a synchronized Map (see java.util.Collections#synchronizedMap). Of course, if you use a wholly synchronized map then your double-checked locking (extra optimization) becomes redundant.

0

精彩评论

暂无评论...
验证码 换一张
取 消

关注公众号