[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/584#discussion_r205961633
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[],
List acl,
HashSet list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _list;
+if ((_list = ephemerals.putIfAbsent(ephemeralOwner,
list)) != null) {
+list = _list;
+}
}
synchronized (list) {
--- End diff --
@anmolnar
I will test it with that unit test , using a 'threadpool' and create
100 nodes to reproduce what I guessã
I wish I was wrong,but if i'm right ,you need to watch Breaking Bad series
(smirk).
---
[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
Github user breed commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/584#discussion_r205959572
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[],
List acl,
HashSet list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _list;
--- End diff --
we need synchronization to prevent znode changes while the znode is being
snapshot to disk, but for request processing there is only a single thread that
does mutations and mutations don't happen while read requests are being
processed.
---
[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
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,
HashSet list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _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.
---
[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/584#discussion_r205941687
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[],
List acl,
HashSet list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _list;
+if ((_list = ephemerals.putIfAbsent(ephemeralOwner,
list)) != null) {
+list = _list;
+}
}
synchronized (list) {
--- End diff --
Even if when `createNode` is called by multi-thread.this fix is also not
thread-safe.
Step1:One thread has acquired the `list` lock and enters into L487,add a
element into `list`
Step2:But at the same time just after Step1, another thread enters into
L483,the reference of `list` is pointed to the old `_list` object. It will
cause thread-unsafe
BTW: **Breaking Bad** is the best U.S.TV series
---
[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/584#discussion_r205749086
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[],
List acl,
HashSet list = ephemerals.get(ephemeralOwner);
if (list == null) {
list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _list;
--- End diff --
You can use `computeIfAbsent()` like this:
```java
HashSet list = ephemerals.computeIfAbsent(ephemeralOwner, k -> new
HashSet());
```
---
[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...
GitHub user MichaelScofield opened a pull request: https://github.com/apache/zookeeper/pull/584 ZOOKEEPER-3102: Potential race condition when create ephemeral nodes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/MichaelScofield/zookeeper ZOOKEEPER-3102 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/584.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #584 commit f7b494858e719b2c6dc08befea5bd2caf82c3976 Author: luo.fucong Date: 2018-07-26T04:46:24Z ZOOKEEPER-3102: Potential race condition when create ephemeral nodes. ---
