[GitHub] [zookeeper] eolivelli commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

[GitBox <gi...@apache.org>]

eolivelli commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r759023451



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       if you want to use `stateChangeMutex` to handle concurrent access to `state` you have to use it at every read/write access.
   
   Using AtomicReference is better and easier to understand.
   I suggest to switch to `AtomicReference` and drop `stateChangeMutex`




-- 
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