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:
[email protected]
With regards,
Apache Git Services
