Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/584#discussion_r205950078
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[],
List<ACL> acl,
HashSet<String> list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet<String>();
- ephemerals.put(ephemeralOwner, list);
+ HashSet<String> _list;
+ if ((_list = ephemerals.putIfAbsent(ephemeralOwner,
list)) != null) {
+ list = _list;
+ }
}
synchronized (list) {
--- End diff --
I'm not sure about that.
ephemerals.putIfAbsent() is thread safe: it guarantees that newly created
instance will be added to the HashMap only if the key is associated with a null
value. Otherwise you will get the instance already present in the HashMap.
Either case `list` will point to the one and only instance present in the
hashmap.
In your example when Step2 enters L483 it will get back the instance which
was created and added to the HashMap by Step1 before it acquired the `list`
lock. And that instance cannot be replaced in the HashMap with this code.
After all `synchronized (list)` only guards access to the List, HashMap is
guarded by putIfAbsent().
I still believe that `computeIfAbsent()` is more convenient and readable
here.
---
