zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [zookeeper] BELUGABEHR commented on a change in pull request #911: ZOOKEEPER-3365: Use Concurrent HashMap in NettyServerCnxnFactory
Date Sun, 14 Apr 2019 15:02:28 GMT
BELUGABEHR commented on a change in pull request #911: ZOOKEEPER-3365: Use Concurrent HashMap
in NettyServerCnxnFactory
URL: https://github.com/apache/zookeeper/pull/911#discussion_r275160987
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java
 ##########
 @@ -512,44 +511,34 @@ public InetSocketAddress getLocalAddress() {
         return localAddress;
     }
 
-    private void addCnxn(NettyServerCnxn cnxn) {
+    private void addCnxn(final NettyServerCnxn cnxn) {
         cnxns.add(cnxn);
-        synchronized (ipMap){
-            InetAddress addr =
-                ((InetSocketAddress)cnxn.getChannel().remoteAddress()).getAddress();
-            Set<NettyServerCnxn> s = ipMap.get(addr);
-            if (s == null) {
-                s = new HashSet<>();
-                ipMap.put(addr, s);
+        InetAddress addr =
+            ((InetSocketAddress) cnxn.getChannel().remoteAddress()).getAddress();
+
+        ipMap.compute(addr, (a, cnxnSet) -> {
+            if (cnxnSet == null) {
+                cnxnSet = new HashSet<>();
             }
-            s.add(cnxn);
-        }
+            cnxnSet.add(cnxn);
 
 Review comment:
   @eolivelli 
   
   Take a look at the Map API for [computeIfAbsert](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-).
 They provide an example of how to use it:
   
   ```
   to implement a multi-value map, Map<K,Collection<V>>, supporting multiple values
per key:
    
    map.computeIfAbsent(key, k -> new HashSet<V>()).add(v);
   ```
   
   The example is for a simple case where there is no multi-threading and the map accumulates
records without removing empty records.  However, in this situation, there are a few issues
with this approach:
   
   1. The underlying `HashSet` is not synchronized, so to threads from the same source could
cause issue on insert
   2. There is a clean-up routine in `removeCnxnFromIpMap` where a key is removed if its associated
`Set` has no entries.  Well, that could also conflict here since it could be the case that
the `Set` returned by the `computeIfAbsent` is returned, the Thread pauses, and then another
thread deletes the key in `removeCnxnFromIpMap` and therefore when the Thread resumes the
`add` happens on a `Set` that was removed.  Better to do it all in a protected area.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message