You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/12/29 10:56:53 UTC
[GitHub] [zookeeper] sonatype-lift[bot] commented on a change in pull request #1789: ZOOKEEPER-4432: Backport ZOOKEEPER-3109 to branch-3.5
sonatype-lift[bot] commented on a change in pull request #1789:
URL: https://github.com/apache/zookeeper/pull/1789#discussion_r776268265
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
##########
@@ -418,6 +419,13 @@ public void run() {
*/
Vote current = self.getCurrentVote();
if(ackstate == QuorumPeer.ServerState.LOOKING){
+ if (self.leader != null) {
+ if (leadingVoteSet != null) {
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `FastLeaderElection$Messenger$WorkerReceiver.run()` reads without synchronization from `this.this$1.this$0.leadingVoteSet`. Potentially races with write in method `FastLeaderElection$Messenger$WorkerReceiver.run()`.
Reporting because this access may occur on a background thread.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
##########
@@ -418,6 +419,13 @@ public void run() {
*/
Vote current = self.getCurrentVote();
if(ackstate == QuorumPeer.ServerState.LOOKING){
+ if (self.leader != null) {
+ if (leadingVoteSet != null) {
+ self.leader.setLeadingVoteSet(leadingVoteSet);
+ leadingVoteSet = null;
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `FastLeaderElection$Messenger$WorkerReceiver.run()` writes to field `this.this$1.this$0.leadingVoteSet` outside of synchronization.
Reporting because this access may occur on a background thread.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
##########
@@ -998,10 +1025,9 @@ else if (validVoter(n.sid) && validVoter(n.leader)) {
* relevant message from the reception queue
*/
if (n == null) {
- self.setPeerState((proposedLeader == self.getId()) ?
- ServerState.LEADING: learningState());
+ setPeerState(proposedLeader, voteSet);
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `FastLeaderElection.lookForLeader()` indirectly writes to field `this.leadingVoteSet` outside of synchronization.
Reporting because this access may occur on a background thread.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
##########
@@ -663,6 +671,7 @@ public void shutdown(){
stop = true;
proposedLeader = -1;
proposedZxid = -1;
+ leadingVoteSet = null;
Review comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `FastLeaderElection.shutdown()` writes to field `this.leadingVoteSet` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java
##########
@@ -1201,6 +1201,60 @@ synchronized public long startForwarding(LearnerHandler handler,
}
// VisibleForTesting
protected final Set<Long> connectingFollowers = new HashSet<Long>();
+
+ private volatile boolean quitWaitForEpoch = false;
+ private volatile long timeStartWaitForEpoch = -1;
+ private volatile SyncedLearnerTracker voteSet;
+
+ public static final String MAX_TIME_TO_WAIT_FOR_EPOCH = "zookeeper.leader.maxTimeToWaitForEpoch";
+ private static int maxTimeToWaitForEpoch;
+ static {
+ maxTimeToWaitForEpoch = Integer.getInteger(MAX_TIME_TO_WAIT_FOR_EPOCH, -1);
+ LOG.info("{} = {}ms", MAX_TIME_TO_WAIT_FOR_EPOCH, maxTimeToWaitForEpoch);
+ }
+
+ // visible for test
+ public static void setMaxTimeToWaitForEpoch(int maxTimeToWaitForEpoch) {
+ Leader.maxTimeToWaitForEpoch = maxTimeToWaitForEpoch;
+ LOG.info("Set {} to {}ms", MAX_TIME_TO_WAIT_FOR_EPOCH, Leader.maxTimeToWaitForEpoch);
+ }
+
+ /**
+ * Quit condition:
+ *
+ * 1 voter goes to looking again and time waitForEpoch > maxTimeToWaitForEpoch
+ *
+ * Note: the voter may go to looking again in case of:
+ * 1. change mind in the last minute when received a different notification
+ * 2. the leader hadn't started leading when it tried to connect to it
+ * 3. connection broken between the voter and leader
+ * 4. voter being shutdown or restarted
+ */
+ private void quitLeading() {
+ synchronized(connectingFollowers) {
+ quitWaitForEpoch = true;
+ connectingFollowers.notifyAll();
+ }
+ LOG.info("Quit leading due to voter changed mind.");
+ }
+
+ public void setLeadingVoteSet(SyncedLearnerTracker voteSet) {
+ this.voteSet = voteSet;
+ }
+
+ public void reportLookingSid(long sid) {
+ if (maxTimeToWaitForEpoch < 0 || timeStartWaitForEpoch < 0
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `Leader.reportLookingSid(...)` reads without synchronization from `this.waitingForNewEpoch`. Potentially races with write in method `Leader.lead()`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org