You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@storm.apache.org by zd-project <gi...@git.apache.org> on 2018/08/10 17:25:26 UTC

[GitHub] storm pull request #2800: STORM-3162: Fix concurrent modification bug

GitHub user zd-project opened a pull request:

    https://github.com/apache/storm/pull/2800

    STORM-3162: Fix concurrent modification bug

    See: https://issues.apache.org/jira/browse/STORM-3162
    
    I managed to alter the Java implementation and it seems to be passing storm-server test. However in storm-core there's a nimbus_test.clj which depends on some of the older implementation that I changed in this PR. I myself am not very familiar with Clojure so I don't know how to fix it. If any of you can take a look it'll be great.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/zd-project/storm STORM-3162

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/storm/pull/2800.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 #2800
    
----
commit e7bacee9ecc2c136a7ef3c5d3144eae2d95a9306
Author: Zhengdai Hu <zh...@...>
Date:   2018-08-08T21:33:19Z

    STORM-3162: Fix concurrent modification bug

----


---

[GitHub] storm pull request #2800: STORM-3162: Fix concurrent modification bug

Posted by danny0405 <gi...@git.apache.org>.

Github user danny0405 commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2800#discussion_r212800892
  
    --- Diff: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
    @@ -490,7 +493,7 @@ public Nimbus(Map<String, Object> conf, INimbus inimbus, IStormClusterState stor
                 stormClusterState = makeStormClusterState(conf);
             }
             this.stormClusterState = stormClusterState;
    -        this.heartbeatsCache = new AtomicReference<>(new HashMap<>());
    +        this.heartbeatsCache = new ConcurrentHashMap<>();
    --- End diff --
    
    Please change back to `AtomicReference` cause it is multi_thread visible, actually the thrift server serves the RPC methods through multi threading, so we should keep the heartbeatsCache modification be seen as much as possible.


---

[GitHub] storm issue #2800: STORM-3162: Fix concurrent modification bug

Posted by srdo <gi...@git.apache.org>.

Github user srdo commented on the issue:

https://github.com/apache/storm/pull/2800

I'm sorry if this turns a bit verbose, but I'm going to write down what I see as the issue here, so we can hopefully come to a common understanding (and so I don't forget and have to look at this again)

As far as I can tell, the uses of `heartbeatsCache` in Nimbus are thread safe, because the values are never modified, just overwritten. That is, we don't do `heartbeatsCache.get(topoId).put(foo, bar)`, instead we do `heartbeatsCache.getAndUpdate(func)`, which replaces the value entirely. I don't believe we need further synchronization here, since the `AtomicReference` ensures that the value changes are propagated to all threads, and two threads reading from an effectively immutable map at the same time should be fine(?)

However in the `updateHeartbeatCache` method in StatsUtil https://github.com/apache/storm/blob/4c42ee3d259d5d90a4e7d3445d1c119601eec6c7/storm-client/src/jvm/org/apache/storm/stats/StatsUtil.java#L1565 we take one of the values from `heartbeatsCache` and modify it.

There are a couple of problems here. First, the `cache` value is a regular `HashMap` and not a `ConcurrentHashMap`, so modifying it from two threads at once isn't safe. Second, in the branch in `updateHeartbeatCache` where `executorBeats` is null, we iterate over the `cache` parameter. If one thread is in the iteration, and another thread is in the other branch in `updateHeartbeatCache`, we get the exception.

The reason this exception isn't thrown in a real cluster is that the `executorBeats` parameter is only null when called from https://github.com/apache/storm/blob/4c42ee3d259d5d90a4e7d3445d1c119601eec6c7/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java#L1681

This only happens when Nimbus is booting up as part of `launchServer`, or when someone triggers a rebalance in the topology. We see it in the tests, because Nimbus and the supervisors are started concurrently, so Nimbus can be in one branch in `StatsUtil.updateHeartbeatCache` while one of the supervisors is in the other branch. It can technically happen in a real cluster, but someone would have to get extremely unlucky with rebalance timing.

I think the fix here should be making sure that `StatsUtil.updateHeartbeatCache` is thread safe. One option is to make the `cache` value a `ConcurrentHashMap`. Another option would be to make `updateHeartbeatCache` create and return a new map, instead of modifying the existing one.

---