You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by "Raul Gutierrez Segales (JIRA)" <ji...@apache.org> on 2014/07/19 19:54:38 UTC
[jira] [Commented] (ZOOKEEPER-1972) Fix invalid volatile long/int
increment (++)
[ https://issues.apache.org/jira/browse/ZOOKEEPER-1972?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14067609#comment-14067609 ]
Raul Gutierrez Segales commented on ZOOKEEPER-1972:
---------------------------------------------------
lgtm, +1.
[~phunt]: maybe we can pick this one up for 3.5.0?
> Fix invalid volatile long/int increment (++)
> --------------------------------------------
>
> Key: ZOOKEEPER-1972
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1972
> Project: ZooKeeper
> Issue Type: Sub-task
> Affects Versions: 3.5.0
> Reporter: Hongchao Deng
> Assignee: Hongchao Deng
> Fix For: 3.5.0
>
> Attachments: ZOOKEEPER-1972.patch
>
>
> The findbugs complains about incrementing a volatile variable in [AuthFastLeaderElection|https://github.com/apache/zookeeper/blob/087abf83684167ae56864fe4c3be0079fa653266/src/java/main/org/apache/zookeeper/server/quorum/AuthFastLeaderElection.java#L737] and [FastLeaderElection|https://github.com/apache/zookeeper/blob/087abf83684167ae56864fe4c3be0079fa653266/src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java]:
> {code}
> volatile long logicalclock; /* Election instance */
> ...
> logicalclock++;
> {code}
> Actually this is a bug. It should use AtomicLong here instead of volatile.
> Leader.java and [QuorumPeer.java|https://github.com/apache/zookeeper/blob/087abf83684167ae56864fe4c3be0079fa653266/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java#L428] and LearnerHandler.java:
> {code}
> volatile int tick;
> {code}
> I don't think it needs volatile here. The tick is incremented only in [Leader.java|https://github.com/apache/zookeeper/blob/087abf83684167ae56864fe4c3be0079fa653266/src/java/main/org/apache/zookeeper/server/quorum/Leader.java#L590]:
> {code}
> synchronized (this) {
> ...
> if (!tickSkip) {
> self.tick++;
> }
> }
> {code}
> and it's protected by the synchronized statement. I just remove volatile keyword.
--
This message was sent by Atlassian JIRA
(v6.2#6252)