线程安全映射操作

Question

我遇到了以下代码，并注意到一些不一致 - 对于多线程安全代码。

    Map<String,Map<String,Set<String>> clusters = new HashMap<.........>;
    Map<String,Set<String>> servers = clusters.get(clusterkey);
    if(servers==null){
      synchronized(clusterkey){
       servers = clusters.get(clusterkey);
       if(servers==null){....initialize new hashmap and put...}
      }
    }
    Set<String> users=servers.get(serverkey);
    if(users==null){
      synchronized(serverkey){
       users=servers.get(serverkey);
       if(users==null){ ... initialize new hashset and put...}
      }
    }
    users.add(userid);

1. 为什么地图会在群集密钥上同步 - 它不应该作为监视器本身在地图上吗？
2. 最后的users.add不应该同步吗？
3. 这似乎是以线程安全的方式添加单个用户的许多代码。什么是更智能的实现？

Answer 1

这里只是一些观察：

1. Synchronizing on a String is a very bad idea - > clusterKey和serverKey的同步可能不会按照预期的方式工作。
2. 更好的是使用ConcurrentHashMaps和ConcurrentHashSets。

虽然没有更多的背景，但实际上不可能回答这个问题。似乎代码作者想要安全地为每个clusterKey和serverKey创建一个映射，因此用户只需添加一次。

一个（可能更好）的方式是在synchronize地图本身上只有clusters然后你是安全的，因为只有一个线程可以读取和/或写入所述地图。

另一种方法是使用自定义Locks，可能一个用于阅读，另一个用于写入，但如果一个线程写入Map而另一个线程正在从中读取该确切值，则可能再次导致不一致。

Answer 2

该代码看起来像Double checked locking idiom的非思想版本，有时用于延迟初始化。阅读提供的链接，了解为什么这是一个非常糟糕的实现。

给定代码的问题是它间歇性地失败。当有多个线程尝试使用相同的键（或具有相同哈希码的键）在地图上工作时存在竞争条件，这意味着首先创建的映射可能被第二个散列映射替换。

Answer 3

1 - 同步试图避免这两个线程同时在该Map中创建一个新条目。第二个必须等待，所以他的(servers==null)也不会返回true。

2 - users列表似乎超出范围，但似乎不需要同步。也许程序员知道没有重复的userIds，或者他可能不关心一次又一次地重置同一个用户。

也许3- ConcurrentHashMap？